Summary: | [Qt] Support click event for notifications | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yael <yael> | ||||||
Component: | WebKit Misc. | Assignee: | Yael <yael> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ademar, commit-queue, kling | ||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 44800, 45352 | ||||||||
Bug Blocks: | 39995 | ||||||||
Attachments: |
|
Description
Yael
2010-08-29 06:12:11 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 (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. (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 :-) 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? (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. 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 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? (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 on attachment 66833 [details]
Patch
r=me
Created attachment 67194 [details]
Patch
Remove redundnt check for notification->ScriptExecutionContext.
Comment on attachment 67194 [details] Patch Clearing flags on attachment: 67194 Committed r67272: <http://trac.webkit.org/changeset/67272> All reviewed patches have been landed. Closing bug. Revision r67272 cherry-picked into qtwebkit-2.1 with commit 7cb97137ea15daf78870cf1b4028e0bb17784c56 |