Opened 2 years ago

Closed 2 years ago

#5847 closed change (fixed)

Add prefs.set message handler

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

https://codereview.adblockplus.org/29570584/

Description (last modified by kzar)

Background

For #5593 we're switching to using messaging for the popup window, instead of accessing the background context directly. We can call toggleIgnoreCategory() in adblockpluscore/lib/notification.js by using the prefs.toggle message, but unfortunately that doesn't allow us to pass through the optional forceValue parameter.

It was assumed that toggling notifications to be ignored / unignored was sufficient, since if a notification is displayed we know that notifications aren't being ignored and therefore toggling would always result in them being ignored. Unfortunately though, as Manish points out, this is not true since relentless notifications are displayed even if notifications are set to be ignored. (For more context see this discussion.)

What to change

Add a prefs.set listener in messageResponders.js, which acts similarly to prefs.toggle but passes through the value parameter through to toggleIgnoreCategory if the message key is notifications_ignoredcategories and simply assigns the value to Prefs[message.key] otherwise.

Change History (14)

comment:1 Changed 2 years ago by kzar

  • Description modified (diff)

comment:2 Changed 2 years ago by kzar

  • Description modified (diff)

comment:3 Changed 2 years ago by kzar

  • Description modified (diff)

comment:4 Changed 2 years ago by greiner

While at it honour the forceValue option for other preferences too.

Which preferences are you referring to with that?

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

Well for all other message keys the handler does Prefs[message.key] = !Prefs[message.key];, so I figure we might as well honour the forceValue parameter for those too, so the interface is consistent. But I don't mind, if you prefer I can remove that logic too.

comment:6 Changed 2 years ago by kzar

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

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

  • Priority changed from Unknown to P2
  • Ready set

Replying to kzar:

Well for all other message keys the handler does Prefs[message.key] = !Prefs[message.key];, so I figure we might as well honour the forceValue parameter for those too, so the interface is consistent. But I don't mind, if you prefer I can remove that logic too.

Ok, makes sense to extend the logic for all preferences across the board with this parameter.

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

  • Description modified (diff)
  • Summary changed from Pass forceValue through from prefs.toggle message to notification.toggleIgnoreCategory to Add prefs.set message handler

What do you think Thomas? I've updated the issue with Manish's idea of instead having a new message called prefs.set.

comment:9 Changed 2 years ago by mjethani

If it's called prefs.set, then I would give it key and value parameters.

comment:10 Changed 2 years ago by kzar

  • Description modified (diff)

Yea I already did that, just forgot to change the name in the description.

comment:11 in reply to: ↑ 8 Changed 2 years ago by greiner

Replying to kzar:

What do you think Thomas? I've updated the issue with Manish's idea of instead having a new message called prefs.set.

Should be fine as long as we're not introducing any unreasonable amount of duplication.

comment:12 follow-up: Changed 2 years ago by sebastian

How about using similar semantics as DOMTokenList.toggle(), i.e. adding an optional argument to prefs.toggle that sets the value, so that we don't need to introduce yet another message type?

comment:13 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 5847 - Add prefs.set message handler

comment:14 in reply to: ↑ 12 Changed 2 years ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed

Replying to sebastian:

How about using similar semantics as DOMTokenList.toggle(), i.e. adding an optional argument to prefs.toggle that sets the value, so that we don't need to introduce yet another message type?

Well I did it that way originally but Manish preferred the message to be separate. I didn't mind too much either way, but since this is reviewed and pushed now let's just go with it.

Note: See TracTickets for help on using tickets.