Bug 64541 - [Qt] Consider the scale of the page view when scrolling QTouchWebView.
: [Qt] Consider the scale of the page view when scrolling QTouchWebView.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Jocelyn Turcotte
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-14 10:39 PDT by Jocelyn Turcotte
Modified: 2011-07-18 06:48 PDT (History)
1 user (show)

See Also:


Attachments
Patch (1.46 KB, patch)
2011-07-14 10:42 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (1.49 KB, patch)
2011-07-15 09:25 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (1.97 KB, patch)
2011-07-15 10:19 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (1.95 KB, patch)
2011-07-18 05:21 PDT, Jocelyn Turcotte
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2011-07-14 10:39:06 PDT
[Qt] Consider the scale of the page view when scrolling QTouchWebView.
Comment 1 Jocelyn Turcotte 2011-07-14 10:42:18 PDT
Created attachment 100829 [details]
Patch
Comment 2 Benjamin Poulain 2011-07-15 08:42:52 PDT
Comment on attachment 100829 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100829&action=review

> Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:42
> +    pageView->moveBy(deltaX * pageView->scale(), deltaY * pageView->scale());

I think TouchViewInterface::panGestureRequestScroll() should use m_viewportView->mapFromItem() and send the coordinate in the right space to the viewport.

QTouchWebViewPrivate::scroll(qreal deltaX, qreal deltaY) should be in viewport coordinate.
Comment 3 Jocelyn Turcotte 2011-07-15 09:25:52 PDT
Created attachment 100993 [details]
Patch

Use item mapping functions instead.
Comment 4 Jocelyn Turcotte 2011-07-15 09:31:49 PDT
(In reply to comment #2)
> I think TouchViewInterface::panGestureRequestScroll() should use m_viewportView->mapFromItem() and send the coordinate in the right space to the viewport.
> 
> QTouchWebViewPrivate::scroll(qreal deltaX, qreal deltaY) should be in viewport coordinate.

Humm didn't see your comment, since the method is on the viewport it would make sense to give it viewport coordinates. Cooking a new patch.
Comment 5 Jocelyn Turcotte 2011-07-15 10:19:03 PDT
Created attachment 101001 [details]
Patch

Move the mapping to TouchViewInterface
Comment 6 Benjamin Poulain 2011-07-15 10:35:13 PDT
Comment on attachment 101001 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101001&action=review

> Source/WebKit2/UIProcess/qt/TouchViewInterface.cpp:51
> +    // This assumes that m_viewportView's and m_pageView parent's coordinate systems match.
> +    QPointF destInViewportCoords = m_pageView->mapToParent(m_pageView->mapFromParent(m_pageView->pos()) + QPointF(deltaX, deltaY));
> +    QPointF offsetInViewportCoords = destInViewportCoords - m_pageView->pos();

I don't understand why you do all this.
Isn't it 
QPointF destInViewportCoords = m_viewportView->mapFromItem(m_pageView, QPointF(deltaX, deltaY)); ?

The 2 delta is like a vector, you can just multiply it by the combined transformation matrix of a subtree to get the transformed vector.  Or am I missing something?
Comment 7 Jocelyn Turcotte 2011-07-15 11:10:17 PDT
(In reply to comment #6)
> I don't understand why you do all this.
> Isn't it 
> QPointF destInViewportCoords = m_viewportView->mapFromItem(m_pageView, QPointF(deltaX, deltaY)); ?
> 
> The 2 delta is like a vector, you can just multiply it by the combined transformation matrix of a subtree to get the transformed vector.  Or am I missing something?

I tried but it's not enough when there is also a translation transform. i.e. a (1,2) delta is mapped to (11,12) if there is a (10,10) translation on top.
Comment 8 Benjamin Poulain 2011-07-15 11:19:21 PDT
Comment on attachment 101001 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101001&action=review

Isn't m_pageView->mapFromParent(m_pageView->pos()) always gonna be zero though?
In page view coordinate, its own position would be the top left corner.

> Source/WebKit2/UIProcess/qt/TouchViewInterface.cpp:49
> +    // This assumes that m_viewportView's and m_pageView parent's coordinate systems match.

Personnally I would remove that comment and remove the assumption by using mapFromItem instead of mapFromParent :)
Comment 9 Jocelyn Turcotte 2011-07-15 14:49:57 PDT
(In reply to comment #8)
> (From update of attachment 101001 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101001&action=review
> 
> Isn't m_pageView->mapFromParent(m_pageView->pos()) always gonna be zero though?
> In page view coordinate, its own position would be the top left corner.
> 
That's a good point, and along that your code seems right in comment 6, but what you get is a vector from the viewport's origin toward the applied vector on the page view's origin. I didn't realize that this is the final destination in viewport coordinate and was trying to apply it as a vector again by sending it as the delta values.

> > Source/WebKit2/UIProcess/qt/TouchViewInterface.cpp:49
> > +    // This assumes that m_viewportView's and m_pageView parent's coordinate systems match.
> 
> Personnally I would remove that comment and remove the assumption by using mapFromItem instead of mapFromParent :)

That sounds like a good idea too, would also help fixing my assumptions instead of spreading them around :)
Comment 10 Jocelyn Turcotte 2011-07-18 05:21:28 PDT
Created attachment 101149 [details]
Patch

New patch using mapFromItem instead of mapToParent.
I tried assuming that mapFromParent(pos()) and remove the call, but the assumption wouldn't hold when scaling the item, event though the documentation says it should (it might not help that we set a different transform origin on that item.).
Comment 11 Benjamin Poulain 2011-07-18 05:24:46 PDT
Comment on attachment 101149 [details]
Patch

Great.
Comment 12 Jocelyn Turcotte 2011-07-18 06:48:56 PDT
Committed r91183: <http://trac.webkit.org/changeset/91183>