Bug 11272 - Implement currentScale(), setCurrentScale() and currentTranslate() in SVGSVGElement
Summary: Implement currentScale(), setCurrentScale() and currentTranslate() in SVGSVGE...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-10-13 01:11 PDT by Rob Buis
Modified: 2007-05-31 19:02 PDT (History)
0 users

See Also:


Attachments
First attempt (16.58 KB, patch)
2007-05-15 05:21 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Now should compile (16.36 KB, patch)
2007-05-16 01:24 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Using zoomFactor (15.63 KB, patch)
2007-05-19 12:09 PDT, Rob Buis
mjs: review-
Details | Formatted Diff | Diff
Without the webkit changes (12.97 KB, patch)
2007-05-30 07:25 PDT, Rob Buis
darin: review-
Details | Formatted Diff | Diff
Cleaning up (12.55 KB, patch)
2007-05-30 08:35 PDT, Rob Buis
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2006-10-13 01:11:46 PDT
Implement the getting/setting of the property currentScale and getting of property currentTranslate so the FIXMEs can be removed.
Comment 1 Rob Buis 2007-05-15 05:21:45 PDT
Created attachment 14563 [details]
First attempt

This patch allows simple zooming and panning. I think some of the values can be stored in the svg container in the render tree as well, but that would need first some cleanup of the svg render classes, as
already listed in another bug report. Nevertheless I think this patch is valuable, going into the experimental branch first is also an option. Panning should work on all platforms, zooming only on Mac (since AFAIK others have no context menu's where I can easily add Zoom entries to).
Cheers,

Rob.
Comment 2 Rob Buis 2007-05-16 01:24:05 PDT
Created attachment 14579 [details]
Now should compile

Olliej noticed the previous patch didnt compile, this one should.
Cheers,

Rob.
Comment 3 Nikolas Zimmermann 2007-05-16 15:27:56 PDT
Hi Rob,

I think the incZoom() etc methods should at least go into SVGDocument,
if not somewhere else where it fits better. Suggestions?

Greetings,
Niko
Comment 4 Rob Buis 2007-05-17 02:59:08 PDT
Hi Niko,

(In reply to comment #3)
> Hi Rob,
> 
> I think the incZoom() etc methods should at least go into SVGDocument,
> if not somewhere else where it fits better. Suggestions?

They are in SVGDocument now :)
I also added them to Document.h since SVGDocument.h was not visible for WebKit. I now know it can
be made visible. We could stick with the methods being only in SVGDocument, but some more #if ENABLE(SVG) constructions would be needed then, which are not always considered nice. Anyway this and other issues will probably come up during reviewing. Also notice that probably the svg render object handling the outer <svg> should know enough about it to ask the zoom and pan factor from
the SVGSVGElement, and other svg container render objects don't need to know about these values. But I propose to do that later (there already is a bug report for it).
Cheers,

Rob.
Comment 5 Maciej Stachowiak 2007-05-17 17:42:03 PDT
Instead of having SVG-specific zoom in and out menu items, I would rather see SVG work off of the scale factor that is used for text zoom in HTML. Then Cmd-plus and Cmd-minus would work in apps that bind those to text zoom, as would the menu items. I think on mac at least we already do this for PDF.

Comment 6 Rob Buis 2007-05-19 12:09:54 PDT
Created attachment 14629 [details]
Using zoomFactor

This version uses zoomFactor from the Frame. Note that it behaves a bit differently, ie. the root svg increases/decreases along the zoom, also when embedded. I noticed this behaviour when testing with Opera, and it seems intuitive.
Cheers,

Rob.
Comment 7 Maciej Stachowiak 2007-05-29 01:42:57 PDT
I would suggest not adding the zoom items to the context menu - I think the menu items, keyboard shortcuts, and optional toolbar buttons are more than enough UI for zooming. The rest of this seems good to go for the feature branch.
Comment 8 Maciej Stachowiak 2007-05-29 01:44:14 PDT
Comment on attachment 14629 [details]
Using zoomFactor

r- for the context menu items, I think it's best not to add those and instead we should make the text zoom menu item labels dynamic.
Comment 9 Rob Buis 2007-05-30 07:25:09 PDT
Created attachment 14782 [details]
Without the webkit changes

As agreed, this time without the webkit changes to the context menu.
Cheers,

Rob.
Comment 10 Darin Adler 2007-05-30 07:50:03 PDT
Comment on attachment 14782 [details]
Without the webkit changes

I don't think we should necessarily mix the zoom factor together with the text size multiplier. The text size multiplier is still called "zoomFactor" in Frame.cpp, but I think we may want a separate way to manipulate the zoom factor and text size multiplier independently. And I think a zoom factor should zoom everything, not just SVG content.

+    if (document() && document()->frame())
+        return float(document()->frame()->zoomFactor()) / 100.;
+    return 1.0;

The right way to write the above so it will be floating point is:

    if (document() && document()->frame())
        return document()->frame()->zoomFactor() / 100.0f;
    return 1.0f;

There's no need for the cast to "float".

+        document()->frame()->setZoomFactor(scale / 100.);

This will do the math as double. You want "scale / 100.0f".

The word "translate" is a verb not a noun. It should be "translation" instead in the various function names.

-    //canvasView()->enableZoomAndPan(zoomAndPan == SVG_ZOOMANDPAN_MAGNIFY);

We normally don't check in commented-out code. if we must check this in, it needs a comment to indicate why it's commented out.

         return m_lastScrollbarUnderMouse->handleMouseMoveEvent(mouseEvent);
-
     // Treat mouse move events while the mouse is pressed as "read-only" in prepareMouseEvent

Why remove this blank line?

+       static_cast<SVGDocument*>(m_frame->document())->zoomEnabled()) {

Why does zoomEnabled() control panning? Should it be named zoomAndPanEnabled() instead?

+        AffineTransform ctm;

This local variable is unused?

+        m_width = float(m_width) * svg->currentScale();
+        m_height = float(m_height) * svg->currentScale();

No need for the cast to float here.

review- at least for the "translate" name.
Comment 11 Rob Buis 2007-05-30 08:34:23 PDT
Hi Darin,

(In reply to comment #10)
> (From update of attachment 14782 [details] [edit])
> I don't think we should necessarily mix the zoom factor together with the text
> size multiplier. The text size multiplier is still called "zoomFactor" in
> Frame.cpp, but I think we may want a separate way to manipulate the zoom factor
> and text size multiplier independently. And I think a zoom factor should zoom
> everything, not just SVG content.

Right, Text zoom seems very specific and doesn't translate well to the svg world. My original patch had them separated.

> +    if (document() && document()->frame())
> +        return float(document()->frame()->zoomFactor()) / 100.;
> +    return 1.0;
> 
> The right way to write the above so it will be floating point is:
> 
>     if (document() && document()->frame())
>         return document()->frame()->zoomFactor() / 100.0f;
>     return 1.0f;
> 
> There's no need for the cast to "float".

Fixed.

> +        document()->frame()->setZoomFactor(scale / 100.);
> 
> This will do the math as double. You want "scale / 100.0f".

Fixed.
 
> The word "translate" is a verb not a noun. It should be "translation" instead
> in the various function names.

Fixed.

> -    //canvasView()->enableZoomAndPan(zoomAndPan == SVG_ZOOMANDPAN_MAGNIFY);
> 
> We normally don't check in commented-out code. if we must check this in, it
> needs a comment to indicate why it's commented out.

Its being removed here :)

>          return m_lastScrollbarUnderMouse->handleMouseMoveEvent(mouseEvent);
> -
>      // Treat mouse move events while the mouse is pressed as "read-only" in
> prepareMouseEvent
> 
> Why remove this blank line?

No particular reason. Fixed.

> +       static_cast<SVGDocument*>(m_frame->document())->zoomEnabled()) {
> 
> Why does zoomEnabled() control panning? Should it be named zoomAndPanEnabled()
> instead?

Agreed. Fixed.

> +        AffineTransform ctm;
> 
> This local variable is unused?

Maybe copy and paste error. Fixed.

> +        m_width = float(m_width) * svg->currentScale();
> +        m_height = float(m_height) * svg->currentScale();
> 
> No need for the cast to float here.

Fixed.
Cheers,

Rob.
Comment 12 Rob Buis 2007-05-30 08:35:26 PDT
Created attachment 14784 [details]
Cleaning up

This should address Darin's issues.
Cheers,

Rob.
Comment 13 Darin Adler 2007-05-30 09:18:04 PDT
Comment on attachment 14784 [details]
Cleaning up

+        rootElement()->setCurrentTranslate(FloatPoint(pos.x() - m_translate.x(), m_translate.y() - pos.y()));
+        if (renderer())
+            renderer()->repaint();

Shouldn't setCurrentTranslate be the code doing the repaint?

+        FloatPoint m_translate;

I think this should be removed.

+    if (m_svgPan) {
+        m_svgPan = false;
+        return true;
+    }

I think we want to do a final "updatePan" at the end location.

+        m_width = float(m_width) * svg->currentScale();
+        m_height = float(m_height) * svg->currentScale();

This still has the cast, even though you mentioned removing it in your comments.

review+, although I think this needs a little refinement
Comment 14 Rob Buis 2007-05-30 10:20:03 PDT
Hi Darin,

(In reply to comment #13)
> (From update of attachment 14784 [details] [edit])
> +        rootElement()->setCurrentTranslate(FloatPoint(pos.x() -
> m_translate.x(), m_translate.y() - pos.y()));
> +        if (renderer())
> +            renderer()->repaint();
> 
> Shouldn't setCurrentTranslate be the code doing the repaint?

Right, that is better, will do.

> +        FloatPoint m_translate;
> 
> I think this should be removed.

I am not sure that is possible. We have to keep track of intermendiate results like that I think.

> +    if (m_svgPan) {
> +        m_svgPan = false;
> +        return true;
> +    }
> 
> I think we want to do a final "updatePan" at the end location.

Right, will fix.

> +        m_width = float(m_width) * svg->currentScale();
> +        m_height = float(m_height) * svg->currentScale();
> 
> This still has the cast, even though you mentioned removing it in your
> comments.

Right, I searched for (float), should have searched for float(. Will also fix.
Patch will go to the feature branch.
Cheers,

Rob.
Comment 15 Sam Weinig 2007-05-31 19:02:53 PDT
Landed in r21925.