Opened 2 years ago

Closed 2 years ago

#5835 closed change (fixed)

Add "notifications.get" message handler for the popup

Reported by: kzar Assignee: kzar
Priority: P2 Milestone:
Module: User-Interface Keywords:
Cc: greiner, sebastian, mjethani Blocked By:
Blocking: #5593 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29567743/

Description (last modified by kzar)

Background

With #5593 we're replacing direct use of the background page from the popup window with messaging. We need to do this for the notification handling code too, but the required message handler has not been added to adblockplusui/messageResponder.js yet.

What to change

Add a "notifications.get" message handler to adblockplusui/messageResponders.js.

It should return an Array of active notifications, or an empty Array if there aren't any. The notifications can be fetched using getActiveNotifications() in adblockpluschrome/lib/notificationHelper.js. For each notification in the Array the texts property should be populated with the result of getLocalizedTexts() from adblockpluscore/lib/notifications.js.

The message handler should optionally take a displayMethod field, for which if specified restricts the returned notifications to the given display method e.g. "popup". The handler can make use of shouldDisplay() in adblockpluschrome/lib/notificationHelper.js to filter the notifications by the given display method.

Change History (10)

comment:1 Changed 2 years ago by kzar

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

comment:2 Changed 2 years ago by greiner

I'd recommend choosing "notifications.get" and passing {active: true} instead of "notifications.getActive" to keep it consistent with the existing naming scheme.

We could even include the "shouldDisplay" information in there, for instance, by returning its associated display methods. Same applies to the localized texts because based on what I see how it's used this should be sufficient.

Example:

ext.backgroundPage.sendMessage({
  type: "notifications.get",
  active: true
}, ([notification]) =>
{
  if (!notification)
    return;

  let [{data, displayMethods, texts}] = notifications;
  ...
})

comment:3 follow-up: Changed 2 years ago by kzar

  • Description modified (diff)
  • Summary changed from Add notification message handlers for popup to Add "notifications.get" message handler for the popup

Sounds sensible to me, except I'm not sure how we could return non-active notifications. I've updated the description does it look OK to you now?

comment:4 Changed 2 years ago by mjethani

notifications.get could also take a shouldDisplay argument.

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

comment:5 in reply to: ↑ 3 Changed 2 years ago by greiner

Replying to kzar:

Sounds sensible to me, except I'm not sure how we could return non-active notifications. I've updated the description does it look OK to you now?

If there are no inactive notifications then only returning the active ones sounds good to me. I was under the impression that some of them might be inactive.

Replying to mjethani:

notifications.get could also take a shouldDisplay argument.

That'd already be covered by the "displayMethods" in the response but I guess we could instead add this to the query as

{
  type: "notifications.get",
  displayMethod: "popup"
}

to retrieve only the notifications that we are interested in. In that case we wouldn't need to pass all display methods to the UI anymore.

@kzar What do you think about this approach?

comment:6 Changed 2 years ago by kzar

  • Description modified (diff)

Cool idea, I've updated the description again.

comment:7 Changed 2 years ago by kzar

  • Description modified (diff)

comment:8 Changed 2 years ago by greiner

  • Priority changed from Unknown to P2
  • Ready set

comment:9 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 5835 - Add notifications.get message handler

comment:10 Changed 2 years ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.