issue 3741 - Add "remove" option to list items in new options page

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 284 matching lines @@
     {
       id: "recommend-list-table",
       onClick: toggleRemoveSubscription
     }
   ]);
   collections.langs = new Collection(
   [
     {
       id: "blocking-languages-table",
       emptyText: "options_dialog_language_added_empty",
-      onClick: toggleRemoveSubscription
+      onClick: toggleDisableSubscription

[saroyanm, 2016/03/23 16:34:38]

I think we do not need this listeners being specified on collections, I think we
can specify them in HTML data-action="". 
I can handle that in this in this review or separately, please let me know know
what you think.

[Thomas Greiner, 2016/05/23 12:43:25]

I'd be fine with either but if it's not too much effort to do it'd be great to
include it in this review.

[saroyanm, 2016/06/08 15:21:06]

Done.

     },
     {
       id: "blocking-languages-dialog-table",
       emptyText: "options_dialog_language_added_empty"
     }
   ]);
   collections.allLangs = new Collection(
   [
     {
       id: "all-lang-table",
       emptyText: "options_dialog_language_other_empty",
       onClick: onAddLanguageSubscriptionClick
     }
   ]);
   collections.acceptableAds = new Collection(
   [
     {
       id: "acceptableads-table",
       onClick: toggleRemoveSubscription
     }
   ]);
   collections.custom = new Collection(
   [
     {
       id: "custom-list-table",
-      onClick: toggleRemoveSubscription
+      onClick: toggleDisableSubscription
     }
   ]);
   collections.whitelist = new Collection(
   [
     {
       id: "whitelisting-table",
       emptyText: "options_whitelisted_empty",
       onClick: onRemoveFilterClick
     }
   ]);
