MD Settings: cr-slider: Make display consistent and clean up.

chrome/browser/resources/settings/device_page/display.js

 // Copyright 2016 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.
 
 /**
  * @fileoverview
  * 'settings-display' is the settings subpage for display settings.
  */
 
 cr.define('settings.display', function() {
@@ skipping 35 matching lines @@
 
     /** @type {!chrome.system.display.DisplayUnitInfo|undefined} */
     selectedDisplay: {type: Object, observer: 'selectedDisplayChanged_'},
 
     /** Id passed to the overscan dialog. */
     overscanDisplayId: {
       type: String,
       notify: true,
     },
 
-    /** Maximum mode index value for slider. */
-    maxModeIndex_: {type: Number, value: 0},
+    /** @private {!Array<number>} Mode index values for slider. */

[dschuyler, 2016/12/22 02:36:35]

This looks like a departure from the common style, which would be:

/**
 * Mode index values for slider. 
 * @private {!Array<number>}
 */

grepping in settings/ found 1323 hits for @private. Only one of those, in
passwords_and_forms_page.js uses the style on line 56 :)

If you do want to change the way we are commenting @private, I'm not against
that. I just thought this might be a whoops or "didn't notice that" kind of
thing.

[stevenjb, 2016/12/22 02:55:25]

It's shorthand, used more commonly when the type isn't needed, but as you
noticed not entirely without precedent. I generally prefer short when it's
clear.

[michaelpg, 2016/12/22 07:31:32]

We must be searching differently, because I found a settings hit (only) in
keyboard.js:

\/\*\*\s@private\s{.*}\s[^*]
+1. I'm surprised we have so little of this; I thought I used it commonly myself
but I guess not? Also a small amount of precedent for the substantially similar
@type annotation:

/** @type {foo} Foo fighters. */

+    modeValues_: Array,
 
-    /** Selected mode index value for slider. */
-    selectedModeIndex_: {type: Number},
+    /** @private Selected mode index value for slider. */
+    selectedModeIndex_: Number,
 
-    /** Immediate selected mode index value for slider. */
-    immediateSelectedModeIndex_: {type: Number, value: 0}
+    /** @private Selected mode index received from chrome. */
+    currentSelectedModeIndex_: Number,
   },
 
   /**
    * Listener for chrome.system.display.onDisplayChanged events.
    * @type {function(void)|undefined}
    * @private
    */
   displayChangedListener_: undefined,
 
   /** @override */
