Issue 4156 - Adblocking filter only being removed in advanced tab fix

new-options.js

Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -393,18 +393,15 @@
 
   function updateLanguageCollections(subscription)
   {
-    if (subscription.recommended == "ads")

[Thomas Greiner, 2016/06/16 17:08:38]

Why did you move this check out of here? This function only makes sense for
"ad"-type subscriptions.

[saroyanm, 2016/06/16 17:42:11]

For code readability, we were calling this function for every subscription with
prev. implementation and it were not making sense IMO, the name of the function
make it obvious that the method is updating LanguageCollection, so it should
expect subscription to be of "ads" type, but let me know if you think that this
is not making code more readable.

[Thomas Greiner, 2016/06/17 13:48:03]

Ok, what about getting rid of this function then altogether? There's not much in
it anyway and it's used only in `onSubscriptionMessage()` - same for
`updateSubscription()`.

One way to achieve this would be as follows:

switch (action)
{
  case "added":
  case "disabled":
  case "downloading":
  ...
  case "title":
    if (subscription.url in subscriptionMap)
      // merge updateSubscription() here
    else
      addSubscription(subscription);

    if (subscription.recommended == "ads")
      // merge updateLanguageCollections() here
    break;
  case "removed":
    ...
}

Alternatively, we could stop using the switch-statement and do something more
flexible like:

if (action == "removed")
{
  if (subscription.url == acceptableAdsUrl || subscription.recommended)
  {
    action = "disabled";
    subscription.disabled = true;
  }
  else
    ...
  ...
}

if (action != "removed")
{
  if (action == "added")
    addSubscription(subscription);
  else
    // merge updateSubscription() here

  if (subscription.recommended == "ads")
    // merge updateLanguageCollections() here
}

if (action == "added" || action == "disabled" || action == "removed")
  updateShareLinks();

[saroyanm, 2016/06/21 12:53:07]

I think it's bit trickier than we thought,I've merged
"updateLanguageCollections", but with "updateSubscription" as you suggested is
bit trickier, because we can't merge action "disabled" and "added" without
checking for that actions as well, because imagine the scenario when the title,
or "downloadingStatus" will be changed in that case we will again remove and add
items to "languages" related tables, that why we only need to add/delete items
from that tables only if the item is being added/deleted/changed the "disabled"
property.
I've uploaded a new patch, so please let me know if changes in the patch are
making the code more readable. If you think having separate
"updateLanguageCollections" is better I'll revert the changes and keep it as it
was initially.

[Thomas Greiner, 2016/06/22 10:08:04]

Ok, I guess I assumed that `addItems()` would only add the subscription if it's
not already in there but apparently it doesn't care about duplicates.

Maybe we should fix that at some point but for now we can revert it to leave it
as it is.

[saroyanm, 2016/06/22 12:31:59]

I agree, but if we only fix that we will fix simptome of the current
duplication, when the item is being added, so first I'll suggest find reason of
why the duplication taking place in first place in the corresponding ticket, but
I wonder do we expect the subscription to be added twice ? If not I think we
should worry about that.

+    if (subscription.disabled)
     {
-      if (subscription.disabled)
-      {
-        collections.allLangs.addItems(subscription);
-        collections.langs.removeItem(subscription);
-      }
-      else
-      {
-        collections.allLangs.removeItem(subscription);
-        collections.langs.addItems(subscription);
-      }
+      collections.allLangs.addItems(subscription);
+      collections.langs.removeItem(subscription);
+    }
+    else
+    {
+      collections.allLangs.removeItem(subscription);
+      collections.langs.addItems(subscription);
     }
   }
 
@@ -432,19 +429,8 @@
 
   function updateSubscription(subscription)
   {
-    var knownSubscription = subscriptionsMap[subscription.url];
-    for (var property in subscription)
-    {
-      if (property == "title" && subscription.recommended)
-        knownSubscription.originalTitle = subscription.title;
-      else
-        knownSubscription[property] = subscription[property];
-    }
-
     for (var name in collections)
-      collections[name].updateItem(knownSubscription);
-
-    return knownSubscription;
+      collections[name].updateItem(subscription);
   }
 
   function updateFilter(filter)
