Issue 5169 - Add whitelisted tab to the new options page

new-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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 66 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.addItem = function(item)
-  {
-    if (this.items.indexOf(item) >= 0)
-      return;
-
-    this.items.push(item);
+  Collection.prototype._sortItems = function()
+  {
     this.items.sort((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;
-      if (a[timestampUI])
-        return b[timestampUI] ? b[timestampUI] - a[timestampUI] : -1;
-      if (b[timestampUI])
-        return a[timestampUI] ? a[timestampUI] - b[timestampUI] : 1;

[Thomas Greiner, 2017/05/26 11:10:39]

This condition is duplicated and contradictory because if `a` and `b` both have
timestamps it'll already return `b - a` above.

You could write it instead as

if (a[timestampUI] || b[timestampUI])
  return (b[timestampUI] || 0) - (a[timestampUI] || 0);

or if you want to spread it out a bit more:

let aTimestamp = a[timestampUI] || 0;
let bTimestamp = b[timestampUI] || 0;
if (a || b)
  return b - a;

[Thomas Greiner, 2017/05/26 11:10:40]

Detail: Please add a comment as we did with the special treatment for Acceptable
Ads above.

[saroyanm, 2017/05/31 08:30:27]

Done.

[saroyanm, 2017/05/31 08:30:28]

Done.

+
+      // Make sure that newly added entries always appear on top in descending
+      // chronological order
+      let aTimestamp = a[timestampUI] || 0;
+      let bTimestamp = b[timestampUI] || 0;
+      if (aTimestamp || bTimestamp)
+        return bTimestamp - aTimestamp;
 
       let aTitle = this._getItemTitle(a, 0).toLowerCase();
       let bTitle = this._getItemTitle(b, 0).toLowerCase();
       return aTitle.localeCompare(bTitle);
     });
-
+  };
+
+  Collection.prototype.addItem = function(item)
+  {
+    if (this.items.indexOf(item) >= 0)
+      return;
+
+    this.items.push(item);
+    this._sortItems();
     for (let j = 0; j < this.details.length; j++)
     {
       let table = E(this.details[j].id);
       let template = table.querySelector("template");
       let 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");
 
@@ skipping 61 matching lines @@
       }
 
       element.parentElement.removeChild(element);
       if (this.items.length == 0)
         this._setEmpty(table, detail.emptyText);
     }
   };
 
   Collection.prototype.updateItem = function(item)
   {
+    let oldIndex = this.items.indexOf(item);
+    this._sortItems();
     let access = (item.url || item.text).replace(/'/g, "\\'");
     for (let i = 0; i < this.details.length; i++)
     {
       let table = E(this.details[i].id);
       let element = table.querySelector("[data-access='" + access + "']");
       if (!element)
         continue;
 
       let title = this._getItemTitle(item, i);
       element.querySelector(".display").textContent = title;
@@ skipping 42 matching lines @@
       {
         if (item.homepage)
           websiteElement.setAttribute("href", item.homepage);
         else
           websiteElement.setAttribute("aria-hidden", true);
       }
 
       let sourceElement = element.querySelector(".context-menu .source");
       if (sourceElement)
         sourceElement.setAttribute("href", item.url);
+
+      let newIndex = this.items.indexOf(item);
+      if (oldIndex != newIndex)
+        table.insertBefore(element, table.childNodes[newIndex]);
     }
   };
 
   Collection.prototype.clearAll = function()
   {
     this.items = [];
     for (let detail of this.details)
     {
       let table = E(detail.id);
       let element = table.firstChild;
@@ skipping 479 matching lines @@
 
     updateShareLink();
     updateTooltips();
 
     // Initialize interactive UI elements
     document.body.addEventListener("click", onClick, false);
     document.body.addEventListener("keyup", onKeyUp, false);
     let placeholderValue = getMessage("options_dialog_language_find");
     E("find-language").setAttribute("placeholder", placeholderValue);
     E("find-language").addEventListener("keyup", onFindLanguageKeyUp, false);
-    let exampleValue = getMessages("options_whitelist_placeholder_example", 
+    let exampleValue = getMessage("options_whitelist_placeholder_example", 
       ["www.example.com"]);
     E("whitelisting-textbox").setAttribute("placeholder", exampleValue);
     E("whitelisting-textbox").addEventListener("keyup", (e) =>
     {
-      let addWhitelistButton = E("whitelisting-add-button");
-      addWhitelistButton.disabled = false;
-      if (getKey(e) == "Enter")
-      {
-        if (!addWhitelistButton.disabled)
-          addWhitelistedDomain();
-      }
-      else
-      {
-        if (!e.target.value)
-        {
-          addWhitelistButton.disabled = true;

[Thomas Greiner, 2017/05/26 11:10:39]

Detail: Now that we reduced it to a simple check, you could simplify this logic
a bit by writing it as:

addWhitelistButton.disabled = !e.target.value;


Also note that resetting `addWhitelistButton.disabled` to `false` causes the
above condition `!addWhitelistButton.disabled` to always be `true`.

[saroyanm, 2017/05/31 08:30:27]

Done.

-        }
-      }
+      E("whitelisting-add-button").disabled = !e.target.value;
     }, false);
 
     // Advanced tab
     let tweaks = document.querySelectorAll("#tweaks li[data-pref]");
     tweaks = Array.prototype.map.call(tweaks, (checkbox) =>
     {
       return checkbox.getAttribute("data-pref");
     });
     for (let key of tweaks)
     {
@@ skipping 192 matching lines @@
     });
   }
 
   function addWhitelistedDomain()
   {
     let domain = E("whitelisting-textbox");
     for (let whitelistItem of collections.whitelist.items)
     {
       if (whitelistItem.title == domain.value)
       {
-        collections.whitelist.removeItem(whitelistItem);
         whitelistItem[timestampUI] = Date.now();
-        collections.whitelist.addItem(whitelistItem);

[Thomas Greiner, 2017/05/26 11:10:40]

Removing and then adding the item again is quite a hack. Calling
`collections.whitelist.updateItem()` seems more appropriate.

[saroyanm, 2017/05/30 12:30:43]

I agree with you.

The problem is that we are currently sorting items only when we are adding them
(which I think is a bug), I think we can fix that behavior in current review, by
sorting items both when adding and updating.

[saroyanm, 2017/05/31 08:30:27]

Done.

+        collections.whitelist.updateItem(whitelistItem);
         domain.value = "";

[Thomas Greiner, 2017/05/26 11:10:39]

No need to continue the loop because we already found the item we were searching
for. So let's `break` here.

[saroyanm, 2017/05/31 08:30:27]

Done.

+        break;
       }
     }
     if (domain.value)
     {
       sendMessageHandleErrors({
         type: "filters.add",
         text: "@@||" + domain.value.toLowerCase() + "^$document"
       });
     }
 
@@ skipping 35 matching lines @@
       message.homepage = homepage;
 
     ext.backgroundPage.sendMessage(message);
   }
 
   function onFilterMessage(action, filter)
   {
     switch (action)
     {
       case "added":
+        filter[timestampUI] = Date.now();
         updateFilter(filter);
         updateShareLink();
         break;
       case "loaded":
         populateLists();
         break;
       case "removed":
         let knownFilter = filtersMap[filter.text];
         collections.whitelist.removeItem(knownFilter);
         collections.customFilters.removeItem(knownFilter);
@@ skipping 260 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);
 }