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 52 matching lines @@
 
   Collection.prototype._getItemTitle = function(item, i)
   {
     if (item.url == acceptableAdsUrl)
       return getMessage("options_acceptableAds_description");
     if (this.details[i].useOriginalTitle && item.originalTitle)
       return item.originalTitle;
     return item.title || item.url || item.text;
   };
 
   Collection.prototype.addItems = function()

[Thomas Greiner, 2016/08/04 15:54:02]

Note: We should really change this function to only accept a single argument
because that's the only way we're currently using it. That should make the
content of this function simpler and avoid this unnecessary duplication that we
now introduced.

Anyway, that can be tackled in a separate ticket. Just wanted to mention it.

[saroyanm, 2016/08/17 14:59:15]

Done.

   {
-    var length = Array.prototype.push.apply(this.items, arguments);
-    if (length == 0)
+    var itemsToAdd = [];
+    for (var i = 0; i < arguments.length; i++)

[Thomas Greiner, 2016/08/04 15:54:02]

Detail: I don't mind omitting the braces for a single line but, to avoid
confusion, please use them when the content stretches across multiple lines.

[saroyanm, 2016/08/17 14:59:15]

This was fixed during previous comment implementation

+      if (this.items.indexOf(arguments[i]) == -1)
+        itemsToAdd.push(arguments[i]);
+
+    if (itemsToAdd.length == 0)
       return;
 
+    Array.prototype.push.apply(this.items, itemsToAdd);
     this.items.sort(function(a, b)
     {
       // Make sure that Acceptable Ads is always last, since it cannot be
       // disabled, but only be removed. That way it's grouped together with
       // the "Own filter list" which cannot be disabled either at the bottom
       // of the filter lists in the Advanced tab.
       if (a.url == acceptableAdsUrl)
         return 1;
       if (b.url == acceptableAdsUrl)
         return -1;
 
       var aTitle = this._getItemTitle(a, 0).toLowerCase();
       var bTitle = this._getItemTitle(b, 0).toLowerCase();
       return aTitle.localeCompare(bTitle);
     }.bind(this));
 
     for (var j = 0; j < this.details.length; j++)
     {
       var table = E(this.details[j].id);
       var template = table.querySelector("template");
-      for (var i = 0; i < arguments.length; i++)
+      for (var i = 0; i < itemsToAdd.length; i++)
       {
-        var item = arguments[i];
+        var item = itemsToAdd[i];
         var listItem = document.createElement("li");
         listItem.appendChild(document.importNode(template.content, true));
         listItem.setAttribute("aria-label", this._getItemTitle(item, j));
         listItem.setAttribute("data-access", item.url || item.text);
         listItem.setAttribute("role", "section");
 
         var label = listItem.querySelector(".display");
         if (item.recommended && label.hasAttribute("data-tooltip"))
         {
           var tooltipId = label.getAttribute("data-tooltip");
@@ skipping 216 matching lines @@
     }
   ]);
   collections.filterLists = new Collection(
   [
     {
       id: "all-filter-lists-table",
       useOriginalTitle: true
     }
   ]);
 
-  function updateLanguageCollections(subscription)
+  function toggleShowLanguage(subscription)
   {
     if (subscription.recommended == "ads")
     {
-      if (subscription.disabled)
+      if (subscription.disabled == false)

[Thomas Greiner, 2016/08/04 15:54:03]

Detail: Do we need to explicitly check for `false`? Because that's the only
thing that's changed about this function when comparing it with the base
revision.

Also note that it's a double negation which would be great to avoid to make it
easier to understand.

[saroyanm, 2016/08/17 14:59:15]

This change was required because with latest changes we were also calling this
function during recommendation initialization. The problem is that when we are
creating recommendation object we are assigning "null" to the disabled status of
the recommendation, but I can't see any reason why this needs to be done that
way, so I changed to assign disable=true during initial initialization of the
recommendations.

+      {
+        collections.allLangs.removeItem(subscription);
+        collections.langs.addItems(subscription);
+      }
+      else
       {
         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;
+    toggleShowLanguage(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(subscription);
 
-    for (var name in collections)
-      collections[name].updateItem(knownSubscription);
-
-    return knownSubscription;
+    toggleShowLanguage(subscription);
   }
 
   function updateFilter(filter)
   {
     var match = filter.text.match(/^@@\|\|([^\/:]+)\^\$document$/);
     if (match && !filtersMap[filter.text])
     {
       filter.title = match[1];
       collections.whitelist.addItems(filter);
     }
@@ skipping 617 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);
+        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);
+          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 207 matching lines @@
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
         "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
   window.addEventListener("hashchange", onHashChange, false);
 })();