Bug 84591 - [Qt] Restore x-position when restoring previous zoom-level
: [Qt] Restore x-position when restoring previous zoom-level
Status: RESOLVED FIXED
: WebKit
WebKit2
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 84602
  Show dependency treegraph
 
Reported: 2012-04-23 06:17 PST by
Modified: 2012-04-24 02:41 PST (History)


Attachments
Patch (5.98 KB, patch)
2012-04-23 06:25 PST, Allan Sandfeld Jensen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.18 KB, patch)
2012-04-23 08:48 PST, Allan Sandfeld Jensen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.02 KB, patch)
2012-04-23 09:14 PST, Allan Sandfeld Jensen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2012-04-24 01:12 PST, Allan Sandfeld Jensen
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-23 06:17:02 PST
When zooming out on tap to zoom the previous zoom-level is reestablished, but the previous position is not. 

The X-position needs to be restored to ensure the columns that were visible before zooming-in will be visible again after zooming out.

Y-position does not need to be restored since zooming is fit-to-width, and moving Y-position is an intended feature of double-tap.
------- Comment #1 From 2012-04-23 06:25:25 PST -------
Created an attachment (id=138333) [details]
Patch
------- Comment #2 From 2012-04-23 06:51:50 PST -------
(From update of attachment 138333 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=138333&action=review

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:96
> +static inline bool fuzzyCompare(qreal a, qreal b, qreal f)

qFuzzyCompare is not good enough?
------- Comment #3 From 2012-04-23 06:56:44 PST -------
(In reply to comment #2)
> (From update of attachment 138333 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138333&action=review
> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:96
> > +static inline bool fuzzyCompare(qreal a, qreal b, qreal f)
> 
> qFuzzyCompare is not good enough?

I tried it, but I still had cases where a double-tap didn't zoom out, and qFuzzyCompare couldn't take an argument for epsilon. I am kinda of surprised I couldn't find a proper Qt floating-point compare with epsilon argument.
------- Comment #4 From 2012-04-23 08:48:05 PST -------
Created an attachment (id=138355) [details]
Patch
------- Comment #5 From 2012-04-23 08:51:42 PST -------
Attachment 138355 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #6 From 2012-04-23 09:04:44 PST -------
(From update of attachment 138355 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=138355&action=review
> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:99
> +    return std::abs(a - b) < epsilon;

I think you could use qAbs() here, then you wouldn't need cmath.
Looks good otherwise.
------- Comment #7 From 2012-04-23 09:06:19 PST -------
(In reply to comment #5)
> Attachment 138355 [details] [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 3 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Style-bot is wrong by the way. The includes are sorted with system-includes after other includes.
------- Comment #8 From 2012-04-23 09:14:15 PST -------
(In reply to comment #7)
> (In reply to comment #5)
> > Attachment 138355 [details] [details] [details] did not pass style-queue:
> > 
> > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
> > Total errors found: 1 in 3 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> Style-bot is wrong by the way. The includes are sorted with system-includes after other includes.

I think the bot uses C locale for sort (LC_COLLATE=C) which would expect cmath after QWheelEvent.
------- Comment #9 From 2012-04-23 09:14:43 PST -------
Created an attachment (id=138361) [details]
Patch
------- Comment #10 From 2012-04-23 09:32:58 PST -------
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > Attachment 138355 [details] [details] [details] [details] did not pass style-queue:
> > > 
> > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> > > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
> > > Total errors found: 1 in 3 files
> > > 
> > > 
> > > If any of these errors are false positives, please file a bug against check-webkit-style.
> > 
> > Style-bot is wrong by the way. The includes are sorted with system-includes after other includes.
> 
> I think the bot uses C locale for sort (LC_COLLATE=C) which would expect cmath after QWheelEvent.

That was where I first placed it. There was no place the style-checker would accept it. Anyway it is gone now, qAbs was a better choice.
------- Comment #11 From 2012-04-23 09:47:36 PST -------
(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > > Attachment 138355 [details] [details] [details] [details] [details] did not pass style-queue:
> > > > 
> > > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> > > > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
> > > > Total errors found: 1 in 3 files
> > > > 
> > > > 
> > > > If any of these errors are false positives, please file a bug against check-webkit-style.
> > > 
> > > Style-bot is wrong by the way. The includes are sorted with system-includes after other includes.
> > 
> > I think the bot uses C locale for sort (LC_COLLATE=C) which would expect cmath after QWheelEvent.
> 
> That was where I first placed it. There was no place the style-checker would accept it. Anyway it is gone now, qAbs was a better choice.

That's probably because your system has something like en_US.UTF-8 as a locale, 
using LC_COLLATE=C Tools/Scripts/check-webkit-style might give the correct result.
------- Comment #12 From 2012-04-24 01:12:08 PST -------
Created an attachment (id=138512) [details]
Patch
------- Comment #13 From 2012-04-24 01:13:51 PST -------
discussed on #irc. the updated(coming) patch looks good. :)
------- Comment #14 From 2012-04-24 02:41:51 PST -------
(From update of attachment 138512 [details])
Clearing flags on attachment: 138512

Committed r115022: <http://trac.webkit.org/changeset/115022>
------- Comment #15 From 2012-04-24 02:41:57 PST -------
All reviewed patches have been landed.  Closing bug.