Bug 44836 - [Qt] Support click event for notifications
Summary: [Qt] Support click event for notifications
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yael
URL:
Keywords: Qt, QtTriaged
Depends on: 44800 45352
Blocks: 39995
  Show dependency treegraph
 
Reported: 2010-08-29 06:12 PDT by Yael
Modified: 2010-09-13 07:25 PDT (History)
3 users (show)

See Also:


Attachments
Patch (14.11 KB, patch)
2010-09-07 19:45 PDT, Yael
akling: review+
Details | Formatted Diff | Diff
Patch (14.11 KB, patch)
2010-09-10 09:32 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2010-08-29 06:12:11 PDT
Notifications should support a click event.
Comment 1 Ademar Reis 2010-09-01 14:31:29 PDT
Now that Bug #44800 has been fixed, is anything else needed on the Qt side?

This is the only bug left blocking Bug #39995
Comment 2 Yael 2010-09-01 16:58:29 PDT
(In reply to comment #1)
> Now that Bug #44800 has been fixed, is anything else needed on the Qt side?
> 
> This is the only bug left blocking Bug #39995

There are more requirements coming for Notificatons in Qt. I updated the list in #3995 recently.
Comment 3 Yael 2010-09-01 16:59:09 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Now that Bug #44800 has been fixed, is anything else needed on the Qt side?
> > 
> > This is the only bug left blocking Bug #39995
> 
> There are more requirements coming for Notificatons in Qt. I updated the list in #3995 recently.
That is bug #39995, of course :-)
Comment 4 Ademar Reis 2010-09-06 10:13:47 PDT
OK, so bug #39995 has some tasks left, but I guess we can at least close this one, right? Or do we still need something on the Qt side to support click events for notifications?
Comment 5 Yael 2010-09-06 15:41:46 PDT
(In reply to comment #4)
> OK, so bug #39995 has some tasks left, but I guess we can at least close this one, right? Or do we still need something on the Qt side to support click events for notifications?

Yes, I am currently working on the Qt implementation of the click event.
Comment 6 Yael 2010-09-07 19:45:52 PDT
Created attachment 66833 [details]
Patch

Add support for sending a click event when either QSystemTrayIcon is clicked, or the platform plugin's notification is clicked.
Also added a new method NotificationWrapper::openerPageUrl(), so that if the platform plugin wants to open the same url in a new page, when the user clicks the notification, it can do that too.
I added that somewhat related method here since I did not want to bump the plugin version twice.
Comment 7 Andreas Kling 2010-09-10 02:41:07 PDT
Comment on attachment 66833 [details]
Patch

> +    if (notification && notification->scriptExecutionContext())
> +        sendEvent(notification, eventNames().clickEvent);

Redundancy: sendEvent() will also check notification->scriptExecutionContext() before proceeding.

> +    if (ev->type() == QEvent::MouseButtonRelease) {
> +        emit notificationClicked();

Are you sure the click event should fire for all mouse buttons?
Comment 8 Yael 2010-09-10 09:03:55 PDT
(In reply to comment #7)
Thank you for the review, Andreas.
> (From update of attachment 66833 [details])
> > +    if (notification && notification->scriptExecutionContext())
> > +        sendEvent(notification, eventNames().clickEvent);
> 
> Redundancy: sendEvent() will also check notification->scriptExecutionContext() before proceeding.
I'll remove the second part of the check. However, the first part is still needed.
> 
> > +    if (ev->type() == QEvent::MouseButtonRelease) {
> > +        emit notificationClicked();
> 
> Are you sure the click event should fire for all mouse buttons?
This is _example_ code, and I don't think we care much about that.
Comment 9 Andreas Kling 2010-09-10 09:08:47 PDT
Comment on attachment 66833 [details]
Patch

r=me
Comment 10 Yael 2010-09-10 09:32:31 PDT
Created attachment 67194 [details]
Patch

Remove redundnt check for notification->ScriptExecutionContext.
Comment 11 WebKit Commit Bot 2010-09-10 20:42:38 PDT
Comment on attachment 67194 [details]
Patch

Clearing flags on attachment: 67194

Committed r67272: <http://trac.webkit.org/changeset/67272>
Comment 12 WebKit Commit Bot 2010-09-10 20:42:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Ademar Reis 2010-09-13 06:59:18 PDT
Revision r67272 cherry-picked into qtwebkit-2.1 with commit 7cb97137ea15daf78870cf1b4028e0bb17784c56