Issue 5255 - Advanced tab (HTML, strings and functionality)

new-options.js

Index: new-options.js
===================================================================
--- a/new-options.js
+++ b/new-options.js
@@ -27,6 +27,7 @@
   let acceptableAdsUrl = null;
   let isCustomFiltersLoaded = false;
   let {getMessage} = ext.i18n;
+  let customFiltersArray = [];

[Thomas Greiner, 2017/07/14 12:26:13]

Detail: I'd recommend not to include the value type in the variable name. For
instance, the variable name "customFiltersListElement" will need to be updated
if we decide not to use `<li>` elements to represent those anymore.

Something more generic like "customFilters" instead of "customFiltersArray" and
"customFiltersElement" instead of "customFiltersListElement" would already be
sufficient to avoid such inconsistencies.

[saroyanm, 2017/07/14 16:17:25]

Acknowledged.

[saroyanm, 2017/07/14 17:11:06]

Done.

   let filterErrors = new Map([
     ["synchronize_invalid_url",
      "options_filterList_lastDownload_invalidURL"],
@@ -38,6 +39,12 @@
      "options_filterList_lastDownload_checksumMismatch"]
   ]);
 
+  const whitelistedDomainRegexp = /^@@\|\|([^/:]+)\^\$document$/;
+  // Period of time in milliseconds
+  const minuteInMs = 60000;
+  const hourInMs = 3600000;
+  const fullDayInMs = 86400000;
+
   function Collection(details)
   {
     this.details = details;
@@ -100,7 +107,8 @@
 
     for (let j = 0; j < this.details.length; j++)
     {
-      let table = E(this.details[j].id);
+      let detail = this.details[j];
+      let table = E(detail.id);
       let template = table.querySelector("template");
       let listItem = document.createElement("li");
       listItem.appendChild(document.importNode(template.content, true));
@@ -126,13 +134,11 @@
       }
 
       this._setEmpty(table, null);
-      if (table.hasChildNodes())
-      {
-        table.insertBefore(listItem,
-            table.childNodes[this.items.indexOf(item)]);
-      }
+      if (table.children.length > 0)
+        table.insertBefore(listItem, table.children[this.items.indexOf(item)]);
       else
         table.appendChild(listItem);
+
       this.updateItem(item);
     }
     return length;
@@ -200,9 +206,8 @@
           control.setAttribute("disabled", true);
       }
 
-      let dateElement = element.querySelector(".date");
-      let timeElement = element.querySelector(".time");
-      if (dateElement && timeElement)
+      let lastUpdateElement = element.querySelector(".last-update");
+      if (lastUpdateElement)
       {
         let message = element.querySelector(".message");
         if (item.isDownloading)
@@ -222,9 +227,39 @@
         }
         else if (item.lastDownload > 0)
         {
-          let dateTime = i18nFormatDateTime(item.lastDownload * 1000);
-          dateElement.textContent = dateTime[0];
-          timeElement.textContent = dateTime[1];
+          let lastUpdate = item.lastDownload * 1000;
+          let lastUpdateLive = function()
+          {
+            let sinceUpdate = Date.now() - lastUpdate;
+            if (sinceUpdate > fullDayInMs)
+            {
+              let lastUpdateDate = new Date(item.lastDownload * 1000);
+              let monthName = lastUpdateDate.toLocaleString(undefined,
+                {month: "short"});
+              let day = lastUpdateDate.getDate();
+              day = day < 10 ? "0" + day : day;
+              lastUpdateElement.textContent = day + " " + monthName + " " +
+                lastUpdateDate.getFullYear();
+            }
+            else if (sinceUpdate > hourInMs)
+            {
+              let placeholder = [Math.round(sinceUpdate / hourInMs)];
+              lastUpdateElement.textContent =
+                getMessage("options_filterList_hours", placeholder);

[Thomas Greiner, 2017/07/14 12:26:12]

This placeholder isn't defined in new-options.json and in the last meeting we
agreed on leaving out the numbers as a compromise to not having to implement the
live updating of the dates.

Therefore there'd be no need for this placeholder.

[saroyanm, 2017/07/14 16:17:24]

Well spotted, will remove it.

[saroyanm, 2017/07/14 17:11:06]

Done.

+            }
+            else if (sinceUpdate > minuteInMs)
+            {
+              let placeholder = [Math.round(sinceUpdate / minuteInMs)];
+              lastUpdateElement.textContent =
+                getMessage("options_filterList_minutes", placeholder);
+            }
+            else
+            {
+              lastUpdateElement.textContent =
+                getMessage("options_filterList_now");
+            }
+          };
+          lastUpdateLive();
           element.classList.remove("show-message");
         }
       }
