MD History: Refactored design for displaying history information

chrome/browser/resources/md_history/history_list.js

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 Polymer({
-  is: 'history-card-manager',
+  is: 'history-list',
 
   properties: {
-    // An array of objects sorted in reverse chronological order.
-    // Each object has a date and the history items belonging to that date.
-    historyDataByDay_: {
+    // An array of history entries in reverse chronological order.
+    historyData: {
       type: Array,
       value: function() { return []; }
     },
 
-    // The time of access of the last element of historyDataByDay_.
+    // The time in seconds of the last history item in historyData.
     lastVisitedTime: {
       type: Number,
       value: 0
     },
 
     menuOpen: {
       type: Boolean,
       value: false,
       reflectToAttribute: true
     },
@@ skipping 36 matching lines @@
       this.menuIdentifier = e.detail.accessTime;
 
       cr.ui.positionPopupAtPoint(e.detail.x + this.X_OFFSET_, e.detail.y, menu,
           cr.ui.AnchorType.BEFORE);
 
       menu.focus();
     }
   },
 
   /**
-   * Split the newly updated history results into history items sorted via day
-   * accessed.
-   * @param {!Array<!HistoryEntry>} results The new history results.
+   * Adds the newly updated history results into historyData. Adds new fields
+   * for each result.
+   * @param {!Array<!HistoryEntry>} historyResults The new history results.
    */
-  addNewResults: function(results) {
-    if (results.length == 0)
+  addNewResults: function(historyResults) {
+    if (historyResults.length == 0)
       return;
 
-    var dateSortedData = [];
-    var historyItems = [];
+    // Creates a copy of historyResults to prevent accidentally modifying the

[tsergeant, 2016/02/11 02:43:02]

Nit: "modifying the this"

[yingran, 2016/02/12 02:03:05]

Done.

+    // this field.
+    var results = historyResults.slice();
+
     var currentDate = results[0].dateRelativeDay;
+    for (var i = 0; i < results.length; i++) {
+      // Sets the default values for these fields to prevent undefined types.
+      results[i].selected = false;
+      results[i].isLastItem = false;
+      results[i].isFirstItem = false;
 
-    for (var i = 0; i < results.length; i++) {
-      if (!currentDate)
-        continue;
-
-      results[i].selected = false;
       if (results[i].dateRelativeDay != currentDate) {
-        this.appendHistoryData_(currentDate, historyItems);
+        results[i - 1].isLastItem = true;
+        results[i].isFirstItem = true;
         currentDate = results[i].dateRelativeDay;
-        historyItems = [];
       }
-      historyItems.push(results[i]);
+      results[i].needsTimeGap = this.needsTimeGap_(results, i);
     }
 
-    if (currentDate)
-      this.appendHistoryData_(currentDate, historyItems);
+    // If it's the first time we get data, the first item will always be the
+    // first card.
+    if (this.historyData.length == 0)
+      results[0].isFirstItem = true;
 
-    this.lastVisitedTime = historyItems[historyItems.length - 1].time;
+    // Adds results to the beginning of the historyData array.
+    results.unshift('historyData');
+    this.push.apply(this, results);
+
+    this.lastVisitedTime = this.historyData[this.historyData.length - 1].time;
   },
 
   /**
-   * Cycle through each entry in historyDataByDay_ and set all items to be
+   * Cycle through each entry in historyData and set all items to be
    * unselected.
-   * @param {number} overallItemCount The number of items selected.
+   * @param {number} overallItemCount The number of checkboxes selected.
    */
   unselectAllItems: function(overallItemCount) {
-    var historyCardData = this.historyDataByDay_;
-
-    for (var i = 0; i < historyCardData.length; i++) {
-      var items = historyCardData[i].historyItems;
-      for (var j = 0; j < items.length; j++) {
-        if (items[j].selected) {
-          this.set('historyDataByDay_.' + i + '.historyItems.' + j +
-              '.selected', false);
-          overallItemCount--;
-          if (overallItemCount == 0)
-            return;
-        }
+    for (var i = 0; i < this.historyData.length; i++) {
+      if (this.historyData[i].selected) {
+        this.set('historyData.' + i + '.selected', false);
+        overallItemCount--;
+        if (overallItemCount == 0)
+          break;
       }
     }
   },
 
   /**
    * Remove all selected items from the overall array so that they are also
    * removed from view. Make sure that the card length and positioning is
    * updated accordingly.
    * @param {number} overallItemCount The number of items selected.
    */
   removeDeletedHistory: function(overallItemCount) {
-    for (var i = 0; i < this.historyDataByDay_.length; i++) {
-      var items = this.historyDataByDay_[i].historyItems;
-      var itemDeletedFromCard = false;
+    for (var i = this.historyData.length - 1; i >= 0; i--) {
+      if (!this.historyData[i].selected)
+        continue;
 
-      for (var j = items.length - 1; j >= 0; j--) {
-        if (!items[j].selected)
-          continue;
+      // Resets the first history item.

[tsergeant, 2016/02/11 02:43:02]

This is a bit gnarly. It feels very manual and error-prone, and Polymer is meant
to help us with that.

Have you thought about using computed properties to help with this? Something
like

<history-item ...
    is-first-card="[[isFirstCard_(index, item, historyData.length)]]"

I think you should be able to make it so that it will recompute at the right
time and get everything right with less effort.

(I don't want to give you a comment that's going to take half a day to fix when
you have a day left. If you don't get it working fairly quickly, let me know and
we can TODO it instead).

[yingran, 2016/02/12 02:03:05]

Acknowledged.

+      if (this.historyData[i].isFirstItem &&
+          this.historyData[i].dateRelativeDay ==
+          this.historyData[i + 1].dateRelativeDay) {
+        this.set('historyData.' + parseInt(i + 1) + '.isFirstItem', true);
+      }
 
-        this.splice('historyDataByDay_.' + i + '.historyItems', j, 1);
-        itemDeletedFromCard = true;
-        overallItemCount--;
-        if (overallItemCount == 0) {
-          this.removeEmptyCards_();
-          // If the last card has been removed don't try to update its size.
-          if (i < this.historyDataByDay_.length)
-            this.$['infinite-list'].updateSizeForItem(i);
-          return;
-        }
+      // Resets the last history item.
+      if (this.historyData[i].isLastItem &&
+          this.historyData[i].dateRelativeDay ==
+          this.historyData[i - 1].dateRelativeDay) {
+        this.set('historyData.' + parseInt(i - 1) + '.isLastItem', true);
       }
-      if (itemDeletedFromCard)
-        this.$['infinite-list'].updateSizeForItem(i);
-    }
-  },
 
-  /**
-   * If a day has had all the history it contains removed, remove this day from
-   * the array.
-   * @private
-   */
-  removeEmptyCards_: function() {
-    var historyCards = this.historyDataByDay_;
-    for (var i = historyCards.length - 1; i >= 0; i--) {
-      if (historyCards[i].historyItems.length == 0) {
-        this.splice('historyDataByDay_', i, 1);
+      // Makes sure that the time gap separators are preserved.
+      if (this.historyData[i].needsTimeGap && i > 0) {

[tsergeant, 2016/02/11 02:43:02]

Nit: Remove {}

[yingran, 2016/02/12 02:03:05]

Done.

+        this.set('historyData.' + parseInt(i - 1) + '.needsTimeGap', true);
+      }
+
+      // Removes the selected item from historyData.
+      this.splice('historyData', i, 1);
+
+      overallItemCount--;
+      if (overallItemCount == 0) {

[tsergeant, 2016/02/11 02:43:02]

{} here too

[yingran, 2016/02/12 02:03:05]

Done.

+        break;
       }
     }
   },
 
-  /**
-   * Adds the given items into historyDataByDay_. Adds items to the last
-   * existing day if the date matches, creates a new element otherwise.
-   * @param {string} date The date of the history items.
-   * @param {!Array<!HistoryEntry>} historyItems The list of history items for
-   * the current date.
-   * @private
-   */
-  appendHistoryData_: function(date, historyItems) {
-    var lastDay = this.historyDataByDay_.length - 1;
-    if (lastDay > 0 && date == this.historyDataByDay_[lastDay].date) {
-      this.set('historyDataByDay_.' + lastDay + '.historyItems',
-          this.historyDataByDay_[lastDay].historyItems.concat(historyItems));
-    } else {
-      this.push('historyDataByDay_', {
-        date: date,
-        historyItems: historyItems
-      });
-    }
-  },
-
   /**
    * Based on which items are selected, collect an array of the info required
    * for chrome.send('removeHistory', ...).
    * @param {number} count The number of items that are selected.
    * @return {Array<HistoryEntry>} toBeRemoved An array of objects which contain
    * information on which history-items should be deleted.
    */
   getSelectedItems: function(count) {
     var toBeRemoved = [];
-    for (var i = 0; i < this.historyDataByDay_.length; i++) {
-      var items = this.historyDataByDay_[i].historyItems;
-      for (var j = 0; j < items.length; j++) {
-        if (items[j].selected) {
+    for (var i = 0; i < this.historyData.length; i++) {
+      if (this.historyData[i].selected) {
+        toBeRemoved.push({
+          url: this.historyData[i].url,
+          timestamps: this.historyData[i].allTimestamps
+        });
 
-          toBeRemoved.push({
-            url: items[j].url,
-            timestamps: items[j].allTimestamps
-          });
-
-          count--;
-          if (count == 0)
-            return toBeRemoved;
-        }
+        count--;
+        if (count == 0)
+          return toBeRemoved;
       }
     }
-    return toBeRemoved;
   },
 
   /**
    * Called when the card manager is scrolled.
    * @private
    */
   scrollHandler_: function() {
     // Close overflow menu on scroll.
     this.closeMenu();
 
     // Requests the next list of results when the scrollbar is near the bottom
     // of the window.
     var scrollOffset = 10;
     var scrollElem = this.$['infinite-list'];
 
     if (scrollElem.scrollHeight <=
         scrollElem.scrollTop + scrollElem.clientHeight + scrollOffset) {
       chrome.send('queryHistory',
           ['', 0, 0, this.lastVisitedTime, RESULTS_PER_PAGE]);
     }
+  },
+
+  /**
+   * Check whether the time difference between the given history item and the
+   * next one is large enough for a spacer to be required.
+   * @param {Array<HistoryEntry>} results A list of history results.
+   * @param {number} index The index number of the first item being compared.
+   * @return {boolean} Whether or not time gap separator is required.
+   * @private
+   */
+  needsTimeGap_: function(results, index) {
+    var currentItem = results[index];
+    var nextItem = results[index + 1];
+
+    if (index + 1 >= results.length)
+      return false;
+
+    return currentItem.time - nextItem.time > BROWSING_GAP_TIME &&
+      currentItem.dateRelativeDay == nextItem.dateRelativeDay;
   }
 });