#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 23 months ago by kzar

  • Description modified (diff)

comment:2 Changed 23 months ago by kzar

  • Description modified (diff)

comment:3 Changed 23 months ago by kzar

  • Description modified (diff)

comment:4 Changed 23 months 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 23 months 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 23 months ago by kzar

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

comment:7 in reply to: ↑ 5 Changed 23 months 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 23 months 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 23 months ago by mjethani

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

comment:10 Changed 23 months 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 23 months 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 23 months 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 23 months ago by abpbot

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

comment:14 in reply to: ↑ 12 Changed 23 months 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.