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

new-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 373 matching lines @@
   ]);
   collections.filterLists = new Collection(
   [
     {
       id: "all-filter-lists-table",
       onClick: toggleDisableSubscription,
       useOriginalTitle: true
     }
   ]);
 
-  function updateLanguageCollections(subscription)
-  {
-    if (subscription.recommended == "ads")
-    {
-      if (subscription.disabled)
-      {
-        collections.allLangs.addItems(subscription);
-        collections.langs.removeItem(subscription);
-      }
-      else
-      {
-        collections.allLangs.removeItem(subscription);
-        collections.langs.addItems(subscription);
-      }
-    }
-  }
-
   function addSubscription(subscription)
   {
     var collection;
     if (subscription.recommended)
     {
       if (subscription.recommended != "ads")
         collection = collections.popular;
       else if (subscription.disabled == false)
         collection = collections.langs;
       else
         collection = collections.allLangs;
     }
     else if (subscription.url == acceptableAdsUrl)
       collection = collections.acceptableAds;
     else
       collection = collections.custom;
 
     collection.addItems(subscription);
     subscriptionsMap[subscription.url] = subscription;
     updateTooltips();
   }
 
   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)
   {
     var match = filter.text.match(/^@@\|\|([^\/:]+)\^\$document$/);
     if (match && !filtersMap[filter.text])
     {
       filter.title = match[1];
       collections.whitelist.addItems(filter);
     }
@@ skipping 464 matching lines @@
         collections.whitelist.removeItem(knownFilter);
         collections.customFilters.removeItem(knownFilter);
         delete filtersMap[filter.text];
         updateShareLink();
         break;
     }
   }
 
   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")
+        {
+          if (subscription.disabled == true)
+          {
+            collections.allLangs.addItems(subscription);
+            collections.langs.removeItem(subscription);
+          }
+          else
+          {
+            collections.allLangs.removeItem(subscription);
+            collections.langs.addItems(subscription);
+          }
+        }
+        updateSubscription(subscription);
         break;
       case "downloading":
       case "downloadStatus":
       case "homepage":
       case "lastDownload":
       case "title":
         updateSubscription(subscription);
         break;
       case "added":
-        if (subscription.url in subscriptionsMap)
-          subscription = updateSubscription(subscription);
+        if (subscription.recommended == "ads")
+        {
+          collections.allLangs.removeItem(subscription);

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

The subscription might be disabled when it's added so don't assume that it
isn't. Reverting the merge of `updateLanguageCollections()` should fix that
regression though.

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

Well spotted, done

+          collections.langs.addItems(subscription);
+        }
+        else if (subscription.url in subscriptionsMap)
+          updateSubscription(subscription);
         else
           addSubscription(subscription);
-
+        
         collections.filterLists.addItems(subscription);
-        updateLanguageCollections(subscription);
         break;
       case "removed":
-        var knownSubscription = subscriptionsMap[subscription.url];
-
         if (subscription.url == acceptableAdsUrl || subscription.recommended)
         {
           subscription.disabled = true;
           onSubscriptionMessage("disabled", subscription);
         }
         else
         {
-          collections.custom.removeItem(knownSubscription);
+          collections.custom.removeItem(subscription);
           delete subscriptionsMap[subscription.url];
         }
-        collections.filterLists.removeItem(knownSubscription);
+        collections.filterLists.removeItem(subscription);
         break;
     }
 
     updateShareLink();
   }
 
   function hidePref(key, value)
   {
     var element = document.querySelector("[data-pref='" + key + "']");
     if (element)
@@ skipping 220 matching lines @@
   });
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
         "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 })();