@@ -315,12 +350,6 @@
       emptyText: "options_whitelisted_empty"
     }
   ]);
-  collections.customFilters = new Collection([
-    {
-      id: "custom-filters-table",
-      emptyText: "options_customFilters_empty"
-    }
-  ]);
   collections.filterLists = new Collection([
     {
       id: "all-filter-lists-table",
@@ -378,18 +407,38 @@
 
   function updateFilter(filter)
   {
-    let match = filter.text.match(/^@@\|\|([^/:]+)\^\$document$/);
+    let match = filter.text.match(whitelistedDomainRegexp);
     if (match && !filtersMap[filter.text])
     {
       filter.title = match[1];
       collections.whitelist.addItem(filter);
     }
     else
-      collections.customFilters.addItem(filter);
+    {
+      customFiltersArray.push(filter.text);
+      if (isCustomFiltersLoaded)

[Thomas Greiner, 2017/07/14 12:26:12]

Detail: What is this check for? It seems that `updateCustomFiltersUi()` isn't
doing anything that requires custom filters to be loaded.

[saroyanm, 2017/07/14 16:17:25]

Shouldn't we wait only for the time when all filters are loaded to update the
view ? Otherwise the view will be updated for each filter, on start. ex.: When
user have a lot of filters, this might have performance impact.

[Thomas Greiner, 2017/07/14 16:37:42]

Right, I remember and I see that this has actually already been there before.

Presumably, the most appropriate way would be to handle this within the
collection but let's leave it like this for now since it works.

[saroyanm, 2017/07/14 16:41:34]

I agree, I'll create a separate issue for that, when this review is resolved.

+        updateCustomFiltersUi();
+    }
 
     filtersMap[filter.text] = filter;
   }
 
+  function removeCustomFilter(text)
+  {
+    let index = customFiltersArray.indexOf(text);
+    if (index >= 0)
+      customFiltersArray.splice(index, 1);

[Thomas Greiner, 2017/07/14 12:26:12]

What if there are multiple instances of the same filter?

[saroyanm, 2017/07/14 16:17:25]

There shouldn't be multiple instances of same filter AFAIK. Extension should not
allow having duplicated instances ?

[Thomas Greiner, 2017/07/14 16:37:42]

My thinking was that a pre-existing filter could already be duplicated even
before our UI has had the chance to check for that. But seeing that the current
options page also checks for duplicates and also doesn't consider this
circumstance, we should be safe if we simply ignore this.

[saroyanm, 2017/07/14 16:41:34]

Acknowledged.

+
+    updateCustomFiltersUi();
+  }
+
+  function updateCustomFiltersUi()
+  {
+    let customFiltersListElement = E("custom-filters-raw");
+    customFiltersListElement.value = "";

[Thomas Greiner, 2017/07/14 12:26:12]

Detail: This is redundant because the line below will have the same effect if
`customFiltersArray` is empty.

[saroyanm, 2017/07/14 16:17:24]

Thanks for noticing. Will update.

[saroyanm, 2017/07/14 17:11:06]

Done.

+    customFiltersListElement.value = customFiltersArray.join("\n");
+  }
+
   function loadRecommendations()
   {
     fetch("subscriptions.xml")
@@ -474,7 +523,7 @@
     location.hash = id;
   }
 
-  function execAction(action, element)
+  function execAction(action, element, event)
   {
     switch (action)
     {
@@ -493,7 +542,8 @@
         break;
       }
       case "cancel-custom-filters":
-        E("custom-filters").classList.remove("mode-edit");
+        updateCustomFiltersUi();
+        setCustomFiltersView("read");
         break;
       case "cancel-domain-exception":
         E("whitelisting-textbox").value = "";
@@ -504,7 +554,7 @@
         closeDialog();
         break;
       case "edit-custom-filters":
-        editCustomFilters();
+        setCustomFiltersView("write");
         break;
       case "edit-domain-exception":
         document.querySelector("#whitelisting .controls").classList
@@ -553,7 +603,7 @@
         },
         () =>
         {
-          E("custom-filters").classList.remove("mode-edit");
+          setCustomFiltersView("read");
         });
         break;
       case "switch-tab":
@@ -599,6 +649,28 @@
     }
   }
 
