[NTP4] Auto-deletion of empty apps panes.

chrome/browser/resources/ntp4/page_list_view.js

Index: chrome/browser/resources/ntp4/page_list_view.js
diff --git a/chrome/browser/resources/ntp4/page_list_view.js b/chrome/browser/resources/ntp4/page_list_view.js
index a5065b12ff35217dad3cde04fba1ee714c689a1f..0fa8d640b3c7190a9b6f1046da8d173219bee4db 100644
--- a/chrome/browser/resources/ntp4/page_list_view.js
+++ b/chrome/browser/resources/ntp4/page_list_view.js
@@ -35,7 +35,7 @@ cr.define('ntp4', function() {
     cardSlider: undefined,
 
     /**
-     * The frame div for cardSlider.
+     * The frame div for this.cardSlider.
      * @type {!Element|undefined}
      */
     sliderFrame: undefined,
@@ -144,8 +144,8 @@ cr.define('ntp4', function() {
       if (this.pageSwitcherEnd)
         ntp4.initializePageSwitcher(this.pageSwitcherEnd);
 
-      this.shownPage = templateData['shown_page_type'];
-      this.shownPageIndex = templateData['shown_page_index'];
+      this.shownPage = templateData.shown_page_type;
+      this.shownPageIndex = templateData.shown_page_index;
 
       // Request data on the apps so we can fill them in.
       // Note that this is kicked off asynchronously.  'getAppsCallback' will be
@@ -172,6 +172,16 @@ cr.define('ntp4', function() {
           cr.ui.CardSlider.EventType.CARD_CHANGED,
           this.cardChangedHandler_.bind(this));
 
+      // Handle the end of cards being changed (useful if animated).
+      this.pageList.addEventListener(
+          cr.ui.CardSlider.EventType.CARD_CHANGE_ENDED,
+          this.cardChangeEndedHandler_.bind(this));
+
+      // Handle tiles being removed from tile pages.
+      this.pageList.addEventListener(
+          ntp4.TilePage.EventType.TILE_REMOVED,
+          this.tileRemovedHandler_.bind(this));
+
       // Ensure the slider is resized appropriately with the window
       window.addEventListener('resize', this.onWindowResize_.bind(this));
 
@@ -189,7 +199,7 @@ cr.define('ntp4', function() {
      * @param {string} title The title of the tile page.
      * @param {bool} titleIsEditable If true, the title can be changed.
      * @param {TilePage} opt_refNode Optional reference node to insert in front
-     * of.
+     *     of.
      * When opt_refNode is falsey, |page| will just be appended to the end of
      * the page list.
      */
@@ -197,6 +207,21 @@ cr.define('ntp4', function() {
       // When opt_refNode is falsey, insertBefore acts just like appendChild.
       this.pageList.insertBefore(page, opt_refNode);
 
+      if (!document.documentElement.classList.contains('starting-up')) {
+        // If we're inserting a card in front of the current card, add 1 to the
+        // current index to adjust for the new card in the order.
+        var index = Array.prototype.indexOf.call(this.tilePages, page);
+        if (typeof this.cardSlider.currentCard != 'undefined' &&
+            index <= this.cardSlider.currentCard) {
+          this.cardSlider.currentCard += 1;
+        }
+        if (page instanceof ntp4.AppsPage &&
+            this.shownPage == templateData.apps_page_id &&
+            this.getAppsPageIndex(page) <= this.shownPageIndex) {
+          this.shownPageIndex += 1;
+        }
+      }
+
       // Remember special MostVisitedPage.
       if (typeof ntp4.MostVisitedPage != 'undefined' &&
           page instanceof ntp4.MostVisitedPage) {
@@ -227,15 +252,31 @@ cr.define('ntp4', function() {
      *     the app.
      * @param {boolean} isUninstall True if the app is being uninstalled;
      *     false if the app is being disabled.
+     * @param {boolean} fromPage If the removal was from the current page.
      */
-    appRemoved: function(appData, isUninstall) {
+    appRemoved: function(appData, isUninstall, fromPage) {
       var app = $(appData.id);
       assert(app, 'trying to remove an app that doesn\'t exist');
 
-      if (!isUninstall)
+      if (!isUninstall) {
         app.replaceAppData(appData);
-      else
-        app.remove();
+      } else {
+        // Unset the ID immediately, because the app is already gone. But leave
+        // the tile on the page as it animates out.
+        app.id = '';

[Evan Stade, 2011/12/08 02:34:56]

i think this can go inside the if (fromPage) case?

[Dan Beam, 2011/12/08 04:46:18]

Done.

+        // If the uninstall was from this page, run the blipout animation and
+        // the tile will be removed in TilePage#onContentsAnimationEnd_.
+        // Otherwise delete the tile without auto-deleting the page to avoid
+        // re-deleting the same page (or the page that slid in to take its
+        // place).
+        if (fromPage) {
+          app.classList.add('removing-tile-contents');
+        } else {
+          var tilePage = app.tile.tilePage;
+          tilePage.removeTile(app.tile, false, true);
+          this.removeAppsPageIfEmpty_(tilePage, false, true);
+        }
+      }
     },
 
     /**
@@ -257,19 +298,16 @@ cr.define('ntp4', function() {
       // uninstall.
       while (this.appsPages.length > 0) {
         var page = this.appsPages[0];
-        var dot = page.navigationDot;
 
         this.eventTracker.remove(page);
-        page.tearDown();
-        page.parentNode.removeChild(page);
-        dot.parentNode.removeChild(dot);
+        this.removeTilePageAndDot_(page);
       }
 
       // Get the array of apps and add any special synthesized entries
       var apps = data.apps;
 
       // Get a list of page names
-      var pageNames = data.appPageNames;
+      var pageNames = data.appsPageNames;
 
       function stringListIsEmpty(list) {
         for (var i = 0; i < list.length; i++) {
@@ -338,15 +376,7 @@ cr.define('ntp4', function() {
       }
 
       var pageIndex = appData.page_index || 0;
-
-      if (pageIndex >= this.appsPages.length) {
-        while (pageIndex >= this.appsPages.length) {
-          this.appendTilePage(new ntp4.AppsPage(),
-                              localStrings.getString('appDefaultPageName'),
-                              true);
-        }
-        this.updateSliderCards();
-      }
+      this.ensureAppsPageAtIndex_(pageIndex);
 
       var page = this.appsPages[pageIndex];
       var app = $(appData.id);
@@ -362,15 +392,19 @@ cr.define('ntp4', function() {
      *     applications.
      */
     appsPrefChangedCallback: function(data) {
-      for (var i = 0; i < data.apps.length; ++i) {
+      for (var i = 0; i < data.apps.length; ++i)
         $(data.apps[i].id).appData = data.apps[i];
-      }
 
-      // Set the App dot names. Skip the first dot (Most Visited).
+      // Set the App dot names. Skip dots that don't belong to apps pages.
       var dots = this.dotList.getElementsByClassName('dot');
-      var start = this.mostVisitedPage ? 1 : 0;
-      for (var i = start; i < dots.length; ++i) {
-        dots[i].displayTitle = data.appPageNames[i - start] || '';
+      var firstAppDot = this.getTilePageIndex(this.appsPages[0]);
+      for (var i = 0; i < data.appsPageNames.length; ++i) {
+        // When we drop an app on a navigation dot or new page we first move the
+        // tile (which calls this), *then* save the page name so sometimes we'll
+        // be one short here.
+        var dot = dots[firstAppDot + i];
+        if (dot)
+          dot.displayTitle = data.appsPageNames[i];
       }
     },
 
@@ -379,21 +413,36 @@ cr.define('ntp4', function() {
      * the Slider knows about the new elements.
      */
     updateSliderCards: function() {
-      var pageNo = Math.min(this.cardSlider.currentCard,
-                            this.tilePages.length - 1);
-      this.cardSlider.setCards(Array.prototype.slice.call(this.tilePages),
-                                                          pageNo);
+      var index = -1;
+      var prevPage = this.shownPage;
+      var prevIndex = this.shownPageIndex;
+      var tiles = Array.prototype.slice.call(this.tilePages);
+      // It doesn't hurt to clamp value this every time, even if it's only
+      // applicable to apps. It helps self-heal strange / un-expected data, i.e.

[Evan Stade, 2011/12/08 02:34:56]

strange/unexepected data most likely comes from prefs files. If you mention that
fact it will be more motivating than "if something does something somehow"

[Dan Beam, 2011/12/08 04:46:18]

Done. (changed comment)

+      // going from an apps page to the most visited page directly without
+      // setting this.shownPageIndex = 0 somehow.
+      this.shownPageIndex = Math.max(0, Math.min(this.shownPageIndex,
+                                                 this.appsPages.length - 1));
       switch (this.shownPage) {
-        case templateData['apps_page_id']:
-          this.cardSlider.selectCardByValue(
-              this.appsPages[Math.min(this.shownPageIndex,
-                                      this.appsPages.length - 1)]);
+        case templateData.apps_page_id:
+          index = tiles.indexOf(this.appsPages[this.shownPageIndex]);
           break;
-        case templateData['most_visited_page_id']:
-          if (this.mostVisitedPage)
-            this.cardSlider.selectCardByValue(this.mostVisitedPage);
+        case templateData.most_visited_page_id:
+          index = tiles.indexOf(this.mostVisitedPage);
           break;
       }
+      // If shownPage was saved as a page that's now disabled or the shownPage
+      // doesn't exist any more, index will be -1. Change to the preferred
+      // default page (first apps pane) in this case.
+      if (index < 0) {
+        this.shownPage = templateData.apps_page_id;
+        index = tiles.indexOf(this.appsPages[0]);
+      }
+      // Set the new cards and index.
+      this.cardSlider.setCards(tiles, index);
+      // If we got new page or index, update the pref.
+      if (prevIndex != this.shownPageIndex || prevPage != this.shownPage)

[Evan Stade, 2011/12/08 02:34:56]

you should just always do this. this.shownPageIndex can be out of sync with the
browser's idea of shown page index (e.g. if you click around on a different NTP
after opening this one)

[Dan Beam, 2011/12/08 04:46:18]

Done.  OK, I just noticed it was often doing this more than necessary.

+        chrome.send('pageSelected', [this.shownPage, this.shownPageIndex]);
     },
 
     /**
@@ -403,13 +452,10 @@ cr.define('ntp4', function() {
     enterRearrangeMode: function() {
       var tempPage = new ntp4.AppsPage();
       tempPage.classList.add('temporary');
-      this.appendTilePage(tempPage,
-                          localStrings.getString('appDefaultPageName'),
-                          true);
-      var tempIndex = Array.prototype.indexOf.call(this.tilePages, tempPage);
-      if (this.cardSlider.currentCard >= tempIndex)
-        this.cardSlider.currentCard += 1;
+      var pageName = localStrings.getString('appDefaultPageName');
+      this.appendTilePage(tempPage, pageName, true);
       this.updateSliderCards();
+      this.updatePageSwitchers();
 
       if (ntp4.getCurrentlyDraggingTile().firstChild.canBeRemoved())
         $('footer').classList.add('showing-trash-mode');
@@ -420,18 +466,11 @@ cr.define('ntp4', function() {
      */
     leaveRearrangeMode: function() {
       var tempPage = document.querySelector('.tile-page.temporary');
-      var dot = tempPage.navigationDot;
-      if (!tempPage.tileCount && tempPage != this.cardSlider.currentCardValue) {
-        dot.animateRemove();
-        var tempIndex = Array.prototype.indexOf.call(this.tilePages, tempPage);
-        if (this.cardSlider.currentCard > tempIndex)
-          this.cardSlider.currentCard -= 1;
-        tempPage.parentNode.removeChild(tempPage);
-        this.updateSliderCards();
-      } else {
+      // Either remove a temp page if it's empty or save the page name (as an
+      // app has just been dropped on it or created somehow.

[Evan Stade, 2011/12/08 02:34:56]

close paren

[Dan Beam, 2011/12/08 04:46:18]

Done.  http://xkcd.com/859/

+      if (tempPage && !this.removeAppsPageIfEmpty_(tempPage, true, true)) {
+        this.saveAppsPageName(tempPage, tempPage.navigationDot.displayTitle);
         tempPage.classList.remove('temporary');
-        this.saveAppPageName(tempPage,
-                             localStrings.getString('appDefaultPageName'));
       }
 
       $('footer').classList.remove('showing-trash-mode');
@@ -488,9 +527,10 @@ cr.define('ntp4', function() {
     },
 
     /**
-     * Returns the index of the given page.
-     * @param {AppsPage} page The AppsPage for we wish to find.
-     * @return {number} The index of |page|, or -1 if it is not here.
+     * Returns the index of the given apps page.
+     * @param {AppsPage} page The AppsPage we wish to find.
+     * @return {number} The index of |page| or -1 if it is not in the
+     *    collection.
      */
     getAppsPageIndex: function(page) {
       return Array.prototype.indexOf.call(this.appsPages, page);
@@ -507,16 +547,19 @@ cr.define('ntp4', function() {
       // Don't change shownPage until startup is done (and page changes actually
       // reflect user actions).
       if (!document.documentElement.classList.contains('starting-up')) {
+        var prevPage = this.shownPage;
+        var prevIndex = this.shownPageIndex;
         if (page.classList.contains('apps-page')) {
-          this.shownPage = templateData['apps_page_id'];
+          this.shownPage = templateData.apps_page_id;
           this.shownPageIndex = this.getAppsPageIndex(page);
         } else if (page.classList.contains('most-visited-page')) {
-          this.shownPage = templateData['most_visited_page_id'];
+          this.shownPage = templateData.most_visited_page_id;
           this.shownPageIndex = 0;
         } else {
           console.error('unknown page selected');
         }
-        chrome.send('pageSelected', [this.shownPage, this.shownPageIndex]);
+        if (prevPage != this.shownPage || prevIndex != this.shownPageIndex)

[Dan Beam, 2011/12/08 04:46:18]

estade: I'm assuming you'll want me to remove the conditional around here as
well?

+          chrome.send('pageSelected', [this.shownPage, this.shownPageIndex]);
       }
 
       // Update the active dot
@@ -527,16 +570,78 @@ cr.define('ntp4', function() {
       this.updatePageSwitchers();
     },
 
-    /*
-     * Save the name of an app page.
-     * Store the app page name into the preferences store.
-     * @param {AppsPage} appPage The app page for which we wish to save.
+    /**
+     * Save the name of an apps page.
+     * Store the apps page name into the preferences store.
+     * @param {AppsPage} appsPage The app page for which we wish to save.
      * @param {string} name The name of the page.
      */
-    saveAppPageName: function(appPage, name) {
-      var index = this.getAppsPageIndex(appPage);
+    saveAppsPageName: function(appsPage, name) {
+      var index = this.getAppsPageIndex(appsPage);
       assert(index != -1);
-      chrome.send('saveAppPageName', [name, index]);
+      chrome.send('saveAppsPageName', [name, index]);
+    },
+
+    /**
+     * An Array of callbacks to be called on the next CARD_CHANGE_ENDED event
+     * handled from the cardSlider.
+     * @private
+     */
+    cardChangeEndedCallbacks_: [],
+
+    /**
+     * Handler for CARD_CHANGE_ENDED on cardSlider.
+     * @param {Event} e The CARD_CHANGE_ENDED event.
+     * @private
+     */
+    cardChangeEndedHandler_: function(e) {
+      if (!document.documentElement.classList.contains('starting-up')) {
+        while (this.cardChangeEndedCallbacks_.length > 0)
+          this.cardChangeEndedCallbacks_.shift().call(this, e);
+      }
+    },
+
+    /**
+     * Happens when a tile is removed from a tile page.
+     * @param {Event} e An event dispatched from a tile when it is removed.
+     */
+    tileRemovedHandler_: function(e) {
+      if (e.tilePage instanceof ntp4.AppsPage)
+        this.removeAppsPageIfEmpty_(e.tilePage, e.wasAnimated);
+    },
+
+    /**
+     * Remove an apps page if it now has no tiles (is empty).
+     * @param {AppsPage} appsPage A page to check for emptiness.
+     * @param {boolean=} opt_animate Whether the prospective removal should be
+     *     animated.
+     * @param {boolean=} opt_dontNotify Whether this NTP's AppLauncherHandler
+     *     shouldn't be notified of this pages' prospective removal (default is
+     *     to notify).
+     * @return {boolean} If |appsPage| was removed or not.
+     */
+    removeAppsPageIfEmpty_: function(appsPage, opt_animate, opt_dontNotify) {
+      assert(appsPage instanceof ntp4.AppsPage,
+             '|appsPage| is really an AppsPage');

[Evan Stade, 2011/12/08 02:34:56]

isn't the string an error message? Hence you would say "...is not really..."

[Dan Beam, 2011/12/08 04:46:18]

Done.

+      if (appsPage.tileCount == 0) {
+        var whenDone = function() {

[Evan Stade, 2011/12/08 02:34:56]

some comment about whenDone is necessary

[Dan Beam, 2011/12/08 04:46:18]

Done.

+          if (!opt_dontNotify) {

[Evan Stade, 2011/12/08 02:34:56]

no curlies

[Dan Beam, 2011/12/08 04:46:18]

Done.

+            chrome.send('deleteAppsPage', [this.getAppsPageIndex(appsPage)]);
+          }
+          this.removeTilePageAndDot_(appsPage, opt_animate);
+          this.updateSliderCards();
+          this.updatePageSwitchers();
+        };

[Evan Stade, 2011/12/08 02:34:56]

newline

[Dan Beam, 2011/12/08 04:46:18]

Done.

+        // If currently on the empty apps pane, move off of it before deleting.
+        if (appsPage === this.cardSlider.currentCardValue) {
+          this.cardChangeEndedCallbacks_.push(whenDone);
+          this.moveOffEmptyAppsPage(appsPage, opt_animate);
+        } else {
+          whenDone.call(this);
+        }
+        return true;
+      }
+      return false;

[Evan Stade, 2011/12/08 02:34:56]

if (appsPage.tileCount != 0)
  return false;

...

this way the entire function body isn't indented

[Dan Beam, 2011/12/08 04:46:18]

Done.

     },
 
     /**
@@ -587,7 +692,109 @@ cr.define('ntp4', function() {
       this.cardSlider.selectCard(cardIndex, true);
 
       e.stopPropagation();
-    }
+    },
+
+    /**
+     * Re-order apps on this inactive page when an active NTP gets re-ordered.
+     * @param {Object} data Position hashmap ordered by app id with indices:
+     *    {id => {page_index: ##, app_launch_index: ##}}
+     */
+    appsReordered: function(data) {
+      var pages = Object.keys(data);
+      for (var i = 0; i < pages.length; ++i) {
+        var page = data[pages[i]];
+        var indices = Object.keys(page);
+        for (var j = 0; j < indices.length; ++j) {
+          var app = $(page[indices[j]]);
+          var originalPage = app.tile.tilePage;
+          if (i != this.getAppsPageIndex(originalPage) || j != app.tile.index) {
+            originalPage.removeTile(app.tile, false, true);
+            this.ensureAppsPageAtIndex_(i);
+            this.appsPages[i].addTileAt(app, j);
+          }
+        }
+      }
+      for (var i = 0; i < this.appsPages.length; ++i) {
+        // If a page is now empty, remove it and move backward one in our

[Evan Stade, 2011/12/08 02:34:56]

comments are nice but this code is self-documenting/self-evident enough that I
don't think it needs a comment. The other thing you can do is

forEach removeAppsPageIfEmpty
forEach cleanupDrag

which is a flow that's even a little easier to follow imho

[Dan Beam, 2011/12/08 04:46:18]

Done.
I originally did this but it was annoying as I had to subtract one from i every
time as it is a live list so I just combined the two.  That cool?

+        // array-like object as it is a live NodeList and the length/indice

[Evan Stade, 2011/12/08 02:34:56]

INDICE

[Dan Beam, 2011/12/08 04:46:18]

DONE. (well, removed whole comment...)

+        // change when you update it. Otherwise, make sure the tiles are in the
+        // right positions (just like after a user is done dragging).
+        if (this.removeAppsPageIfEmpty_(this.appsPages[i], false, true))
+          --i;
+        else
+          this.appsPages[i].cleanupDrag();
+      }
+    },
+
+    /**
+     * Ensure there is an apps page at the given |index|. If index is smaller

[Evan Stade, 2011/12/08 02:34:56]

smaller??

[Dan Beam, 2011/12/08 04:46:18]

Done.

+     * than the current number of apps pages, additional apps pages will be
+     * created.
+     * @param {number} index An apps page index to ensure exists.
+     */
+    ensureAppsPageAtIndex_: function(index) {
+      if (index >= this.appsPages.length) {
+        var pageName = localStrings.getString('appDefaultPageName');
+        while (index >= this.appsPages.length)
+          this.appendTilePage(new ntp4.AppsPage(), pageName, true);
+        this.updateSliderCards();
+      }
+    },
+
+    /**
+     * Returns the index of a given tile page.
+     * @param {TilePage} page The TilePage we wish to find.
+     * @return {number} The index of |page| or -1 if it is not in the
+     *    collection.
+     */
+    getTilePageIndex: function(page) {
+      return Array.prototype.indexOf.call(this.tilePages, page);
+    },
+
+    /**
+     * Move to a non-empty page before/while we delete an empty one.
+     * @param {AppsPage} appsPage An empty apps page that we want to move off of
+     *     (probably because it's empty).
+     * @param {boolean=} opt_animate Whether we should animate or not.
+     */
+    moveOffEmptyAppsPage: function(appsPage, opt_animate) {
+      assert(appsPage instanceof ntp4.AppsPage,
+             '|appsPage| is really an AppsPage');
+      // Select the page further toward the end than the empty |appsPage|. If
+      // the empty |appsPage| is at the end, the apps page before it will be
+      // selected.
+      var index = this.getAppsPageIndex(appsPage);
+      assert(index >= 0, '|appsPage| is in this.appsPages');
+      var direction = (index == this.appsPages.length - 1 ? -1 : 1);

[Evan Stade, 2011/12/08 02:34:56]

no ()

[Dan Beam, 2011/12/08 04:46:18]

Done. (but I still don't agree that this is easier to read or safe)

+      this.cardSlider.selectCard(this.cardSlider.currentCard + direction,
+                                 opt_animate);
+    },
+
+    /**
+     * Removes a page and navigation dot (if the navdot exists).
+     * @param {TilePage} page The page to be removed.
+     * @param {boolean=} opt_animate If the removal should be animated.
+     */
+    removeTilePageAndDot_: function(page, opt_animate) {
+      if (!document.documentElement.classList.contains('starting-up')) {
+        // If the card being deleted is before the current card, keep the same
+        // spot in the list by removing 1 from the current card index.
+        var index = Array.prototype.indexOf.call(this.tilePages, page);
+        if (index < this.cardSlider.currentCard) {

[Evan Stade, 2011/12/08 02:34:56]

no curlies

[Dan Beam, 2011/12/08 04:46:18]

Done.

+          this.cardSlider.currentCard -= 1;
+        }
+        // If the page we're about to delete is in front of the current page,
+        // remove 1 from the shown page index.
+        if (page instanceof ntp4.AppsPage &&
+            this.shownPage == templateData.apps_page_id &&
+            this.getAppsPageIndex(page) < this.shownPageIndex) {
+          this.shownPageIndex -= 1;

[Evan Stade, 2011/12/08 02:34:56]

this doesn't seem necessary. Wont the shownPageIndex be updated by
cardSlider.currentCard assignment?

[Dan Beam, 2011/12/08 04:46:18]

cardSlider.currentCard is an ES5 getter but all it's doing it just setting the
private member and not automagically switching anything.  Is this what you were
thinking?  Why do you think this *isn't* necessary?  I'm hesitant to change it
now as everything works, gar.  Let me try.

[Dan Beam, 2011/12/08 07:27:52]

Working on fixing this to be in cardSlider instead like we talked about.

+        }
+      }
+      if (page.navigationDot)
+        page.navigationDot.remove(opt_animate);
+      page.remove();
+    },
   };
 
   return {