Opened on 07/27/2015 at 04:40:49 PM
Closed on 10/03/2017 at 02:45:24 PM
Last modified on 11/03/2017 at 01:41:08 PM
#2824 closed change (fixed)
Ignore non-ads subscriptions in Utils.chooseFilterSubscription
| Reported by: | greiner | Assignee: | wspee |
|---|---|---|---|
| Priority: | P2 | Milestone: | Adblock-Plus-3.0-for-Firefox |
| Module: | Platform | Keywords: | |
| Cc: | trev, saroyanm, arthur, sebastian | Blocked By: | #2822 |
| Blocking: | #2821 | Platform: | Unknown / Cross platform |
| Ready: | yes | Confidential: | no |
| Tester: | Ross | Verified working: | yes |
| Review URL(s): | |||
Description (last modified by sebastian)
Background
See #2821. Since we will add subscriptions to subscriptions.xml which are not of type "ads", to include those in the new options page, we need to make sure that the default subscription is always of type "ads" and of no other.
What to change
Move the logic from Utils.chooseFilterSubscription into the subscriptionInit module, and add a check to to make sure that it only considers subscriptions of type "ads".
Open question
In Firefox, the subscription drop-down in the filter preferences will also include recommended subscriptions for the newly added types. We could tackle it either by (1) leaving it like that, (2) grouping subscriptions within that drop-down list or (3) only showing subscriptions of type "ads" in that list.
Attachments (0)
Change History (16)
comment:1 Changed on 01/20/2016 at 10:40:26 AM by greiner
- Cc trev added
comment:2 Changed on 01/24/2017 at 10:09:31 AM by saroyanm
@Wladimir is there any information missing in this ticket besides of an open question ? Please let me know if there is something me or @Thomas still need to provide or update to make this ticket ready.
This is blocking #2821 so would be great to tackle this and other issues that do not directly affect new options page layout while we are waiting for the final Style guides from @Jeen.
Note: Me and @Thomas aligned yesterday and think that this ticket is still actual even was created a long while ago.
comment:3 Changed on 01/24/2017 at 11:58:59 AM by saroyanm
- Cc saroyanm added
comment:4 Changed on 04/21/2017 at 01:04:52 PM by trev
- Component changed from Core to Adblock-Plus-for-Firefox
- Ready set
This is an issue specific to Firefox code, so I am putting it into the right module - ready otherwise.
comment:5 Changed on 04/28/2017 at 02:55:04 PM by arthur
- Cc arthur added
comment:6 follow-up: ↓ 8 Changed on 09/28/2017 at 01:27:55 AM by sebastian
What do we do about this issue? We didn't plan to have another release for the legacy extension, but does that mean that this will cause problems for users on the legacy extension as soon as we start using the new options page an therefore add non-ad subscriptions to recommendations.xml?
comment:7 Changed on 09/28/2017 at 01:28:07 AM by sebastian
- Cc sebastian added
comment:8 in reply to: ↑ 6 Changed on 09/28/2017 at 01:52:46 PM by greiner
Replying to sebastian:
does that mean that this will cause problems for users on the legacy extension as soon as we start using the new options page an therefore add non-ad subscriptions to recommendations.xml?
As long as we don't release a new legacy extension version which includes the updated subscriptions.xml (aka recommendations.xml) the introduction of subscription types should have no effect on which filter list the extension chooses during setup.
What I mentioned under "Open questions" still applies though.
comment:9 Changed on 09/28/2017 at 05:27:35 PM by sebastian
Makes sense. For some reason, I mistakenly thought, this file is downloaded from the web. So I guess lets, just remove the dependency on #2821 and close #2821, then.
In the likely case we won't have another update for the legacy extension, this issue will be closed together with all remaining issues for this module, once the WebExt version has been released.
comment:10 Changed on 09/29/2017 at 07:12:00 AM by trev
- Component changed from Adblock-Plus-for-Firefox to Platform
- Ready unset
Actually, this is really a Platform issue now - there is an implementation of Utils.chooseFilterSubscription in adblockpluschrome repository that has the same issue.
comment:11 Changed on 09/29/2017 at 04:15:30 PM by sebastian
- Description modified (diff)
- Priority changed from Unknown to P2
- Ready set
comment:12 Changed on 10/02/2017 at 09:47:02 AM by wspee
- Owner set to wspee
comment:13 Changed on 10/02/2017 at 10:19:19 AM by wspee
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:14 Changed on 10/03/2017 at 02:44:25 PM by abpbot
A commit referencing this issue has landed:
Issue 2824 - Only consider subscriptions of type ads in chooseFilterSubscription
comment:15 Changed on 10/03/2017 at 02:45:24 PM by sebastian
- Milestone set to Adblock-Plus-for-Chrome-Opera-next
- Resolution set to fixed
- Status changed from reviewing to closed
comment:16 Changed on 11/03/2017 at 01:41:08 PM by Ross
- Tester changed from Unknown to Ross
- Verified working set
This change seems fine. I look to have the correct lists on install with both the old options page (Chrome) and the new one (Firefox).
ABP 1.13.4.1903
Chrome 49 / 62 / Windows 10
Opera 36 / 48 / Windows 10
ABP 2.99.0.1902beta
Firefox 50 / 57 / Windows 10
@trev What's your take on the open question in this ticket? It's currently unclear whether the drop-down list in the filter settings dialog is supposed to show all recommended subscriptions or only those for blocking ads.
Also, could you please check the rest of the ticket as well so that we can get it "ready"?