Polish "Displays" Options UI for ChromeOS.

chrome/browser/resources/options2/chromeos/display_options.js

 // Copyright (c) 2012 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.
 
 cr.define('options', function() {
   var OptionsPage = options.OptionsPage;
 
   // The scale ratio of the display rectangle to its original size.
   /** @const */ var VISUAL_SCALE = 1 / 10;
+  // The margin pixels to calculate the arrow position from left.  This happens

[James Hawkins, 2012/08/03 18:08:23]

You use the word 'left' here: does this work in RTL?  You should probably use
the word 'start' instead.

[Jun Mukai, 2012/08/06 08:07:32]

Removed this constant.  According to kuscher's comment in crbug.com/130385#c31,
I put the display rectangles at the center of the area.  Not need to have this
constant.

+  // due to the 'padding' value of $('display-options-displays-view-host').
+  /** @const */ var ARROW_POSITION_MARGIN = 20;
 
   /**
    * Enumeration of secondary display layout.  The value has to be same as the
    * values in ash/monitor/monitor_controller.cc.
    * @enum {number}
    */
   var SecondaryDisplayLayout = {
     TOP: 0,
     RIGHT: 1,
     BOTTOM: 2,
@@ skipping 17 matching lines @@
 
   DisplayOptions.prototype = {
     __proto__: OptionsPage.prototype,
 
     /**
      * Initialize the page.
      */
     initializePage: function() {
       OptionsPage.prototype.initializePage.call(this);
 
-      $('display-options-confirm').onclick = function() {
-        OptionsPage.closeOverlay();
-      };
-
       $('display-options-toggle-mirroring').onclick = (function() {
         this.mirroring_ = !this.mirroring_;
         chrome.send('setMirroring', [this.mirroring_]);
       }).bind(this);
 
+      $('display-options-apply-button').onclick = (function() {
+        chrome.send('setDisplayLayout', [this.layout_]);
+      }).bind(this);
       chrome.send('getDisplayInfo');
     },
 
     /**
      * Mouse move handler for dragging display rectangle.
      * @private
      * @param {Event} e The mouse move event.
      */
     onMouseMove_: function(e) {
       if (!this.dragging_)
@@ skipping 93 matching lines @@
      * @private
      * @param {Event} e The mouse down event.
      */
     onMouseDown_: function(e) {
       if (this.mirroring_)
         return true;
 
       if (e.button != 0)
         return true;
 
-      if (e.target == this.displays_view_)
-        return true;
+      this.focused_index_ = null;
+      for (var i = 0; i < this.displays_.length; i++) {
+        var display = this.displays_[i];
+        if (this.displays_[i].div == e.target) {
+          this.focused_index_ = i;
+          break;
+        } else if (i == 0 && $('display-launcher') == e.target) {
+          this.focused_index_ = i;

[James Hawkins, 2012/08/03 18:08:23]

This is the same functionality as the block above: please move this logic into
the logic above.

[Jun Mukai, 2012/08/06 08:07:32]

Done.

+          break;
+        }
+      }
 
       for (var i = 0; i < this.displays_.length; i++) {
         var display = this.displays_[i];
-        if (display.div == e.target) {
-          // Do not drag the primary monitor.
-          if (i == 0)
-            return true;
+        display.div.className = 'displays-display';
+        if (i != this.focused_index_)
+          continue;
 
-          this.focused_index_ = i;
-          this.dragging_ = {
+        display.div.className += ' displays-focused';
+        // Do not drag the primary monitor.
+        if (i == 0)
+          continue;
+
+        this.dragging_ = {
             display: display,
             click_location: {x: e.offsetX, y: e.offsetY},
             offset: {x: e.pageX - e.offsetX - display.div.offsetLeft,
                      y: e.pageY - e.offsetY - display.div.offsetTop}
-          };
-          if (display.div.className.indexOf('displays-focused') == -1)
-            display.div.className += ' displays-focused';
-        } else if (display.div.className.indexOf('displays-focused') >= 0) {
-          // We can assume that '-primary' monitor doesn't have '-focused'.
-          this.displays_[i].div.className = 'displays-display';
-        }
+        };
       }
+      this.updateSelectedDisplayDescription_();
       return false;
     },
 
     /**
      * Mouse up handler for dragging display rectangle.
      * @private
      * @param {Event} e The mouse up event.
      */
     onMouseUp_: function(e) {
-      if (this.dragging_) {
+      if (this.dragging_)
         this.dragging_ = null;
-        chrome.send('setDisplayLayout', [this.layout_]);
+      this.updateSelectedDisplayDescription_();
+      return false;
+    },
+
+    /**
+     * Update the description of the selected display section.

[James Hawkins, 2012/08/03 18:08:23]

s/Update/Updates.

[Jun Mukai, 2012/08/06 08:07:32]

Done.

+     * @private
+     */
+    updateSelectedDisplayDescription_: function() {
+      if (this.focused_index_ == null) {
+        $('selected-display-data-container').hidden = true;
+        $('display-configuration-arrow').hidden = true;
+        return;
       }
-      return false;
+
+      $('selected-display-data-container').hidden = false;
+      var display = this.displays_[this.focused_index_];
+      var name_element = $('selected-display-name');

[James Hawkins, 2012/08/03 18:08:23]

s/name_element/nameElement/

javaScript case here and elsewhere for variables.

[Jun Mukai, 2012/08/06 08:07:32]

Sorry breaking such a basic rule.  I reviewed the whole file and fixed the
names.

+      while (name_element.childNodes.length > 0)
+        name_element.removeChild(name_element.firstChild);
+      name_element.appendChild(document.createTextNode(display.name));
+
+      var resolution_data = display.width + 'x' + display.height;
+      var resolution_element = $('selected-display-resolution');
+      while (resolution_element.childNodes.length > 0)
+        resolution_element.removeChild(resolution_element.firstChild);
+      resolution_element.appendChild(document.createTextNode(resolution_data));
+
+      var arrow = $('display-configuration-arrow');
+      arrow.hidden = false;
+      arrow.style.top =
+          $('display-configurations').offsetTop - arrow.offsetHeight / 2 + 'px';
+      arrow.style.left = display.div.offsetLeft + display.div.offsetWidth / 2 +

[James Hawkins, 2012/08/03 18:08:23]

RTL

[Jun Mukai, 2012/08/06 08:07:32]

Removed ARROW_POSITION_MARGIN and working fine now on RTL too.

+          ARROW_POSITION_MARGIN - arrow.offsetWidth / 2 + 'px';
     },
 
     /**
      * Clears the drawing area for display rectangles.
      * @private
      */
     resetDisplaysView_: function() {
       var displays_view_host = $('display-options-displays-view-host');
       displays_view_host.removeChild(displays_view_host.firstChild);
       this.displays_view_ = document.createElement('div');
@@ skipping 26 matching lines @@
         height + num_displays * MIRRORING_OFFSET_PIXELS + 'px';
 
       for (var i = 0; i < num_displays; i++) {
         var div = document.createElement('div');
         div.className = 'displays-display';
         div.style.top = i * MIRRORING_OFFSET_PIXELS + 'px';
         div.style.left = i * MIRRORING_OFFSET_PIXELS + 'px';
         div.style.width = width + 'px';
         div.style.height = height + 'px';
         div.style.zIndex = i;
-        if (i == num_displays - 1)
-          div.className += ' displays-primary';
+        // set 'display-mirrored' class for the background display rectangles.
+        if (i != num_displays - 1)
+          div.className += ' display-mirrored';
         this.displays_view_.appendChild(div);
       }
     },
 
     /**
      * Layouts the display rectangles according to the current layout_.
      * @private
      */
     layoutDisplays_: function() {
       var total_size = {width: 0, height: 0};
@@ skipping 10 matching lines @@
         base_point.x = total_size.width;
       else if (this.layout_ == SecondaryDisplayLayout.TOP)
         base_point.y = total_size.height;
 
       for (var i = 0; i < this.displays_.length; i++) {
         var display = this.displays_[i];
         var div = document.createElement('div');
         display.div = div;
 
         div.className = 'displays-display';
-        if (i == 0)
-          div.className += ' displays-primary';
-        else if (i == this.focused_index_)
+        if (i == this.focused_index_)
           div.className += ' displays-focused';
         div.style.width = display.width * VISUAL_SCALE + 'px';
         div.style.height = display.height * VISUAL_SCALE + 'px';
         div.style.lineHeight = div.style.height;
+        if (i == 0) {
+          // We assume that first display is primary and put a grey rectangle to

[James Hawkins, 2012/08/03 18:08:23]

nit: Don't use pronouns (we) in comments; pronouns are ambiguous.

[Jun Mukai, 2012/08/06 08:07:32]

Done.

+          // denote launcher below.
+          var launcher = document.createElement('div');
+          launcher.id = 'display-launcher';
+          launcher.style.width = display.div.style.width;
+          div.appendChild(launcher);
+        }
         switch (this.layout_) {
         case SecondaryDisplayLayout.RIGHT:
           display.div.style.top = '0';
           display.div.style.left = base_point.x + 'px';
           base_point.x += display.width * VISUAL_SCALE;
           break;
         case SecondaryDisplayLayout.LEFT:
           display.div.style.top = '0';
           base_point.x -= display.width * VISUAL_SCALE;
           display.div.style.left = base_point.x + 'px';
           break;
         case SecondaryDisplayLayout.TOP:
           display.div.style.left = '0';
           base_point.y -= display.height * VISUAL_SCALE;
           display.div.style.top = base_point.y + 'px';
           break;
         case SecondaryDisplayLayout.BOTTOM:
           display.div.style.left = '0';
           display.div.style.top = base_point.y + 'px';
           base_point.y += display.height * VISUAL_SCALE;
           break;
         }
 
+        div.appendChild(document.createTextNode(display.name));
+
         this.displays_view_.appendChild(div);
       }
     },
 
     /**
      * Called when the display arrangement has changed.
      * @private
      * @param {boolean} mirroring Whether current mode is mirroring or not.
      * @param {Array} displays The list of the display information.
      * @param {SecondaryDisplayLayout} layout The layout strategy.
      */
     onDisplayChanged_: function(mirroring, displays, layout) {
       this.mirroring_ = mirroring;
       this.layout_ = layout;
 
       $('display-options-toggle-mirroring').textContent =
           loadTimeData.getString(
               this.mirroring_ ? 'stopMirroring' : 'startMirroring');
 
       // Focus to the first display next to the primary one when |displays| list
       // is updated.
-      if (this.displays_.length != displays.length)
+      if (this.mirroring_)
+        this.focused_index_ = null;
+      else if (this.displays_.length != displays.length)
         this.focused_index_ = 1;
 
       this.displays_ = displays;
 
       this.resetDisplaysView_();
       if (this.mirroring_)
         this.layoutMirroringDisplays_();
       else
         this.layoutDisplays_();
+      this.updateSelectedDisplayDescription_();
     },
   };
 
   DisplayOptions.setDisplayInfo = function(mirroring, displays, layout) {
     DisplayOptions.getInstance().onDisplayChanged_(mirroring, displays, layout);
   };
 
   // Export
   return {
     DisplayOptions: DisplayOptions
   };
 });