Opened 2 years ago

Closed 7 weeks ago

Last modified 3 weeks ago

#2824 closed change (fixed)

Ignore non-ads subscriptions in Utils.chooseFilterSubscription

Reported by: greiner Assignee: wspee
Priority: P2 Milestone: Adblock-Plus-3.0-for-Chrome-Opera-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):

https://codereview.adblockplus.org/29562595/

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.

Change History (16)

comment:1 Changed 22 months ago by greiner

  • Cc trev added

@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"?

comment:2 Changed 10 months ago 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 10 months ago by saroyanm

  • Cc saroyanm added

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

  • Cc arthur added

comment:6 follow-up: Changed 2 months ago 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 2 months ago by sebastian

  • Cc sebastian added

comment:8 in reply to: ↑ 6 Changed 8 weeks ago 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 8 weeks ago 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 8 weeks ago 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 8 weeks ago by sebastian

  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set

comment:12 Changed 8 weeks ago by wspee

  • Owner set to wspee

comment:13 Changed 8 weeks ago by wspee

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

comment:15 Changed 7 weeks ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:16 Changed 3 weeks ago 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

Note: See TracTickets for help on using tickets.