Opened on 04/26/2015 at 09:43:17 AM
Closed on 06/09/2015 at 11:10:14 AM
#2420 closed change (fixed)
[Move notification display logic to Core] Make it possible to register a notification show listener
| Reported by: | fhd | Assignee: | fhd |
|---|---|---|---|
| Priority: | P2 | Milestone: | Adblock-Plus-2.6.10-for-Firefox |
| Module: | Core | Keywords: | |
| Cc: | trev, sergz | Blocked By: | |
| Blocking: | #2366, #2367, #2368, #2387, #2419 | Platform: | Unknown |
| Ready: | yes | Confidential: | no |
| Tester: | Verified working: | no | |
| Review URL(s): |
http://codereview.adblockplus.org/5256408131960832/ |
||
Description (last modified by fhd)
Background
See #2366.
What to change
Add API for registering a notification show listener:
- Notification.addShowListener()
- Notification.removeShowListener()
The listeners should be triggered three minutes after the Notification module is initialised.
In addition, don't automatically mark non-question notifications as shown when they are retrieved anymore, clients should do this explicitly by calling Notification.markAsShown().
Attachments (0)
Change History (16)
comment:1 Changed on 04/26/2015 at 09:44:12 AM by fhd
- Summary changed from Make it possible to register a notification show listener to [Move notification display logic to Core] Make it possible to register a notification show listener
comment:2 Changed on 04/26/2015 at 09:45:13 AM by fhd
- Blocking 2367 added
comment:3 Changed on 04/26/2015 at 09:48:20 AM by fhd
- Blocking 2368 added
comment:4 Changed on 04/26/2015 at 09:51:14 AM by fhd
- Blocking 2366 added
comment:5 Changed on 04/26/2015 at 09:57:23 AM by fhd
- Blocking 2387 added
comment:6 Changed on 04/26/2015 at 10:05:09 AM by fhd
- Cc trev added
comment:7 Changed on 04/26/2015 at 10:06:10 AM by fhd
- Blocking 2419 added
comment:8 Changed on 04/27/2015 at 07:08:21 AM by sergz
- Description modified (diff)
Quote from discussion
sergz: The core could notify the client using the callback. When the notification is dismissed (for example, closed by user) the client should notify the core about it.
In this case we still can show notification on startup, we can show notification with some delay after startup, we can show notification when it's downloaded and we can show the next notification when the previous one is closed or schedule it to show later. I would say that we could even update the current notification if there is a new notification which is an emergency notification. It seems that in this case we don't have to spread the same logic (with magic constants) among all clients and we can change it in one place for all clients.
...
palant: the clients will have to mark the notification as shown explicitly then, so that the core code knows when the next notification can be shown. but - well, sounds sane to me.
comment:9 follow-up: ↓ 10 Changed on 04/27/2015 at 08:59:34 AM by fhd
My intention was to just assume a notification has been shown once the listener has been invoked - what's the problem with that?
comment:10 in reply to: ↑ 9 Changed on 05/18/2015 at 09:36:03 AM by sergz
- Cc sergz added
Replying to fhd:
My intention was to just assume a notification has been shown once the listener has been invoked - what's the problem with that?
Except the case when the listener is invoked just before the closing of the browser and the user won't see it, it's fine for me.
However the core should not call the listener to show the next notification until the previous one is not closed.
comment:11 Changed on 06/01/2015 at 03:56:00 PM by fhd
- Owner set to fhd
comment:12 Changed on 06/08/2015 at 10:57:25 AM by fhd
- Description modified (diff)
comment:13 Changed on 06/08/2015 at 10:58:04 AM by fhd
- Ready set
comment:14 Changed on 06/08/2015 at 11:14:30 AM by fhd
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:15 Changed on 06/08/2015 at 11:29:55 AM by fhd
- Review URL(s) modified (diff)
comment:16 Changed on 06/09/2015 at 11:10:14 AM by fhd
- Milestone set to Adblock-Plus-for-Firefox-next
- Resolution set to fixed
- Status changed from reviewing to closed
Haven't set this to ready yet - please have a look Wladimir.