Bug 34160 – [Qt] Add support for Maemo zoom keys in QtLauncher

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Andreas Kling]

[Qt] Add support for Maemo zoom keys in QtLauncher

[Andreas Kling]

Created attachment 47399 [details]
Patch

[WebKit Review Bot]

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.

[Daniel Bates]

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 "..."?

[Antonio Gomes]

(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

[Simon Hausmann]

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 :)

[Andreas Kling]

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

[WebKit Commit Bot]

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

[Andreas Kling]

Created attachment 47693 [details]
Same patch, updated to merge

[Kenneth Rohde Christiansen]

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?

[Andreas Kling]

Created attachment 47705 [details]
Same patch, issues addressed

Thanks for the comments, Kenneth!

[WebKit Commit Bot]

Comment on attachment 47705 [details]
Same patch, issues addressed

Clearing flags on attachment: 47705

Committed r54058: <http://trac.webkit.org/changeset/54058>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.