@@ skipping 69 matching lines @@
   getSelectedModeIndex_: function(selectedDisplay) {
     for (var i = 0; i < selectedDisplay.modes.length; ++i) {
       if (selectedDisplay.modes[i].isSelected)
         return i;
     }
     return 0;
   },
 
   /** @private */
   selectedDisplayChanged_: function() {
-    // Set maxModeIndex first so that the slider updates correctly.
-    if (this.selectedDisplay.modes.length == 0) {
-      this.maxModeIndex_ = 0;
+    // Set |modeValues_| before |selectedModeIndex_| so that the slider updates
+    // correctly.
+    let numModes = this.selectedDisplay.modes.length;
+    if (numModes == 0) {
+      this.modeValues_ = [];
       this.selectedModeIndex_ = 0;
       return;
     }
-    this.maxModeIndex_ = this.selectedDisplay.modes.length - 1;
+    this.modeValues_ = Array.from(Array(numModes).keys());

[michaelpg, 2016/12/22 07:31:32]

I think I have a TODO somewhere to add this to cr-slider directly so we can just
say "use values as ticks" instead of doing all this crap. Not that we have to
fix it now.

[stevenjb, 2016/12/22 20:42:18]

You do, and I thought about doing it that way, but in order for it to be robust
it has to be kind of complicated (e.g. we have one slider that starts at 0 and
one that starts at 1, so it needs min and max). Given how few sliders we have
(and this one may go away), it didn't seem worth the bother.

     this.selectedModeIndex_ = this.getSelectedModeIndex_(this.selectedDisplay);
-    this.immediateSelectedModeIndex_ = this.selectedModeIndex_;
   },
 
   /**
    * @param {!Array<!chrome.system.display.DisplayUnitInfo>} displays
    * @return {boolean}
    * @private
    */
   showDisplayTabMenu_: function(displays) {
     return displays.length > 1;
   },
@@ skipping 70 matching lines @@
   enableSetResolution_: function(selectedDisplay) {
     return selectedDisplay.modes.length > 1;
   },
 
   /**
    * @return {string}
    * @private
    */
   getResolutionText_: function() {
     if (this.selectedDisplay.modes.length == 0) {
-      var widthStr = this.selectedDisplay.bounds.width.toString();
-      var heightStr = this.selectedDisplay.bounds.height.toString();
+      let widthStr = this.selectedDisplay.bounds.width.toString();
+      let heightStr = this.selectedDisplay.bounds.height.toString();
       return this.i18n('displayResolutionText', widthStr, heightStr);
     }
-    // immediateSelectedModeIndex_ is bound to paper-slider.immediate-value
-    // which may not be valid, or may not have updated yet when this is called.
-    if (isNaN(this.immediateSelectedModeIndex_) ||
-        this.immediateSelectedModeIndex_ >= this.selectedDisplay.modes.length) {
-      this.immediateSelectedModeIndex_ =
-          this.getSelectedModeIndex_(this.selectedDisplay);
-    }
-    var mode = this.selectedDisplay.modes[this.immediateSelectedModeIndex_];
-    var best =
+    let mode = this.selectedDisplay.modes[this.selectedModeIndex_];
+    let best =
         this.selectedDisplay.isInternal ? mode.uiScale == 1.0 : mode.isNative;
-    var widthStr = mode.width.toString();
-    var heightStr = mode.height.toString();
+    let widthStr = mode.width.toString();
+    let heightStr = mode.height.toString();
     if (best)
       return this.i18n('displayResolutionTextBest', widthStr, heightStr);
     else if (mode.isNative)
       return this.i18n('displayResolutionTextNative', widthStr, heightStr);
     return this.i18n('displayResolutionText', widthStr, heightStr);
   },
 
   /**
    * @param {!{detail: string}} e |e.detail| is the id of the selected display.
    * @private
@@ skipping 32 matching lines @@
 
     /** @type {!chrome.system.display.DisplayProperties} */ var properties = {
       isPrimary: true
     };
     settings.display.systemDisplayApi.setDisplayProperties(
         this.selectedDisplay.id, properties,
         this.setPropertiesCallback_.bind(this));
   },
 
   /**
-   * @param {!{target: !PaperSliderElement}} e
+   * Triggered when the 'change' event for the selected mode slider is
+   * triggered. This only occurs when the value is comitted (i.e. not while
+   * the slider is being dragged).
    * @private
    */
-  onChangeMode_: function(e) {
-    var curIndex = this.selectedModeIndex_;
-    var newIndex = parseInt(e.target.value, 10);
-    if (newIndex == curIndex)
+  onSelectedModeChange_: function() {
+    if (this.currentSelectedModeIndex_ === undefined ||

[michaelpg, 2016/12/22 07:31:32]

since we remove the listener in detached(), do we need to also clear
this.currentSelectedModeIndex_ there for this logic to still work after another
attach()?

[stevenjb, 2016/12/22 20:42:18]

That's probably a good idea. Done.

+        this.currentSelectedModeIndex_ == this.selectedModeIndex_) {
+      // Don't change the selected display mode until we have received an update
+      // from Chrome and the mode differs from the current mode.
       return;
-    assert(newIndex >= 0);
-    assert(newIndex < this.selectedDisplay.modes.length);
+    }
     /** @type {!chrome.system.display.DisplayProperties} */ var properties = {
-      displayMode: this.selectedDisplay.modes[newIndex]
+      displayMode: this.selectedDisplay.modes[this.selectedModeIndex_]
     };
     settings.display.systemDisplayApi.setDisplayProperties(
         this.selectedDisplay.id, properties,
         this.setPropertiesCallback_.bind(this));
   },
 
   /**
    * @param {!Event} event
    * @private
    */
@@ skipping 49 matching lines @@
       displayIds += display.id;
       if (display.isPrimary && !primaryDisplay)
         primaryDisplay = display;
       if (this.selectedDisplay && display.id == this.selectedDisplay.id)
         selectedDisplay = display;
     }
     this.displayIds = displayIds;
     this.primaryDisplayId = (primaryDisplay && primaryDisplay.id) || '';
     this.selectedDisplay = selectedDisplay || primaryDisplay ||
         (this.displays && this.displays[0]);
+    // Save the selected mode index received from Chrome so that we do not
+    // send an unnecessary setDisplayProperties call (which would log an error).
+    this.currentSelectedModeIndex_ =
+        this.getSelectedModeIndex_(this.selectedDisplay);
+
     this.$.displayLayout.updateDisplays(this.displays, this.layouts);
   },
 
   /** @private */
   setPropertiesCallback_: function() {
     if (chrome.runtime.lastError) {
       console.error(
           'setDisplayProperties Error: ' + chrome.runtime.lastError.message);
     }
   },
 });