@@ -929,11 +915,24 @@
 
   function onSubscriptionMessage(action, subscription)
   {
+    if (subscription.url in subscriptionsMap)
+    {
+      var knownSubscription = subscriptionsMap[subscription.url];
+      for (var property in subscription)
+      {
+        if (property == "title" && knownSubscription.recommended)
+          knownSubscription.originalTitle = subscription.title;
+        else
+          knownSubscription[property] = subscription[property];
+      }
+      subscription = knownSubscription;
+    }
     switch (action)
     {
       case "disabled":
-        subscription = updateSubscription(subscription);
-        updateLanguageCollections(subscription);
+        if (subscription.recommended == "ads")
+          updateLanguageCollections(subscription);
+        updateSubscription(subscription);
         break;
       case "downloading":
       case "downloadStatus":
@@ -943,17 +942,16 @@
         updateSubscription(subscription);
         break;
       case "added":
-        if (subscription.url in subscriptionsMap)
-          subscription = updateSubscription(subscription);
+        if (subscription.recommended == "ads")
+          updateLanguageCollections(subscription);
+        else if (subscription.url in subscriptionsMap)

[Thomas Greiner, 2016/06/16 17:08:38]

This else-if means that ad blocking subscriptions will only be updated in the
language collections and therefore not in other ones such as the one in the
Advanced tab… unless I'm misinterpreting this.

Anyway, why not just call `updateLanguageCollections()` inside
`updateSubscription()` and `addSubscription()`?

[saroyanm, 2016/06/16 17:42:11]

Yes the Advanced tab will be updated by the call below:
collections.filterLists.addItems(subscription);
I see two reasons:
1. We call updateSubscription during "title", "lastDownload", "homepage",
"downloadStatus" and "downloading" change events and this will trigger the call
for the function which in the end will remoe or add the language.
2. I think as in the comment above, this is bit more readable/maintainable that
we have different methods called for different subscriptions.

[Thomas Greiner, 2016/06/23 14:54:29]

Why isn't it updated by `addSubscription()` and `updateSubscription()`? Seems
like those are the ones responsible for adding and updating subscriptions in
collections.

[saroyanm, 2016/06/23 16:27:12]

We are also adding recommendations using "addSubscription", the recommendations
are only being shown in the General tab initially.
With updateSubscription is bit different, it's updating the subscription, as
well it's updates the list in advanced tab, but it's not adding items, so this
was created I assume to remove code duplications from the
"onSubscriptionMessage".
So we have several solutions IMO:
1. Add second "action" parameter to "updateLanguageCollections",
"updateSubscription" and "addSubscription", check for "added" action that has
been passed to the "onSubscriptionMessage" function.
2. Move "updateLanguageCollections", "updateSubscription" and "addSubscription"
functions back to the "onSubscriptionMessage" provide code duplications.
3. Just rename "addSubscription" functions to "addGeneralTabSubscription", so we
will have 3 functions:
"updateLanguageCollections" - which obviously updating 2 collections in General
tab, because we do not have other Language collections
"updateSubscription" - this function is updating, but not adding item
"addGeneralTabSubscription" - self explanatory.

What you think ?

[Thomas Greiner, 2016/07/13 11:35:18]

Ok, so the current behavior is:
- addSubscription: adds subscription
- updateSubscription: updates subscription
- updateLanguageCollections: adds and updates language collections

So what I'd go for is the following:
- addSubscription: adds subscription + calls `toggleShowLanguage(subscription,
true)`
- updateSubscription: updates subscription + calls
`toggleShowLanguage(subscription)`

For that you just need to change `updateLanguageCollections(Subscription
subscription)` to `toggleShowLanguage(Subscription subscription, boolean force)`
like in `Element.classList.toggle()` and `Notification.toggleIgnoreCategory()`
(see
https://hg.adblockplus.org/adblockpluscore/file/tip/lib/notification.js#l391).

Thereby you could avoid code duplication, ensure consistency and clarify what
the code does.

[saroyanm, 2016/08/01 18:24:18]

1. I've moved the call for updateLanguageCollection to "addSubscription" and
"updateSubscription" functions, 
2. reverted old check for the ads type in "updateLanguageCollection"
3. Renamed updateLanguageCollections to toggleShowLanguage
4. restricted duplications in the collection's "add" method, to avoid
duplications caused by multiple calls of "toggleShowLanguage" method

+          updateSubscription(subscription);
         else
           addSubscription(subscription);
-
+        

[Thomas Greiner, 2016/06/16 17:08:38]

Coding style: Trailing white space.

[saroyanm, 2016/06/22 12:31:59]

Done.

         collections.filterLists.addItems(subscription);
-        updateLanguageCollections(subscription);
         break;
       case "removed":
-        var knownSubscription = subscriptionsMap[subscription.url];
-
         if (subscription.url == acceptableAdsUrl || subscription.recommended)
         {
           subscription.disabled = true;
@@ -961,10 +959,10 @@
         }
         else
         {
-          collections.custom.removeItem(knownSubscription);
+          collections.custom.removeItem(subscription);
           delete subscriptionsMap[subscription.url];
         }
-        collections.filterLists.removeItem(knownSubscription);
+        collections.filterLists.removeItem(subscription);
         break;
     }