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)
   {
+    var knownSubscription = subscriptionsMap[subscription.url];

[Thomas Greiner, 2016/06/16 11:09:33]

I'd prefer not having to deal with two different versions of the same
subscription so I'd propose something along this line:

if (subscription.url in subscriptionsMap)
{
  subscription = subscriptionsMap[subscription.url];
  ...
}

With that change the rest of the listener should be a bit cleaner since you no
longer have to think about which subscription object you're supposed to use.

[saroyanm, 2016/06/16 13:35:48]

Very good point!

+    if (knownSubscription)
+    {
+      for (var property in subscription)
+      {
+        if (property == "title" && knownSubscription.recommended)
+          knownSubscription.originalTitle = subscription.title;
+        else
+          knownSubscription[property] = subscription[property];
+      }
+    }
+    
     switch (action)
     {
       case "disabled":
-        subscription = updateSubscription(subscription);
-        updateLanguageCollections(subscription);
+        updateSubscription(knownSubscription);
+        updateLanguageCollections(knownSubscription);
         break;
       case "downloading":
       case "downloadStatus":
       case "homepage":
       case "lastDownload":
       case "title":
-        updateSubscription(subscription);
+        updateSubscription(knownSubscription);
         break;
       case "added":
-        if (subscription.url in subscriptionsMap)
-          subscription = updateSubscription(subscription);
+        if (knownSubscription)
+          updateLanguageCollections(knownSubscription);
         else
           addSubscription(subscription);
 
-        collections.filterLists.addItems(subscription);
-        updateLanguageCollections(subscription);
+        subscription = updateSubscription(knownSubscription || subscription);
+        collections.filterLists.addItems(knownSubscription || subscription);
         break;
       case "removed":
-        var knownSubscription = subscriptionsMap[subscription.url];
-
-        if (subscription.url == acceptableAdsUrl || subscription.recommended)
+        if (subscription.url == acceptableAdsUrl || knownSubscription.recommended)
         {
           subscription.disabled = true;
           onSubscriptionMessage("disabled", subscription);
         }
         else
         {
           collections.custom.removeItem(knownSubscription);
           delete subscriptionsMap[subscription.url];
         }
         collections.filterLists.removeItem(knownSubscription);
@@ skipping 230 matching lines @@
   });
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
         "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 })();