Issue 2377 - Implement user filter lists toggle

Issue 2377 - Implement user filter lists toggle

[saroyanm, 2016-02-26 11:31:22]

https://codereview.adblockplus.org/29337773/diff/29337774/options.js
File options.js (right):

https://codereview.adblockplus.org/29337773/diff/29337774/options.js#newcode1039
options.js:1039: filter: ["addSubscription", "error", "userSubscriptionsToggle"]
It's bit inconsistent,  but the chrome implementation itself is inconsistent, so
I thought it doesn't make sense to represent the listener on
"subscriptions.listen", because with current logic in "subscriptions.listen" we
expect changes for each subscription separately, while with the user filter
lists with current logic I'm only sending final result, so I thought it should
be separate listener from subscriptions.listen.
Ideally we should provide people ability to manage each separate user filter
list and add ability to create new user filter list, but I assume we will
implement that ability in future for Firefox.

[Thomas Greiner, 2016-03-01 12:11:22]

https://codereview.adblockplus.org/29337773/diff/29337774/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29337773/diff/29337774/messageResponder.js...
messageResponder.js:185: case "app.userSubscriptionsToggle":
Please reuse "subscriptions.toggle" for that because that's what it's meant for.
You can introduce a `downloadable: true|false` option as in "subscriptions.get"
to differentiate between toggling of downloadable and user subscriptions.

https://codereview.adblockplus.org/29337773/diff/29337774/messageResponder.js...
messageResponder.js:190: subscriptions[i].disabled = message.state;
It's a "toggle" function so it should toggle the state, not set it.

https://codereview.adblockplus.org/29337773/diff/29337774/messageResponder.js...
messageResponder.js:192:
FilterNotifier.triggerListeners("app.userSubscriptionsToggle",
We already have a filter event for that: "subscription.disabled" (see
"subscriptions.toggle" below)

https://codereview.adblockplus.org/29337773/diff/29337774/options.js
File options.js (right):

https://codereview.adblockplus.org/29337773/diff/29337774/options.js#newcode1039
options.js:1039: filter: ["addSubscription", "error", "userSubscriptionsToggle"]
On 2016/02/26 11:31:22, saroyanm wrote:
> It's bit inconsistent,  but the chrome implementation itself is inconsistent,
so
> I thought it doesn't make sense to represent the listener on
> "subscriptions.listen", because with current logic in "subscriptions.listen"
we
> expect changes for each subscription separately, while with the user filter
> lists with current logic I'm only sending final result, so I thought it should
> be separate listener from subscriptions.listen.
> Ideally we should provide people ability to manage each separate user filter
> list and add ability to create new user filter list, but I assume we will
> implement that ability in future for Firefox.

Yes, that feature will be added for Firefox. However, I'd argue that we can
already avoid inconsistencies by reusing "subscriptions.listen", and
"subscriptions.toggle".

It might be sufficient to just create a dummy subscription that represents all
user subscriptions. We could then en-/disable and listen to that subscription
without having to duplicate existing logic.

[saroyanm, 2016-03-12 15:32:03]

Thomas I made some more investigations and it appears it's bit trickier than it
sounds. 
So far I can see 3 solutions after our last discussion to this issue and each of
them have it's own problems: 

1. Disable/Enable all Special subscriptions (current solution):
In this case we will also disable the user whitelisted domains.
2. Disable/Enable all "user filters" besides "user whitelisted domains filters":
This can lead to even more inconsistency, because we never know what the state
of "Own filter list" should be in advanced tab when some of the filters are
enabled, but some not and we will not be able provide user ability to manage
that, the only way to fix that inconsistency is to have "filter.add",
"filters.disabled" listener in background page to manage the state of the
individual filter according to the user preference in the option page.
3. Introduce a dummy subscription that will include all "user filters" besides
"user whitelisted domains filters", which will need to have it's own separate
subscription again:
This is something that again needs to be done in background page, which need to
listen to  "filters.add", "filters.disabled" and "filters.removed" so it can
keep the separate list updated, in general this will add even more complexity
into current code I assume, because it will be a workaround in actual extension
code which I think we should avoid.

From my point of view this could only be solved with the update into the current
design of options page, for that I see several solutions:
1. Remove completely checkbox next to the "Own filter list" or keep it disabled,
Introduce checkbox next to each filter in  "Edit or create own filter list"
section which will provide user ability to toggle each filter individually, add
a button in the "Edit or create own filter list" which will "enable/disable all
filters" in that view. In that case we will provide user more flexibility in the
filter list management and from my point of view it's less effort than previous
3 "problematic" solutions.
2. checkbox next to the "Own filter list" will disable all Special subscription
and will Disable/Enable "user whitelisted domains filters" list from the General
tab accordingly, In that case we will also need to list all Special
subscriptions including "user whitelisted domains filters" in the "Own filter
list".

If you think both suggestions are reasonable I'll align regarding that with UX
designers, maybe this can be done with introduction of consistency in "Further
blocking options" list ?

[Thomas Greiner, 2016-03-16 12:04:29]

Ok, thanks for the detailed summary. We don't need to have any of those
capabilities in the new options page to achieve feature parity with the existing
one. Therefore the easiest solution would be to just polish the "disabled
checkbox" approach for the initial release. That includes the following tasks:

- Finalize the "disabled" version of the checkbox icon
- Don't intercept right-clicks on the checkbox

[Thomas Greiner, 2016-07-20 16:30:45]

I noticed that the ticket this review is referring to is closed so either reopen
the ticket or create a separate one for this change. However, if this change is
no longer relevant please close this review instead.

[saroyanm, 2016-07-25 13:28:10]

On 2016/07/20 16:30:45, Thomas Greiner wrote:
> I noticed that the ticket this review is referring to is closed so either
reopen
> the ticket or create a separate one for this change. However, if this change
is
> no longer relevant please close this review instead.

Thanks for reminder, I think this is no longer relevant, so I closed the review.
Currently the button is disabled and the implementation of the user filter
toggle is inconsistent with current options page design, probably we will need
to have something more complex when we implement feature parity for the Firefox.