Issue 3885 - Disable the checkbox to toggle the Acceptable Ads list on the options page

options.js

Index: options.js
===================================================================
--- a/options.js
+++ b/options.js
@@ -23,6 +23,7 @@
   var recommendationsMap = Object.create(null);
   var filtersMap = Object.create(null);
   var collections = Object.create(null);
+  var acceptableAdsUrl = null;
   var maxLabelId = 0;
   var getMessage = ext.i18n.getMessage;
   var filterErrors =
@@ -62,6 +63,22 @@
     };
   };
 
+  Collection.prototype._isControlDisabled = function(item, i)
+  {
+    return item.url == acceptableAdsUrl &&
+           this.details[i].onClick == toggleDisableSubscription;
+  };
+
+  Collection.prototype._getItemTitle = function(item, i)
+  {
+    var title = null;
+    if (this.details[i].useOriginalTitle)
+      title = item.originalTitle;
+    if (!title)
+      title = item.title || item.url || item.text;
+    return title;
+  };
+
   Collection.prototype.addItems = function() 
   {
     var length = Array.prototype.push.apply(this.items, arguments);
@@ -70,10 +87,15 @@
 
     this.items.sort(function(a, b)
     {
-      var aValue = (a.title || a.text || a.url).toLowerCase();
-      var bValue = (b.title || b.text || b.url).toLowerCase();
-      return aValue.localeCompare(bValue);
-    });
+      var aDisabled = this._isControlDisabled(a, 0);
+      var bDisabled = this._isControlDisabled(b, 0);

[Thomas Greiner, 2016/04/07 17:21:15]

It's a nice idea to group together all disabled checkboxes but it's not ideal
that we have to hardcode the table index since this special ordering is only
useful for Acceptable Ads and only for this one table. That is especially since
we intend to get rid of disabled checkboxes later on. What about instead
checking specifically for the whitelist here to ensure that it's at the end of
the list?

e.g.
this.items.sort(function(a, b)
{
  if (a.url == acceptableAdsUrl)
    return 1;
  
  ...
});

[Sebastian Noack, 2016/04/08 14:22:17]

Yeah, hard-coding the index isn't great, but we have to do so for
_getItemTitle() anyway. And the way I did it it was more obvious that we are
grouping disabled/enabled items respectively. Now it need a comment to explain
what that is about. But fair enough. Done.

+      if (aDisabled != bDisabled)
+        return aDisabled - bDisabled;
+
+      var aTitle = this._getItemTitle(a, 0).toLowerCase();

[Thomas Greiner, 2016/04/07 17:21:15]

Detail: Hardcoding the table index here might cause items to be not
alphabetically ordered. However, I assume that fixing this will require major
changes in the way we keep the UI in sync with the data so feel free to ignore
it for this review.

[Sebastian Noack, 2016/04/08 14:22:17]

I know. This is kinda a hack. But at least more correct than previously.

+      var bTitle = this._getItemTitle(b, 0).toLowerCase();
+      return aTitle.localeCompare(bTitle);
+    }.bind(this));
 
     for (var j = 0; j < this.details.length; j++) 
     {
@@ -168,17 +190,17 @@
       if (!element)
         continue;
 
-      var title = null;
-      if (this.details[i].useOriginalTitle)
-        title = item.originalTitle;
-      if (!title)
-        title = item.title || item.url || item.text;
+      var title = this._getItemTitle(item, i);
       element.querySelector(".display").textContent = title;
       if (title)
         element.setAttribute("data-search", title.toLowerCase());
       var control = element.querySelector(".control[role='checkbox']");
       if (control)
+      {
         control.setAttribute("aria-checked", item.disabled == false);
+        if (this._isControlDisabled(item, i))
+          control.setAttribute("disabled", true);
+      }
 
       var dateElement = element.querySelector(".date");
       var timeElement = element.querySelector(".time");
@@ -434,27 +456,25 @@
     else
     {
       observeSubscription(subscription);
-      getAcceptableAdsURL(function(acceptableAdsUrl)
+
+      var collection;
+      if (subscriptionUrl in recommendationsMap)
       {
-        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;
+        var recommendation = recommendationsMap[subscriptionUrl];
+        if (recommendation.type != "ads")
+          collection = collections.popular;
+        else if (subscription.disabled == false)
+          collection = collections.langs;
         else
-          collection = collections.custom;
+          collection = collections.allLangs;
+      }
+      else if (subscriptionUrl == acceptableAdsUrl)
+        collection = collections.acceptableAds;
+      else
+        collection = collections.custom;
 
-        collection.addItems(subscription);
-        subscriptionsMap[subscriptionUrl] = subscription;
-      });
+      collection.addItems(subscription);
+      subscriptionsMap[subscriptionUrl] = subscription;
     }
   }
 
@@ -843,13 +863,19 @@
       }
     });
     loadRecommendations();
-    getAcceptableAdsURL(function(acceptableAdsUrl)
+    ext.backgroundPage.sendMessage(
     {
-      var subscription = Object.create(null);
-      subscription.url = acceptableAdsUrl;
-      subscription.disabled = true;
-      subscription.title = getMessage("options_acceptableAds_description");
-      updateSubscription(subscription);
+      type: "prefs.get",
+      key: "subscriptions_exceptionsurl"
+    },
+    function(url)
+    {
+      acceptableAdsUrl = url;
+      updateSubscription({
+        url: acceptableAdsUrl,
+        disabled: true,
+        title: getMessage("options_acceptableAds_description")
+      });
 
       // Load user subscriptions
       ext.backgroundPage.sendMessage(
@@ -890,18 +916,6 @@
     E("custom-filters-raw").value = filterTexts.join("\n");
   }
 
-  function getAcceptableAdsURL(callback)
-  {
-    getPref("subscriptions_exceptionsurl", function(value)
-    {
-      getAcceptableAdsURL = function(callback)
-      {
-        callback(value);
-      };
-      getAcceptableAdsURL(callback);
-    });
-  }
-
   function addEnableSubscription(url, title, homepage)
   {
     var messageType = null;
@@ -964,26 +978,23 @@
         break;
       case "removed":
         var knownSubscription = subscriptionsMap[subscription.url];
-        getAcceptableAdsURL(function(acceptableAdsUrl)
+        if (subscription.url == acceptableAdsUrl)
         {
-          if (subscription.url == acceptableAdsUrl)
-          {
-            subscription.disabled = true;
-            updateSubscription(subscription);
-          }
+          subscription.disabled = true;
+          updateSubscription(subscription);
+        }
+        else
+        {
+          if (subscription.url in recommendationsMap)
+            knownSubscription.disabled = true;
           else
           {
-            if (subscription.url in recommendationsMap)
-              knownSubscription.disabled = true;
-            else
-            {
-              collections.custom.removeItem(knownSubscription);
-              delete subscriptionsMap[subscription.url];
-            }
+            collections.custom.removeItem(knownSubscription);
+            delete subscriptionsMap[subscription.url];
           }
-          updateShareLink();
-          collections.filterLists.removeItem(knownSubscription);
-        });
+        }
+        updateShareLink();
+        collections.filterLists.removeItem(knownSubscription);
         break;
       default:
         updateSubscription(subscription);