Opened 4 years ago

Last modified 11 days ago

#2386 new change

Make it possible to register a notification download listener

Reported by: fhd Assignee:
Priority: P2 Milestone:
Module: Core Keywords:
Cc: 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 (19)

comment:1 Changed 4 years ago by fhd

  • Cc trev added

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

comment:2 Changed 4 years ago by fhd

  • Blocking 2387 added

comment:3 Changed 4 years ago by trev

  • Description modified (diff)
  • Ready set

comment:4 Changed 4 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 4 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 4 years ago by fhd (previous) (diff)

comment:6 Changed 4 years ago by sebastian

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

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

  • Blocking 2419 added

comment:9 Changed 4 years ago by fhd

  • Blocking 2367 removed

comment:10 Changed 4 years ago by fhd

  • Blocking 2368 removed

comment:11 Changed 4 years ago by fhd

  • Blocking 2387 removed

comment:12 Changed 4 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 4 years ago by kzar

  • Owner set to kzar

comment:14 Changed 4 years ago by kzar

  • Owner kzar deleted

comment:15 Changed 4 years ago by fhd

  • Blocking 2419 removed

comment:16 Changed 18 months 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 18 months ago by sebastian (previous) (diff)

comment:17 Changed 15 months ago by fhd

  • Cc trev removed

comment:18 Changed 5 weeks ago by tombaker

Add API for registering a notification download listener.
run 3

comment:19 Changed 11 days ago by rahulkl

It seems you must add an API for that to sort out the issue, same thing happened to me too in the past and yes, it was not resolved.
https://morphtv.org/

Note: See TracTickets for help on using tickets.