Opened on 04/22/2015 at 09:25:09 AM

Closed on 08/29/2019 at 05:43:52 PM

Last modified on 10/08/2019 at 05:31:07 PM

#2386 closed change (rejected)

Make it possible to register a notification download listener

Reported by: fhd Assignee:
Priority: P2 Milestone:
Module: Core Keywords: closed-in-favor-of-gitlab
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.

Attachments (0)

Change History (24)

comment:1 Changed on 04/22/2015 at 09:26:31 AM by fhd

  • Cc trev added

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

comment:2 Changed on 04/22/2015 at 09:29:10 AM by fhd

  • Blocking 2387 added

comment:3 Changed on 04/22/2015 at 11:24:44 AM by trev

  • Description modified (diff)
  • Ready set

comment:4 Changed on 04/22/2015 at 11:31:31 AM 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 on 04/22/2015 at 11:46:41 AM 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 on 04/22/2015 at 11:47:42 AM by fhd

comment:6 Changed on 04/22/2015 at 11:52:31 AM by sebastian

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

comment:7 Changed on 04/22/2015 at 12:12:56 PM 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 on 04/26/2015 at 09:36:27 AM by fhd

  • Blocking 2419 added

comment:9 Changed on 04/26/2015 at 09:45:13 AM by fhd

  • Blocking 2367 removed

comment:10 Changed on 04/26/2015 at 09:48:20 AM by fhd

  • Blocking 2368 removed

comment:11 Changed on 04/26/2015 at 09:57:23 AM by fhd

  • Blocking 2387 removed

comment:12 Changed on 04/26/2015 at 10:10:11 AM 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 on 05/13/2015 at 12:18:16 PM by kzar

  • Owner set to kzar

comment:14 Changed on 05/13/2015 at 12:18:59 PM by kzar

  • Owner kzar deleted

comment:15 Changed on 06/09/2015 at 11:14:29 AM by fhd

  • Blocking 2419 removed

comment:16 Changed on 09/23/2017 at 02:24:34 AM 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 on 09/23/2017 at 02:28:14 AM by sebastian

comment:17 Changed on 12/21/2017 at 11:25:42 AM by fhd

  • Cc trev removed

comment:18 Changed on 05/18/2019 at 02:45:09 AM by alicetaylor

spam

Last edited on 10/08/2019 at 05:30:55 PM by kzar

comment:19 Changed on 06/27/2019 at 04:39:55 AM by phamyen

spam

Last edited on 10/08/2019 at 05:30:59 PM by kzar

comment:20 Changed on 07/01/2019 at 09:55:57 AM by redball12345671211

spam

Last edited on 10/08/2019 at 05:31:02 PM by kzar

comment:21 Changed on 08/19/2019 at 09:39:24 AM by ducklife

spam

Last edited on 10/08/2019 at 05:31:04 PM by kzar

comment:22 Changed on 08/23/2019 at 06:13:13 AM by hamletjohn

hg

comment:23 Changed on 08/29/2019 at 05:43:52 PM by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from new to closed

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the ​new issue tracker.

comment:24 Changed on 09/09/2019 at 06:27:47 AM by theLYRICALLY-Lyrics

spam

Last edited on 10/08/2019 at 05:31:07 PM by kzar

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none).
Attachments ↑ Description ↑
 
Note: See TracTickets for help on using tickets.