Opened 5 years ago

Closed 4 years ago

#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/
http://codereview.adblockplus.org/5499340072157184/

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().

Change History (16)

comment:1 Changed 5 years ago 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 5 years ago by fhd

  • Blocking 2367 added

comment:3 Changed 5 years ago by fhd

  • Blocking 2368 added

comment:4 Changed 5 years ago by fhd

  • Blocking 2366 added

comment:5 Changed 5 years ago by fhd

  • Blocking 2387 added

comment:6 Changed 5 years ago by fhd

  • Cc trev added

Haven't set this to ready yet - please have a look Wladimir.

comment:7 Changed 5 years ago by fhd

  • Blocking 2419 added

comment:8 Changed 5 years ago 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: Changed 5 years ago 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 5 years ago 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 5 years ago by fhd

  • Owner set to fhd

comment:12 Changed 4 years ago by fhd

  • Description modified (diff)

comment:13 Changed 4 years ago by fhd

  • Ready set

comment:14 Changed 4 years ago by fhd

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:15 Changed 4 years ago by fhd

  • Review URL(s) modified (diff)

comment:16 Changed 4 years ago by fhd

  • Milestone set to Adblock-Plus-for-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.