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 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)
+  Collection.prototype.addItem = function(item)
+  {
+    if (this.items.indexOf(item) >= 0)
       return;
 
-    Array.prototype.push.apply(this.items, itemsToAdd);
+    this.items.push(item);
     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 < itemsToAdd.length; 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");
-          tooltipId = tooltipId.replace("%value%", item.recommended);
-          label.setAttribute("data-tooltip", tooltipId);
+      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");
+        tooltipId = tooltipId.replace("%value%", item.recommended);
+        label.setAttribute("data-tooltip", tooltipId);
+      }
+
+      var controls = listItem.querySelectorAll(".control");
+      for (var k = 0; k < controls.length; k++)
+      {
+        if (controls[k].hasAttribute("title"))
+        {
+          var titleValue = getMessage(controls[k].getAttribute("title"));
+          controls[k].setAttribute("title", titleValue)
         }
-
-        var controls = listItem.querySelectorAll(".control");
-        for (var k = 0; k < controls.length; k++)
-        {
-          if (controls[k].hasAttribute("title"))
-          {
-            var titleValue = getMessage(controls[k].getAttribute("title"));
-            controls[k].setAttribute("title", titleValue)
-          }
-        }
-
-        this._setEmpty(table, null);
-        if (table.hasChildNodes())
-        {
-          table.insertBefore(listItem,
-              table.childNodes[this.items.indexOf(item)]);
-        }
-        else
-          table.appendChild(listItem);
-        this.updateItem(item);
-      }
+      }
+
+      this._setEmpty(table, null);
+      if (table.hasChildNodes())
+      {
+        table.insertBefore(listItem,
+            table.childNodes[this.items.indexOf(item)]);
+      }
+      else
+        table.appendChild(listItem);
+      this.updateItem(item);
     }
     return length;
   };
 
   Collection.prototype.removeItem = function(item)
   {
     var index = this.items.indexOf(item);
     if (index == -1)
       return;
 
@@ skipping 186 matching lines @@
     {
       id: "all-filter-lists-table",
       useOriginalTitle: true
     }
   ]);
 
   function toggleShowLanguage(subscription)
   {
     if (subscription.recommended == "ads")
     {
-      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.

+      if (subscription.disabled)
+      {
+        collections.allLangs.addItem(subscription);
+        collections.langs.removeItem(subscription);
+      }
+      else
       {
         collections.allLangs.removeItem(subscription);
-        collections.langs.addItems(subscription);
-      }
-      else
-      {
-        collections.allLangs.addItems(subscription);
-        collections.langs.removeItem(subscription);
+        collections.langs.addItem(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);
+    collection.addItem(subscription);
     subscriptionsMap[subscription.url] = subscription;
     toggleShowLanguage(subscription);
     updateTooltips();
   }
 
   function updateSubscription(subscription)
   {
     for (var name in collections)
       collections[name].updateItem(subscription);
 
     toggleShowLanguage(subscription);
   }
 
   function updateFilter(filter)
   {
     var match = filter.text.match(/^@@\|\|([^\/:]+)\^\$document$/);
     if (match && !filtersMap[filter.text])
     {
       filter.title = match[1];
-      collections.whitelist.addItems(filter);
+      collections.whitelist.addItem(filter);
     }
     else
-      collections.customFilters.addItems(filter);
+      collections.customFilters.addItem(filter);
 
     filtersMap[filter.text] = filter;
   }
 
   function loadRecommendations()
   {
     fetch("subscriptions.xml")
       .then(function(response)
       {
         return response.text();
       })
       .then(function(text)
       {
         var list = document.getElementById("subscriptionSelector");
         var doc = new DOMParser().parseFromString(text, "application/xml");
         var elements = doc.documentElement.getElementsByTagName("subscription");
         for (var i = 0; i < elements.length; i++)
         {
           var element = elements[i];
           var type = element.getAttribute("type");
           var subscription = {
-            disabled: null,
+            disabled: true,
             downloadStatus: null,
             homepage: null,
             originalTitle: element.getAttribute("title"),
             recommended: type,
             url: element.getAttribute("url")
           };
 
           var prefix = element.getAttribute("prefixes");
           if (prefix)
           {
@@ skipping 613 matching lines @@
       case "lastDownload":
       case "title":
         updateSubscription(subscription);
         break;
       case "added":
         if (subscription.url in subscriptionsMap)
           updateSubscription(subscription);
         else
           addSubscription(subscription);
 
-        collections.filterLists.addItems(subscription);
+        collections.filterLists.addItem(subscription);
         break;
       case "removed":
         if (subscription.url == acceptableAdsUrl || subscription.recommended)
         {
           subscription.disabled = true;
           onSubscriptionMessage("disabled", subscription);
         }
         else
         {
           collections.custom.removeItem(subscription);
@@ skipping 220 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);
 })();