Opened 3 years ago

Closed 12 months ago

Last modified 11 months 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-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 3 years 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 20 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 20 months ago by saroyanm

  • Cc saroyanm added

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

  • Cc arthur added

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

  • Cc sebastian added

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

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

comment:12 Changed 12 months ago by wspee

  • Owner set to wspee

comment:13 Changed 12 months ago by wspee

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

comment:15 Changed 12 months 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 11 months 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.