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 414 matching lines @@
     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);
+        updateSubscription(subscription);
         updateLanguageCollections(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)

[Thomas Greiner, 2016/06/16 16:10:34]

This will always be `true`. Compare it with the base revision and you'll find
that this check worked before.

[saroyanm, 2016/06/16 16:51:44]

Well spotted, thanks, Done.

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

[Thomas Greiner, 2016/06/16 16:10:34]

I noticed that those two function calls swapped places. How is that related to
the issue?

[saroyanm, 2016/06/16 16:51:44]

Logic here is bit confusing and yes swiping the function calls doesn't make any
easier or in this case sense.
I've update the implementation, so currently I'm checking if the Subscription is
of type=="ads" only then I'm calling the "updateLanguageCollections" method,
which we were calling for most of the subscriptions which are not type of "ads"
which making the logic confusing.

Let me know if current implementation makes more sense from your point of view
and if we can make keep this fix in this review as well, to make the code more
understandable.

         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);
 })();