Bug 34160 - [Qt] Add support for Maemo zoom keys in QtLauncher
Summary: [Qt] Add support for Maemo zoom keys in QtLauncher
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-26 04:33 PST by Andreas Kling
Modified: 2010-01-29 06:00 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.04 KB, patch)
2010-01-26 04:34 PST, Andreas Kling
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
Patch (2.50 KB, patch)
2010-01-27 02:02 PST, Andreas Kling
hausmann: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Same patch, updated to merge (2.52 KB, patch)
2010-01-29 02:46 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Same patch, issues addressed (2.58 KB, patch)
2010-01-29 05:21 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2010-01-26 04:33:32 PST
[Qt] Add support for Maemo zoom keys in QtLauncher
Comment 1 Andreas Kling 2010-01-26 04:34:34 PST
Created attachment 47399 [details]
Patch
Comment 2 WebKit Review Bot 2010-01-26 04:37:38 PST
Attachment 47399 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/QtLauncher/main.cpp:106:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/QtLauncher/main.cpp:107:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/QtLauncher/main.cpp:213:  QEvent_KeyPress is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Daniel Bates 2010-01-26 15:37:28 PST
I am not too familiar with Maemo or its supported devices, but is there a reason why you chose the keys F7 and F8 for zooming in and out, respectively, as opposed to conforming to the + and - keys used currently by QtLauncher, Safari, and Firefox?

Also, wouldn't this change break current functionality:

-        } else if (event->type() == QEvent::KeyPress
+        } else if (event->type() == QEvent_KeyPress

You seem to point this out by your comment:
// MAEMO: The X11 headers macro "KeyPress" which breaks "QEvent::KeyPress"...

Moreover, can you elaborate what you implying by the "..."?
Comment 4 Antonio Gomes 2010-01-26 19:03:04 PST
(In reply to comment #3)
> I am not too familiar with Maemo or its supported devices, but is there a
> reason why you chose the keys F7 and F8 for zooming in and out, respectively,
> as opposed to conforming to the + and - keys used currently by QtLauncher,
> Safari, and Firefox?

the zoom dedicated HW keys are mapped to f7 and f8 on maemo
Comment 5 Simon Hausmann 2010-01-27 01:25:52 PST
Comment on attachment 47399 [details]
Patch


> +
> +#ifdef Q_WS_MAEMO_5
> +    ~MainWindow()
> +    {
> +        grabZoomKeys(false);
>      }
> +#endif

I suggest to move the #ifdefs into the function body. (as well as in keyPressEvent)

>  #if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
>      void sendTouchEvent()
> @@ -193,6 +209,9 @@ public:
>  
>      bool eventFilter(QObject* obj, QEvent* event)
>      {
> +        // MAEMO: The X11 headers macro "KeyPress" which breaks "QEvent::KeyPress"...
> +        const int QEvent_KeyPress = 6;

A better way to fix this is to #undef KeyPress after including the X11 headers.


The rest looks good to me :)
Comment 6 Andreas Kling 2010-01-27 02:02:28 PST
Created attachment 47512 [details]
Patch

Same patch with raised issues addressed.
For info on the F7/F8 keys, see http://qt.nokia.com/doc/qt-maemo-4.6/maemo5-zoom.html
Comment 7 WebKit Commit Bot 2010-01-27 11:27:01 PST
Comment on attachment 47512 [details]
Patch

Rejecting patch 47512 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Simon Hausmann', '--force']" exit_code: 1
patching file WebKitTools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKitTools/QtLauncher/main.cpp
Hunk #1 FAILED at 50.
Hunk #2 succeeded at 118 (offset -55 lines).
Hunk #3 succeeded at 401 (offset -55 lines).
1 out of 3 hunks FAILED -- saving rejects to file WebKitTools/QtLauncher/main.cpp.rej

Full output: http://webkit-commit-queue.appspot.com/results/215236
Comment 8 Andreas Kling 2010-01-29 02:46:48 PST
Created attachment 47693 [details]
Same patch, updated to merge
Comment 9 Kenneth Rohde Christiansen 2010-01-29 03:46:42 PST
Comment on attachment 47693 [details]
Same patch, updated to merge


> +#ifdef Q_WS_MAEMO_5
> +        grabZoomKeys(false);
> +#endif

I would prefer having the ifdef tests inside grabZoomKeys instead.

>        if (!winId())
>            qWarning("can't grab keys unless we have a window id");

Why no return after this?
Comment 10 Andreas Kling 2010-01-29 05:21:05 PST
Created attachment 47705 [details]
Same patch, issues addressed

Thanks for the comments, Kenneth!
Comment 11 WebKit Commit Bot 2010-01-29 06:00:34 PST
Comment on attachment 47705 [details]
Same patch, issues addressed

Clearing flags on attachment: 47705

Committed r54058: <http://trac.webkit.org/changeset/54058>
Comment 12 WebKit Commit Bot 2010-01-29 06:00:45 PST
All reviewed patches have been landed.  Closing bug.