Opened 2 years ago

Last modified 4 weeks ago

#2386 new change

Make it possible to register a notification download listener

Reported by: fhd Assignee:
Priority: P2 Milestone:
Module: Core Keywords:
Cc: trev, sebastian Blocked By:
Blocking: Platform: Unknown
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by trev)

Background

In #2366, we want to react to notification downloads - there's currently no non-hacky way to do that.

What to change

Add API for registering a notification download listener:

  • Notification.addNewNotificationListener()
  • Notification.removeNewNotificationListener()

The listeners should be triggered after downloading notifications but only if that download changed the result that Notification.getNextToShow() would return.

Change History (16)

comment:1 Changed 2 years ago by fhd

  • Cc trev added

Wladimir: Not set to ready yet, please have a look first.

comment:2 Changed 2 years ago by fhd

  • Blocking 2387 added

comment:3 Changed 2 years ago by trev

  • Description modified (diff)
  • Ready set

comment:4 Changed 2 years ago by sebastian

  • Cc sebastian added

For consistency (and personal preference) I'd rather go with:

  • Notification.addListener
  • Notification.removeListener

Or if this clashes with other notification related events:

  • Notification.onNewNofication.addListener
  • Notification.onNewNofication.removeListener

comment:5 Changed 2 years ago by fhd

While I don't completely like what's in the description either (e.g. not consistent with addQuestionListener), I can't really come up with anything better (addNewListener would be quite confusing e.g.). onNewNotification.addListener would be even more inconsistent than addNewNotificationListener.

Last edited 2 years ago by fhd (previous) (diff)

comment:6 Changed 2 years ago by sebastian

Alright, wasn't aware of addQuestionListener but thinking of the rest of our code.

comment:7 Changed 2 years ago by trev

Note that we aren't using event targets in the core code at this point. While I agree that we want to change the API in that direction (and resolve the inconsistency between Chrome's Prefs implementation and all other platforms), that change won't happen here.

comment:8 Changed 2 years ago by fhd

  • Blocking 2419 added

comment:9 Changed 2 years ago by fhd

  • Blocking 2367 removed

comment:10 Changed 2 years ago by fhd

  • Blocking 2368 removed

comment:11 Changed 2 years ago by fhd

  • Blocking 2387 removed

comment:12 Changed 2 years ago by fhd

  • Ready unset

Removed Ready for now. Since we agreed that we want to move the notification display logic to Core (#2366), the handlers we're adding here wouldn't actually be used outside the Notification module. I doubt that it makes sense to add these listeners just for #2419 - would be simpler to just trigger the show listener directly there.

comment:13 Changed 2 years ago by kzar

  • Owner set to kzar

comment:14 Changed 2 years ago by kzar

  • Owner kzar deleted

comment:15 Changed 2 years ago by fhd

  • Blocking 2419 removed

comment:16 Changed 4 weeks ago by sebastian

  • Tester set to Unknown

This sounds sensible, and meanwhile #2419 moved forward without this change, anyway. So it seems this issue should be rejected now, but that is of course up to the module owner/peers.

Last edited 4 weeks ago by sebastian (previous) (diff)
Note: See TracTickets for help on using tickets.