Opened 4 years ago

Closed 4 years ago

#3839 closed change (fixed)

Adapt for Prefs event changes

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

https://codereview.adblockplus.org/29338734

Description (last modified by greiner)

Background

With #3826 Prefs.onChanged is going to be replaced by Prefs.on(pref, cb) and Prefs.off(pref, cb). So instead of having a single listener for all preference changes, you have to register one callback for each preference.

What to change

  • Adapt the code for the new options page to use Prefs.on/Prefs.off instead Prefs.onChanged.
  • There's no need for aggregating listeners anymore as with Prefs.onChanged so each "prefs.listen" message should be independent of each other.

Change History (8)

comment:1 Changed 4 years ago by sebastian

  • Description modified (diff)

comment:2 Changed 4 years ago by greiner

I wonder whether we should have one listener per preference for all the UI or one listener per UI listener. The former would require some aggregation logic (similar to how we currently have it in messageResponder.js) while the latter may result in more calls to Prefs.on()/Prefs.off().

comment:3 Changed 4 years ago by sebastian

Not that opening the options page is a hotspot, neither is having multiple options pages open a particular common use case (in fact we prevent that to some degree). So if I understand correctly, it doesn't matter much either way. Hence I'd prefer whatever results into a simpler implementation.

comment:4 Changed 4 years ago by greiner

  • Description modified (diff)
  • Ready set

comment:5 Changed 4 years ago by sebastian

  • Blocking 3826 added
  • Owner set to sebastian

comment:6 Changed 4 years ago by sebastian

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

comment:7 Changed 4 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/adblockplusui/rev/49b08709f3d2

comment:8 Changed 4 years ago by sebastian

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