Linux Aura Task Manager Frame Buttons Misaligned

ui/views/window/custom_frame_view.cc

Index: ui/views/window/custom_frame_view.cc
diff --git a/ui/views/window/custom_frame_view.cc b/ui/views/window/custom_frame_view.cc
index 9896b0f9f7b527dac3a9c3ab7ed099e7c0c25b4f..06383154eb9f27ff35b3a41d2f2cd8a96e82b88d 100644
--- a/ui/views/window/custom_frame_view.cc
+++ b/ui/views/window/custom_frame_view.cc
@@ -18,6 +18,11 @@
 #include "ui/gfx/path.h"
 #include "ui/views/color_constants.h"
 #include "ui/views/controls/button/image_button.h"
+
+#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
+#include "ui/views/linux_ui/linux_ui.h"
+#endif
+
 #include "ui/views/views_delegate.h"
 #include "ui/views/widget/native_widget_aura.h"
 #include "ui/views/widget/widget.h"
@@ -80,7 +85,12 @@ CustomFrameView::CustomFrameView()
       restore_button_(NULL),
       close_button_(NULL),
       should_show_maximize_button_(false),
-      frame_background_(new FrameBackground()) {
+      frame_background_(new FrameBackground()),
+      minimum_title_bar_x_(0),
+      maximum_title_bar_x_(INT_MAX) {
+  trailing_buttons_.push_back(views::FRAME_BUTTON_MINIMIZE);
+  trailing_buttons_.push_back(views::FRAME_BUTTON_MAXIMIZE);
+  trailing_buttons_.push_back(views::FRAME_BUTTON_CLOSE);
 }
 
 CustomFrameView::~CustomFrameView() {
@@ -89,12 +99,8 @@ CustomFrameView::~CustomFrameView() {
 void CustomFrameView::Init(Widget* frame) {
   frame_ = frame;
 
-  close_button_ = new ImageButton(this);
-  close_button_->SetAccessibleName(
-      l10n_util::GetStringUTF16(IDS_APP_ACCNAME_CLOSE));
-
-  // Close button images will be set in LayoutWindowControls().
-  AddChildView(close_button_);
+  close_button_ = InitWindowCaptionButton(IDS_APP_ACCNAME_CLOSE,
+      IDR_CLOSE, IDR_CLOSE_H, IDR_CLOSE_P);
 
   minimize_button_ = InitWindowCaptionButton(IDS_APP_ACCNAME_MINIMIZE,
       IDR_MINIMIZE, IDR_MINIMIZE_H, IDR_MINIMIZE_P);
@@ -111,6 +117,12 @@ void CustomFrameView::Init(Widget* frame) {
     window_icon_ = new ImageButton(this);
     AddChildView(window_icon_);
   }
+
+#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
+  views::LinuxUI* ui = views::LinuxUI::instance();
+  if (ui)
+    ui->AddWindowButtonOrderObserver(this);
+#endif
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -254,6 +266,16 @@ void CustomFrameView::ButtonPressed(Button* sender, const ui::Event& event) {
 }
 
 ///////////////////////////////////////////////////////////////////////////////
+// CustomFrameView, WindowButtonOrderObserver:
+
+void CustomFrameView::OnWindowButtonOrderingChange(
+    const std::vector<views::FrameButton>& leading_buttons,
+    const std::vector<views::FrameButton>& trailing_buttons) {
+  leading_buttons_ = leading_buttons;
+  trailing_buttons_ = trailing_buttons;
+}
+
+///////////////////////////////////////////////////////////////////////////////
 // CustomFrameView, private:
 
 int CustomFrameView::FrameBorderThickness() const {
@@ -275,7 +297,7 @@ int CustomFrameView::NonClientTopBorderHeight() const {
 int CustomFrameView::CaptionButtonY() const {
   // Maximized buttons start at window top so that even if their images aren't
   // drawn flush with the screen edge, they still obey Fitts' Law.
-  return frame_->IsMaximized() ? FrameBorderThickness() : kFrameShadowThickness;
+  return frame_->IsMaximized() ? FrameBorderThickness() : kFrameBorderThickness;
 }
 
 int CustomFrameView::TitlebarBottomThickness() const {
@@ -313,7 +335,8 @@ gfx::Rect CustomFrameView::IconBounds() const {
   // 3D edge (or nothing at all, for maximized windows) above; hence the +1.
   int y = unavailable_px_at_top + (NonClientTopBorderHeight() -
       unavailable_px_at_top - size - TitlebarBottomThickness() + 1) / 2;
-  return gfx::Rect(frame_thickness + kIconLeftSpacing, y, size, size);
+  return gfx::Rect(frame_thickness + kIconLeftSpacing + minimum_title_bar_x_,
+                   y, size, size);
 }
 
 bool CustomFrameView::ShouldShowTitleBarAndBorder() const {
@@ -466,68 +489,88 @@ const gfx::ImageSkia* CustomFrameView::GetFrameImage() const {
       frame_->IsActive() ? IDR_FRAME : IDR_FRAME_INACTIVE).ToImageSkia();
 }
 
+void CustomFrameView::LayoutButton(ImageButton* button, int x, int y,
+    bool leading) {

[flackr, 2014/04/22 18:36:45]

I don't think this wrapping conforms to the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi....

Should be wrapped to open bracket or all arguments wrapped.

[jonross, 2014/04/23 18:53:53]

Done.

+  button->SetVisible(true);
+  button->SetImageAlignment(ImageButton::ALIGN_LEFT,
+                            ImageButton::ALIGN_BOTTOM);
+  gfx::Size button_size = button->GetPreferredSize();
+
+  if (!leading)
+    x -= button_size.width();
+
+  button->SetBounds(x, y, button_size.width(), button_size.height());
+}
+
 void CustomFrameView::LayoutWindowControls() {
-  close_button_->SetImageAlignment(ImageButton::ALIGN_LEFT,
-                                   ImageButton::ALIGN_BOTTOM);
+  minimum_title_bar_x_ = 0;
+  maximum_title_bar_x_ = width();
   int caption_y = CaptionButtonY();
   bool is_maximized = frame_->IsMaximized();
   // There should always be the same number of non-shadow pixels visible to the
-  // side of the caption buttons.  In maximized mode we extend the rightmost
-  // button to the screen corner to obey Fitts' Law.
-  int right_extra_width = is_maximized ?
+  // side of the caption buttons.  In maximized mode we extend the edge button
+  // to the screen corner to obey Fitts' Law.
+  int extra_width = is_maximized ?
       (kFrameBorderThickness - kFrameShadowThickness) : 0;
-  gfx::Size close_button_size = close_button_->GetPreferredSize();
-  close_button_->SetBounds(width() - FrameBorderThickness() -
-      right_extra_width - close_button_size.width(), caption_y,
-      close_button_size.width() + right_extra_width,
-      close_button_size.height());
-
-  // When the window is restored, we show a maximized button; otherwise, we show
-  // a restore button.
+  int next_button_x = FrameBorderThickness() + extra_width;
+
   bool is_restored = !is_maximized && !frame_->IsMinimized();
   ImageButton* invisible_button = is_restored ? restore_button_
                                               : maximize_button_;
   invisible_button->SetVisible(false);
 
-  ImageButton* visible_button = is_restored ? maximize_button_
-                                            : restore_button_;
-  FramePartImage normal_part, hot_part, pushed_part;
-  int next_button_x;
-  if (should_show_maximize_button_) {
-    visible_button->SetVisible(true);
-    visible_button->SetImageAlignment(ImageButton::ALIGN_LEFT,
-                                      ImageButton::ALIGN_BOTTOM);
-    gfx::Size visible_button_size = visible_button->GetPreferredSize();
-    visible_button->SetBounds(close_button_->x() - visible_button_size.width(),
-                              caption_y, visible_button_size.width(),
-                              visible_button_size.height());
-    next_button_x = visible_button->x();
-  } else {
-    visible_button->SetVisible(false);
-    next_button_x = close_button_->x();
+  ImageButton* button = NULL;
+  for (std::vector<views::FrameButton>::const_iterator it =
+           leading_buttons_.begin(); it != leading_buttons_.end(); ++it) {
+    switch (*it) {
+      case views::FRAME_BUTTON_MINIMIZE: {
+        button = minimize_button_;

[flackr, 2014/04/22 18:36:45]

What if the button order specifies multiple of the same button? Can this happen?

[jonross, 2014/04/23 18:53:53]

Currently the last specified instance will determine the position of the button.
The offsets then used for the title bar text will be incorrect. However only
three buttons will ever be shown.

The logic for loading the config is:  GConfListener::ParseAndStoreButtonValue

It provides no guarantees that the config file doesn't specify duplicates.
However other handling code assumes that there are none.

+        break;
+      }
+      case views::FRAME_BUTTON_MAXIMIZE: {
+        button = is_restored ? maximize_button_ : restore_button_;
+        if (!should_show_maximize_button_) {
+          button->SetVisible(false);
+          continue;
+        }
+        break;
+      }
+      case views::FRAME_BUTTON_CLOSE: {
+        button = close_button_;
+        break;
+      }
+    }
+    LayoutButton(button, next_button_x, caption_y, true);
+    next_button_x += button->width();
+    minimum_title_bar_x_ = next_button_x;
   }
 
-  minimize_button_->SetVisible(true);
-  minimize_button_->SetImageAlignment(ImageButton::ALIGN_LEFT,
-                                      ImageButton::ALIGN_BOTTOM);
-  gfx::Size minimize_button_size = minimize_button_->GetPreferredSize();
-  minimize_button_->SetBounds(
-      next_button_x - minimize_button_size.width(), caption_y,
-      minimize_button_size.width(),
-      minimize_button_size.height());
-
-  normal_part = IDR_CLOSE;
-  hot_part = IDR_CLOSE_H;
-  pushed_part = IDR_CLOSE_P;
-
-  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-
-  close_button_->SetImage(CustomButton::STATE_NORMAL,
-                          rb.GetImageNamed(normal_part).ToImageSkia());
-  close_button_->SetImage(CustomButton::STATE_HOVERED,
-                          rb.GetImageNamed(hot_part).ToImageSkia());
-  close_button_->SetImage(CustomButton::STATE_PRESSED,
-                          rb.GetImageNamed(pushed_part).ToImageSkia());
+  // Trailing buttions are laid out in a RTL fashion
+  next_button_x = width() - FrameBorderThickness() - extra_width;
+  for (std::vector<views::FrameButton>::const_reverse_iterator it =
+           trailing_buttons_.rbegin(); it != trailing_buttons_.rend(); ++it) {
+    switch (*it) {

[flackr, 2014/04/22 18:36:45]

This duplication should be avoided. Maybe a common function to get the button?

[jonross, 2014/04/23 18:53:53]

Done.

+      case views::FRAME_BUTTON_MINIMIZE: {
+        button = minimize_button_;
+        break;
+      }
+      case views::FRAME_BUTTON_MAXIMIZE: {
+        button = is_restored ? maximize_button_ : restore_button_;
+        if (!should_show_maximize_button_) {
+          button->SetVisible(false);
+          continue;
+        }
+        break;
+      }
+      case views::FRAME_BUTTON_CLOSE: {
+        button = close_button_;
+        break;
+      }
+    }
+    LayoutButton(button, next_button_x, caption_y, false);
+    next_button_x = button->x();
+    maximum_title_bar_x_ = next_button_x;
+  }
 }
 
 void CustomFrameView::LayoutTitleBar() {
@@ -549,7 +592,7 @@ void CustomFrameView::LayoutTitleBar() {
   // title from overlapping the 3D edge at the bottom of the titlebar.
   title_bounds_.SetRect(title_x,
       icon_bounds.y() + ((icon_bounds.height() - title_height - 1) / 2),
-      std::max(0, minimize_button_->x() - kTitleCaptionSpacing -
+      std::max(0, maximum_title_bar_x_ - kTitleCaptionSpacing -

[flackr, 2014/04/22 18:36:45]

Seems like it's very important that this is called after LayoutWindowControls,
is this guaranteed? This layout order dependency should be documented in the
header.

[jonross, 2014/04/23 18:53:53]

Not guaranteed by API. But it is private, and currently only called from Layout
where we force the order. I've added comments to the docs about this.

       title_x), title_height);
 }