+  function setCustomFiltersView(mode)
+  {
+    let customFilters = E("custom-filters");
+    let customFiltersListElement = E("custom-filters-raw");
+    if (mode == "read")
+    {
+      customFiltersListElement.disabled = true;
+      if (!customFiltersListElement.value)
+      {
+        setCustomFiltersView("empty");
+        return;
+      }
+    }
+    else if (mode == "write")
+    {
+      updateCustomFiltersUi();

[Thomas Greiner, 2017/07/14 12:26:13]

Interesting that you're calling `updateCustomFiltersUi()` only for the mode
"write". Is there a reason why you're not calling it unconditionally to ensure
that the list will always be up-to-date whenever switching its mode?

[saroyanm, 2017/07/14 16:17:25]

You are right, I'm not sure why I did this.. I'll fix this

[saroyanm, 2017/07/14 17:11:06]

Done.

+      customFiltersListElement.disabled = false;
+    }
+
+    customFilters.dataset.mode = mode;
+  }
+
   function onClick(e)
   {
     let context = document.querySelector(".show-context-menu");
@@ -612,7 +684,7 @@
     actions = actions.split(",");
     for (let action of actions)
     {
-      execAction(action, e.target);
+      execAction(action, e.target, e);
     }
   }
 
@@ -659,7 +731,7 @@
     let actions = container.getAttribute("data-action").split(",");
     for (let action of actions)
     {
-      execAction(action, element);
+      execAction(action, element, e);
     }
   }
 
@@ -684,10 +756,6 @@
     let tabContentId = tab.getAttribute("aria-controls");
     let tabContent = document.getElementById(tabContentId);
 
-    // Select sub tabs
-    if (tab.hasAttribute("data-subtab"))
-      selectTabItem(tab.getAttribute("data-subtab"), tabContent, false);
-
     if (tab && focus)
       tab.focus();
 
@@ -757,12 +825,12 @@
     }, false);
 
     // Advanced tab
-    let tweaks = document.querySelectorAll("#tweaks li[data-pref]");
-    tweaks = Array.prototype.map.call(tweaks, (checkbox) =>
+    let customize = document.querySelectorAll("#customize li[data-pref]");
+    customize = Array.prototype.map.call(customize, (checkbox) =>
     {
       return checkbox.getAttribute("data-pref");
     });
-    for (let key of tweaks)
+    for (let key of customize)
     {
       getPref(key, (value) =>
       {
@@ -778,26 +846,18 @@
       hidePref("show_devtools_panel", !features.devToolsPanel);
     });
 
-    let filterTextbox = document.querySelector("#custom-filters-add input");
-    placeholderValue = getMessage("options_customFilters_textbox_placeholder");
-    filterTextbox.setAttribute("placeholder", placeholderValue);
-    function addCustomFilters()
+    getDocLink("filterdoc", (link) =>
     {
-      let filterText = filterTextbox.value;
-      sendMessageHandleErrors({
-        type: "filters.add",
-        text: filterText
-      },
-      () =>
-      {
-        filterTextbox.value = "";
-      });
-    }
-    E("custom-filters-add").addEventListener("submit", (e) =>
+      E("link-filters").setAttribute("href", link);
+    });
+
+    getDocLink("subscriptions", (link) =>
     {
-      e.preventDefault();
-      addCustomFilters();
-    }, false);
+      setLinks("filter-lists-description", link);
+    });
+
+    E("custom-filters-raw").setAttribute("placeholder",
+      getMessage("options_customFilters_edit_placeholder", ["/ads/track/*"]));
 
     // Help tab
     getDocLink("faq", (link) =>
@@ -930,6 +990,8 @@
             updateFilter(filter);
 
           isCustomFiltersLoaded = true;
+          updateCustomFiltersUi();
+          setCustomFiltersView("read");
         });
       }
     });
@@ -975,21 +1037,6 @@
       .classList.remove("mode-edit");
   }
 
-  function editCustomFilters()
-  {
-    if (!isCustomFiltersLoaded)
-    {
-      console.error("Custom filters are not loaded");
-      return;
-    }
-
-    E("custom-filters").classList.add("mode-edit");
-    let filterTexts = [];
-    for (let customFilterItem of collections.customFilters.items)
-      filterTexts.push(customFilterItem.text);
-    E("custom-filters-raw").value = filterTexts.join("\n");
-  }
-
   function addEnableSubscription(url, title, homepage)
   {
     let messageType = null;
@@ -1024,8 +1071,11 @@
         break;
       case "removed":
         let knownFilter = filtersMap[filter.text];
-        collections.whitelist.removeItem(knownFilter);
-        collections.customFilters.removeItem(knownFilter);
+        if (whitelistedDomainRegexp.test(knownFilter.text))
+          collections.whitelist.removeItem(knownFilter);
+        else
+          removeCustomFilter(filter.text);
+
         delete filtersMap[filter.text];
         updateShareLink();
         break;