Issue 2375 - Implement "Blocking lists" section in new options page

options.js

Index: options.js
===================================================================
--- a/options.js
+++ b/options.js
@@ -72,6 +72,7 @@
         if (text)
           listItem.setAttribute("data-search", text.toLowerCase());
 
+        updateTimeDate(listItem, item);
         var control = listItem.querySelector(".control");
         if (control)
         {
@@ -81,7 +82,13 @@
 
         this._setEmpty(table, null);
         if (table.hasChildNodes())
-          table.insertBefore(listItem, table.childNodes[this.items.indexOf(item)]);
+        {
+          var index = this.items.indexOf(item);
+          if (table.firstChild.classList.contains("head"))
+            index++;
+
+          table.insertBefore(listItem, table.childNodes[index]);
+        }
         else
           table.appendChild(listItem);
       }
@@ -96,10 +103,11 @@
       return;
 
     this.items.splice(index, 1);
+    var access = (item.url || item.text).replace(/'/g, "\\'");

[Thomas Greiner, 2016/01/19 11:27:28]

I notice the issues with accessing elements by their index so I understand the
need for using this accessor. However, please extract this into its own function
so that we don't have to implement this logic wherever we need to determine the
access value of an item.

For example:

Collection.prototype._createElementQuery = function(item)
{
  var access = (item.url || item.text).replace(/'/g, "\\'");
  return function(container)
  {
    return container.querySelector("[data-access='" + access + "']");
  };
};

[saroyanm, 2016/01/22 09:55:08]

Done, but wouldn't it be simpler to just have a private method that takes two
parameters and return the result ? 
Like: 
Collection.prototype._getListElement = function(item, container)
{
  return container.querySelector("[data-access='" + (item.url ||
item.text).replace(/'/g, "\\'") + "']");
};

What is the benefit of returning the function ?
Maybe I don't understand your approach.

[Thomas Greiner, 2016/01/25 15:40:28]

By returning a function we only create the "access" string once instead of
everytime we call the function. It's not a big difference overall but my code
was only a suggestion anyway so feel free to implement it either way.

     for (var i = 0; i < this.details.length; i++)
     {
       var table = E(this.details[i].id);
-      var element = table.childNodes[index];
+      var element = table.querySelector("[data-access='" + access + "']");
       element.parentElement.removeChild(element);
       if (this.items.length == 0)
         this._setEmpty(table, this.details[i].emptyText);
@@ -113,12 +121,79 @@
     {
       var table = E(this.details[i].id);
       var template = table.querySelector("template");
+      var staticElements = [];

[Thomas Greiner, 2016/01/19 11:27:29]

This logic is quite complicated. Assuming that you merely want to know at which
position elements should be inserted you could go with the following approach:

1. Create a document fragment: document.createDocumentFragment()
2. Append list elements to the document fragment: fragment.appendChild()
3. Insert the document fragment at the appropriate position:
template.parentNode.insertBefore(fragment, template)

Obviously, that means that you won't be able to clear it with `table.innerHTML =
""` anymore and that you therefore have to remove each element individually but
that should be fine since we're only calling this method in `populateLists()`.

[saroyanm, 2016/01/22 09:55:10]

Currently we are not using createDocumentFragment to add List item, I've updated
current removal implementation to only remove the dynamic list items, I also
need to remove text nodes otherwise it's becoming messy to always determine the
type of the node.

[Thomas Greiner, 2016/01/25 15:40:27]

You could use `Element.nextElementSibling` to avoid other kinds of nodes. So
checking for the "static" class and `<template>` elements should be sufficient.

Alternatively, you could use `table.children` to get a list of all child
elements.

In any case you need to be careful since you're removing the element from the
DOM. The safest approach would be to use
`Array.prototype.slice.call(table.children)` and loop over the items in the
array.

[saroyanm, 2016/01/26 18:36:15]

I assume while you didn't commented under updated code, that mean you are okey
with the implementation in patch set "#2"

[Thomas Greiner, 2016/01/27 17:16:56]

Thanks for reminding me. I wrote down some parts that I still wanted to comment
on but forgot about those so I added them now.

+      var element = table.firstChild;
+      while (element)
+      {
+        if (element.tagName == "TEMPLATE" || 
+            (element.classList && element.classList.contains("static")))
+          staticElements.push(element);
+        element = element.nextSibling
+      }
+
       table.innerHTML = "";
-      table.appendChild(template);
+      for (var j = 0; j < staticElements.length; j++)
+        table.appendChild(staticElements[j]);
+
       this._setEmpty(table, this.details[i].emptyText);
     }
   };
 
+  Collection.prototype.getTableIds = function() 

[Thomas Greiner, 2016/01/19 11:27:29]

This information is an implementation detail and should therefore stay within
the Collection.

[saroyanm, 2016/01/22 09:55:09]

updated the method to check if the collection has ID.

+  {
+    var ids = [];
+    for (var i = 0; i < this.details.length; i++)
+      ids.push(this.details[i].id)
+    
+    return ids;
+  };
+
+  function updateTimeDate(listItem, subscription)
+  {
+    var dateElement = listItem.querySelector(".date");
+    var timeElement = listItem.querySelector(".time");
+
+    if(dateElement && timeElement)
+    {
+      if (subscription.downloadStatus && 
+          subscription.downloadStatus != "synchronize_ok")
+      {
+        var map =
+        {
+          "synchronize_invalid_url": "options_subscription_lastDownload_invalidURL",
+          "synchronize_connection_error": "options_subscription_lastDownload_connectionError",
+          "synchronize_invalid_data": "options_subscription_lastDownload_invalidData",
+          "synchronize_checksum_mismatch": "options_subscription_lastDownload_checksumMismatch"
+        };
+        if (subscription.downloadStatus in map)
+          timeElement.textContent = ext.i18n.getMessage(map[subscription.downloadStatus]);
+        else
+          timeElement.textContent = subscription.downloadStatus;
+      }
+      else if (subscription.lastDownload > 0)
+      {
+        var timedate = i18n_timeDateStrings(subscription.lastDownload * 1000);

[Thomas Greiner, 2016/01/19 11:27:29]

This function returns the localized date-time string which is not what you're
looking for because it will differ (e.g. "1970/30/1" or "1970/1/30" or
"1.1.1970") and thereby cause unexpected results and forces you to do all those
modificiations and checks below.

Therefore please don't use this function and instead implement it yourself. Feel
free to add it to "i18n.js" but note that we cannot remove
"i18n_timeDateStrings()" yet because it's still being used in the current
options page.

[saroyanm, 2016/01/22 09:55:10]

I see, I was trying to make it consistent with old code, didn't noticed the
localization part thanks, not really sure if we need separate method for this in
i18n, because it has nothing to do with localization currently, but I have
implemented the logic in current function, I think should be enough for now, but
let me know if you expect this logic to do something else.

[Thomas Greiner, 2016/01/25 15:40:28]

The mere formatting of the string should be generic enough for inclusion in
"i18n.js", all the other parts should stay here.

e.g.

var dateTime = i18n_formatDateTime(date);
dateElement.textContent = dateTime[0];
timeElement.textContent = dateTime[1];

Eventually, we might need something even more generic (e.g.
`i18n_formatDateTime(date, "YYYY-MM-DD hh:mm")`) but that'd probably be too much
effort for this specific use case.

Anyway, please note that the code you implemented formats the date as
"2016-111-1 1:1" instead of the expected "2016-12-01 01:01".

[saroyanm, 2016/01/26 18:36:15]

Done.

+        if (timedate[1])
+          dateElement.textContent = timedate[1].split("/").reverse().join("-");
+        else
+        {
+          var today = new Date();
+          dateElement.textContent = today.getFullYear() + "-" +
+                                    today.getMonth()+1 + "-" + today.getDate();
+        }
+        var time = timedate[0].split(":");
+        time[2] = time[2].split(" ")[1];
+        if (time[2] == "AM" && time[0] == "12")
+          time[0] = 0;
+        else if (time[2] == "PM")
+          time[0] = parseInt(time[0]) + 12;
+        
+        time.pop();
+        timeElement.textContent = time.join(":");
+      }
+    }
+  }
+
   function onToggleSubscriptionClick(e)

[Thomas Greiner, 2016/01/19 11:27:29]

I'd suggest renaming this function to "toggleRemoveSubscription" and the one
below to "toggleDisableSubscription" to clarify how their behavior differs.

[saroyanm, 2016/01/22 09:55:08]

Done.

   {
     e.preventDefault();
@@ -134,6 +209,17 @@
       addEnableSubscription(subscriptionUrl);
   }
 
+  function onToggleSubscriptionStateClick(e)
+  {
+    e.preventDefault();
+    var subscriptionUrl = e.target.parentNode.getAttribute("data-access");

[Thomas Greiner, 2016/01/19 11:27:29]

What about reusing `getParentAccessElement()` since you already created it?

[saroyanm, 2016/01/22 09:55:10]

Done.

[Thomas Greiner, 2016/01/25 15:40:28]

I don't see that you changed anything here.

[saroyanm, 2016/01/26 18:36:15]

Now for sure, thanks.

+    ext.backgroundPage.sendMessage(
+    {
+      type: "subscriptions.toggleState",
+      url: subscriptionUrl
+    });
+  }
+
   function onAddLanguageSubscriptionClick(e)
   {
     e.preventDefault();
@@ -207,31 +293,40 @@
       emptyText: "options_customFilters_empty"
     }
   ]);
+  collections.blockingLists = new Collection(
+  [
+    {
+      id: "blocking-lists-table",
+      onClick: onToggleSubscriptionStateClick
+    }
+  ]);
 
-  function updateSubscription(subscription)
+  function observeSubscription(subscription)
   {
-    var subscriptionUrl = subscription.url;
-    var knownSubscription = subscriptionsMap[subscriptionUrl];
-    if (knownSubscription)
-      knownSubscription.disabled = subscription.disabled;
-    else
+    function onObjectChanged(change)
     {
-      getAcceptableAdsURL(function(acceptableAdsUrl)
+      for (var i = 0; i < change.length; i++)
       {
-        function onObjectChanged()
+        var property = change[i].name;
+        if (property == "disabled")
         {
-          var access = (subscriptionUrl || subscription.text).replace(/'/g, "\\'");
+          var access = (subscription.url || 
+                        subscription.text).replace(/'/g, "\\'");
           var elements = document.querySelectorAll("[data-access='" + access + "']");
           for (var i = 0; i < elements.length; i++)
           {
             var element = elements[i];
+            var tableId = element.parentElement ? element.parentElement.id : "";
             var control = element.querySelector(".control");
-            if (control.localName == "input")
+            if (control && control.localName == "input")
               control.checked = subscription.disabled == false;
-            if (subscriptionUrl in recommendationsMap)
+            if (subscription.url in recommendationsMap)
             {
-              var recommendation = recommendationsMap[subscriptionUrl];
-              if (recommendation.type == "ads")
+              var recommendation = recommendationsMap[subscription.url];
+              var langids = collections.langs.getTableIds();
+              var allLangIds = collections.allLangs.getTableIds();
+              if (recommendation.type == "ads" && 
+                  langids.concat(allLangIds).indexOf(tableId) != -1)
               {
                 if (subscription.disabled == false)
                 {
@@ -247,33 +342,58 @@
             }
           }
         }
+        else 
+        {
+          var blockingListId = collections.blockingLists.details[0].id;
+          var blockingList = document.getElementById(blockingListId);
+          var listItem = blockingList.querySelector("[data-access='" +
+                          subscription.url + "']");
+          updateTimeDate(listItem, subscription);
+        }
+      }
+    }
 
-        if (!Object.observe)
+    if (!Object.observe)
+    {
+      ["disabled", "lastDownload"].forEach(function(property)
+      {
+        subscription["$" + property] = subscription[property];
+        Object.defineProperty(subscription, property,
         {
-          // Currently only "disabled" property of subscription used for observation
-          // but with Advanced tab implementation we should also add more properties.
-          ["disabled"].forEach(function(property)
+          get: function()
           {
-            subscription["$" + property] = subscription[property];
-            Object.defineProperty(subscription, property,
-            {
-              get: function()
-              {
-                return this["$" + property];
-              },
-              set: function(value)
-              {
-                this["$" + property] = value;
-                onObjectChanged();
-              }
-            });
-          });
-        }
-        else
-        {
-          Object.observe(subscription, onObjectChanged);
-        }
+            return this["$" + property];
+          },
+          set: function(value)
+          {
+            this["$" + property] = value;
+            var change = Object.create(null);
+            change.name = property;
+            onObjectChanged([change]);
+          }
+        });
+      });
+    }
+    else
+    {
+      Object.observe(subscription, onObjectChanged);
+    }
+  }
 
+  function updateSubscription(subscription)
+  {
+    var subscriptionUrl = subscription.url;
+    var knownSubscription = subscriptionsMap[subscriptionUrl];
+    if (knownSubscription)
+    {
+      knownSubscription.disabled = subscription.disabled;
+      knownSubscription.lastDownload = subscription.lastDownload;
+    }
+    else
+    {
+      observeSubscription(subscription);
+      getAcceptableAdsURL(function(acceptableAdsUrl)
+      {
         var collection = null;
         if (subscriptionUrl in recommendationsMap)
         {
@@ -366,6 +486,15 @@
       element = element.parentElement;
     }
 
+    function getParentAccessElement()

[Thomas Greiner, 2016/01/19 11:27:30]

Please avoid declaring functions inside other functions or otherwise they're
being created everytime when the function is called.

[saroyanm, 2016/01/22 09:55:09]

Good point.

+    {
+      var elementCopy = element;
+      while (!elementCopy.dataset.access)

[Thomas Greiner, 2016/01/19 11:27:29]

We can't use `Element.dataset` due to older Safari versions so please use
`Element.getAttribute()`.

[saroyanm, 2016/01/22 09:55:09]

Right, done.

+        elementCopy = elementCopy.parentNode;
+
+      return elementCopy;
+    }
+
     var actions = element.getAttribute("data-action").split(",");
     for (var i = 0; i < actions.length; i++)
     {
@@ -419,18 +548,83 @@
           document.body.setAttribute("data-tab",
             element.getAttribute("data-tab"));
           break;
+        case "update-all-subscriptions":
+          ext.backgroundPage.sendMessage(
+          {
+            type: "subscriptions.updateAll"
+          });
+          break;
+        case "open-context-menu":

[Thomas Greiner, 2016/01/19 11:27:29]

We should improve this mechanism because it requires quite a bit of extra work
to get the UX right.

My suggestion would be to listen to clicks:
- click on subscription title:
  - if context menu hidden: show context menu
  - else: hide context menu
- click outside of context menu: hide context menu
- click on action item in context menu: hide context menu
- click somewhere else in context menu: ignore

That would also be consistent with other context menus (e.g. right-click context
menu).

[saroyanm, 2016/01/22 09:55:09]

Done, but bit hacky, not sure if it's what you mean.

[Thomas Greiner, 2016/01/27 17:16:56]

Better but what about replacing it with "toggle-context-menu"? Because right now
it doesn't close again when clicking on the link or arrow again.

[saroyanm, 2016/01/28 17:00:10]

Done.

+          var listItem = getParentAccessElement();
+          var contextMenu = listItem.querySelector(".content");
+          listItem.classList.add("context");
+
+          function mouseover()
+          {
+            contextMenu.addEventListener("mouseout", mouseout, false);
+            contextMenu.removeEventListener("mouseover", mouseover);
+          }
+          function mouseout(event)
+          {
+            if (event.target.parentElement != contextMenu)
+            {
+              var relatedTarget = event.relatedTarget;
+              if (relatedTarget.parentNode == this || relatedTarget == this)
+                 return;
+              listItem.classList.remove("context");
+              contextMenu.removeEventListener("mouseout", mouseout);
+            }
+          }
+
+          contextMenu.addEventListener("mouseover", mouseover, false);
+          break;
+        case "update-now":
+          ext.backgroundPage.sendMessage(
+          {
+            type: "subscriptions.update",
+            url: getParentAccessElement().dataset.access
+          });
+          break;
+        case "website":
+          ext.backgroundPage.sendMessage(
+          {
+            type: "subscriptions.website",
+            url: getParentAccessElement().dataset.access
+          });
+          break;
+        case "source":
+          window.open(getParentAccessElement().dataset.access);
+          break;
+        case "delete":
+          ext.backgroundPage.sendMessage(
+          {
+            type: "subscriptions.remove",
+            url: getParentAccessElement().dataset.access
+          });
+          break;
       }
     }
   }
 
   function onDOMLoaded()
   {
-    var recommendationTemplate = document.querySelector("#recommend-list-table template");
-    var popularText = ext.i18n.getMessage("options_popular");
-    recommendationTemplate.content.querySelector(".popular").textContent = popularText;
-    var languagesTemplate = document.querySelector("#all-lang-table template");
-    var buttonText = ext.i18n.getMessage("options_button_add");
-    languagesTemplate.content.querySelector(".button-add span").textContent = buttonText;
+    function updateTemplate(template, selector, messageId)

[Thomas Greiner, 2016/01/19 11:27:29]

This functionality belongs into "i18n.js". If elements inside templates are not
retrieved on selecting `[class^="i18n_"]` we need to add that it should also
search for those within `<template>` elements.

[saroyanm, 2016/01/22 09:55:08]

Done.

+    {
+      template.content.querySelector(selector).
+              textContent = ext.i18n.getMessage(messageId);
+    }
+
+    var template = document.querySelector("#recommend-list-table template");
+    updateTemplate(template, ".popular", "options_popular");
+
+    template = document.querySelector("#all-lang-table template");
+    updateTemplate(template, ".button-add span", "options_button_add");
+
+    template = document.querySelector("#blocking-lists-table template");
+    updateTemplate(template, ".update-now", "options_blockingList_update_now");
+    updateTemplate(template, ".website", "options_blockingList_website");
+    updateTemplate(template, ".source", "options_blockingList_source");
+    updateTemplate(template, ".delete", "options_blockingList_delete");
 
     populateLists();
 
@@ -705,14 +899,27 @@
     switch (action)
     {
       case "added":
+        updateSubscription(subscription);
+        updateShareLink();
+
+        var knownSubscription = subscriptionsMap[subscription.url];
+        if (knownSubscription)
+          collections.blockingLists.addItems(knownSubscription);
+        else
+          collections.blockingLists.addItems(subscription);
+        break;
       case "disabled":
         updateSubscription(subscription);
         updateShareLink();
         break;
+      case "updated":
+        updateSubscription(subscription);

[Thomas Greiner, 2016/01/19 11:27:29]

That's not what "updated" is meant to notify you of. In FilterNotifier,
"subscription.updated" is triggered whenever filters in the subscription change.
Modifications to the subscription itself have their own events:
- disabled
- downloadStatus
- homepage
- lastDownload
- title

[saroyanm, 2016/01/22 09:55:10]

Ahh right, thanks for pointing that out. Done.

+        break;
       case "homepage":
         // TODO: NYI
         break;
       case "removed":
+        var knownSubscription = subscriptionsMap[subscription.url];
         getAcceptableAdsURL(function(acceptableAdsUrl)
         {
           if (subscription.url == acceptableAdsUrl)
@@ -722,7 +929,6 @@
           }
           else
           {
-            var knownSubscription = subscriptionsMap[subscription.url];
             if (subscription.url in recommendationsMap)
               knownSubscription.disabled = true;
             else
@@ -733,6 +939,7 @@
           }
           updateShareLink();
         });
+        collections.blockingLists.removeItem(knownSubscription);
         break;
       case "title":
         // TODO: NYI
@@ -821,7 +1028,7 @@
   ext.backgroundPage.sendMessage(
   {
     type: "subscriptions.listen",
-    filter: ["added", "disabled", "homepage", "removed", "title"]
+    filter: ["added", "disabled", "updated", "homepage", "removed", "title"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);