@@ skipping 11 matching lines @@
       onClick: toggleDisableSubscription
     }
   ]);
 
   function observeSubscription(subscription)
   {
     function onObjectChanged(change)
     {
       for (var i = 0; i < change.length; i++)
       {
-        if (change[i].name == "disabled")

[saroyanm, 2016/03/23 16:34:38]

This implementation doesn't belong to here as well, we should have handled this
when subscription were added or removed, I think currently the implementation
should be more understandable.

[saroyanm, 2016/05/10 14:26:16]

Please ignore this change as well, as specified below.

-        {
-          var recommendation = recommendationsMap[subscription.url];
-          if (recommendation && recommendation.type == "ads")
-          {
-            if (subscription.disabled == false)
-            {
-              collections.allLangs.removeItem(subscription);
-              collections.langs.addItems(subscription);
-            }
-            else
-            {
-              collections.allLangs.addItems(subscription);
-              collections.langs.removeItem(subscription);
-            }
-          }
-        }
         for (var i in collections)
           collections[i].updateItem(subscription);
       }
     }
 
     if (!Object.observe)
     {
       ["disabled", "lastDownload"].forEach(function(property)
       {
         subscription["$" + property] = subscription[property];
@@ skipping 26 matching lines @@
   function updateSubscription(subscription)
   {
     var subscriptionUrl = subscription.url;
     var knownSubscription = subscriptionsMap[subscriptionUrl];
     if (knownSubscription)
     {
       for (var property in subscription)
         if (property != "title")
           knownSubscription[property] = subscription[property];
     }
-    else
-    {

[saroyanm, 2016/03/23 16:34:38]

This doesn't have anything to do with updateSubscription() function, this should
be done whenever we are adding a new subscription, so I've updated this as well
in scope of current review, I think now it's make more sense. 

[saroyanm, 2016/05/10 14:26:16]

The implementation was changed, I decided to introduce this change and the one
above separately in case needed, so please ignore this and comment above, it
will make the review quicker and rebasing as well.

-      observeSubscription(subscription);
-      getAcceptableAdsURL(function(acceptableAdsUrl)
-      {
-        var collection = null;
-        if (subscriptionUrl in recommendationsMap)
-        {
-          var recommendation = recommendationsMap[subscriptionUrl];
-          if (recommendation.type != "ads")
-            collection = collections.popular;
-          else if (subscription.disabled == false)
-            collection = collections.langs;
-          else
-            collection = collections.allLangs;
-        }
-        else if (subscriptionUrl == acceptableAdsUrl)
-          collection = collections.acceptableAds;
-        else
-          collection = collections.custom;
-
-        collection.addItems(subscription);
-        subscriptionsMap[subscriptionUrl] = subscription;
-      });
-    }
   }
 
   function updateFilter(filter)
   {
     var match = filter.text.match(/^@@\|\|([^\/:]+)\^\$document$/);
     if (match && !filtersMap[filter.text])
     {
       filter.title = match[1];
       collections.whitelist.addItems(filter);
     }
@@ skipping 14 matching lines @@
       {
         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 subscription = Object.create(null);
           subscription.title = element.getAttribute("title");
           subscription.url = element.getAttribute("url");
-          subscription.disabled = null;
+          subscription.disabled = true;
           subscription.downloadStatus = null;
           subscription.homepage = null;
           var recommendation = Object.create(null);
           recommendation.type = element.getAttribute("type");
           var prefix = element.getAttribute("prefixes");
           if (prefix)
           {
             prefix = prefix.replace(/\W/g, "_");
             subscription.title = getMessage("options_language_" + prefix);
           }
           else
           {
             var type = recommendation.type.replace(/\W/g, "_");
             subscription.title = getMessage("common_feature_" + type + "_title");
           }
 
+          var collection = null;
+          if (recommendation.type != "ads")
+            collection = collections.popular;
+          else
+            collection = collections.allLangs;
+
+          collection.addItems(subscription);
           recommendationsMap[subscription.url] = recommendation;
-          updateSubscription(subscription);
+          subscriptionsMap[subscription.url] = subscription;
+          observeSubscription(subscription);
         }
       });
   }
 
   function findParentData(element, dataName, returnElement)
   {
     while (element)
     {
       if (element.hasAttribute("data-" + dataName))
         return returnElement ? element : element.getAttribute("data-" + dataName);
@@ skipping 313 matching lines @@
         });
       }
     });
     loadRecommendations();
     getAcceptableAdsURL(function(acceptableAdsUrl)
     {
       var subscription = Object.create(null);
       subscription.url = acceptableAdsUrl;
       subscription.disabled = true;
       subscription.title = getMessage("options_acceptableAds_description");
-      updateSubscription(subscription);
+      collections.acceptableAds.addItems(subscription);
+      subscriptionsMap[subscription.url] = subscription;
+      observeSubscription(subscription);
 
       // Load user subscriptions
       ext.backgroundPage.sendMessage(
       {
         type: "subscriptions.get",
         downloadable: true
       }, 
       function(subscriptions)
       {
         for (var i = 0; i < subscriptions.length; i++)
@@ skipping 79 matching lines @@
         updateShareLink();
         break;
     }
   }
 
   function onSubscriptionMessage(action, subscription)
   {
     switch (action)
     {
       case "added":
-        updateSubscription(subscription);
-        updateShareLink();
+        getAcceptableAdsURL(function(acceptableAdsUrl)
+        {
+          updateSubscription(subscription);
+          if (subscriptionsMap[subscription.url])
+            subscription = subscriptionsMap[subscription.url];
+          else
+            subscriptionsMap[subscription.url] = subscription;
 
-        var knownSubscription = subscriptionsMap[subscription.url];
-        if (knownSubscription)
-          collections.filterLists.addItems(knownSubscription);
-        else
+          var recommendation = recommendationsMap[subscription.url];
+          if (recommendation)
+          {
+            if (recommendation.type == "ads")
+            {
+              collections.langs.addItems(subscription);
+              collections.allLangs.removeItem(subscription);
+            }
+          }
+          else if (subscription.url != acceptableAdsUrl)
+          {
+            collections.custom.addItems(subscription);
+            observeSubscription(subscription);
+          }
           collections.filterLists.addItems(subscription);
+          updateShareLink();
+        });
         break;
       case "disabled":
         updateSubscription(subscription);
         updateShareLink();
         break;
       case "lastDownload":
         updateSubscription(subscription);
         break;
       case "homepage":
         // TODO: NYI
         break;
       case "removed":
-        var knownSubscription = subscriptionsMap[subscription.url];
+        subscription = subscriptionsMap[subscription.url];
         getAcceptableAdsURL(function(acceptableAdsUrl)
         {
+          collections.filterLists.removeItem(subscription);
           if (subscription.url == acceptableAdsUrl)
           {
             subscription.disabled = true;
-            updateSubscription(subscription);
           }
           else
           {
-            if (subscription.url in recommendationsMap)
-              knownSubscription.disabled = true;
+            var recommendation = recommendationsMap[subscription.url];
+            if (recommendation && recommendation.type == "ads")
+            {
+              collections.allLangs.addItems(subscription);
+              collections.langs.removeItem(subscription);
+            }
+            else if (recommendation)
+              subscription.disabled = true;
             else
             {
-              collections.custom.removeItem(knownSubscription);
+              collections.custom.removeItem(subscription);
               delete subscriptionsMap[subscription.url];
             }
           }
           updateShareLink();
-          collections.filterLists.removeItem(knownSubscription);
         });
         break;
       case "title":
         // TODO: NYI
         break;
     }
   }
 
   function hidePref(key, value)
   {
@@ skipping 133 matching lines @@
   });
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
         "title"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 })();