Polish "Displays" Options UI for ChromeOS.

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

Index: chrome/browser/resources/options2/chromeos/display_options.js
diff --git a/chrome/browser/resources/options2/chromeos/display_options.js b/chrome/browser/resources/options2/chromeos/display_options.js
index 7df934f57e0bfe42bb5adc3f2472d010e696067b..8617478cae1c7515455895c90766f7961ac50a7e 100644
--- a/chrome/browser/resources/options2/chromeos/display_options.js
+++ b/chrome/browser/resources/options2/chromeos/display_options.js
@@ -7,6 +7,9 @@ cr.define('options', function() {
 
   // 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
@@ -44,15 +47,14 @@ cr.define('options', function() {
     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');
     },
 
@@ -166,30 +168,37 @@ cr.define('options', function() {
       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;
     },
 
@@ -199,14 +208,45 @@ cr.define('options', function() {
      * @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;
+      }
+
+      $('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
      */
@@ -250,8 +290,9 @@ cr.define('options', function() {
         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);
       }
     },
@@ -282,13 +323,19 @@ cr.define('options', function() {
         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';
@@ -312,6 +359,8 @@ cr.define('options', function() {
           break;
         }
 
+        div.appendChild(document.createTextNode(display.name));
+
         this.displays_view_.appendChild(div);
       }
     },
@@ -333,7 +382,9 @@ cr.define('options', function() {
 
       // 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;
@@ -343,6 +394,7 @@ cr.define('options', function() {
         this.layoutMirroringDisplays_();
       else
         this.layoutDisplays_();
+      this.updateSelectedDisplayDescription_();
     },
   };