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..0faf4dfadaa2b8cced51c0510f9c148b4b447563 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)

[flackr, 2014/04/29 15:58:28]

Move to end after always included files.

[jonross, 2014/04/30 15:50:51]

Done.

+#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_(-1) {
+  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);
 

[flackr, 2014/04/29 15:58:28]

nit: Probably don't need blank lines between all of these button
initializations.

[jonross, 2014/04/30 15:50:51]

Done.

   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);

[flackr, 2014/04/29 15:58:28]

Should probably call RemoveWindowButtonOrderObserver, also maybe move this add
observer to the constructor so that they're guaranteed to always be called.

[jonross, 2014/04/30 15:50:51]

Done.

+#endif
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -240,7 +252,19 @@ gfx::Size CustomFrameView::GetMaximumSize() {
 }
 
 ///////////////////////////////////////////////////////////////////////////////
-// CustomFrameView, ButtonListener implementation:
+// CustomFrameView, WindowButtonOrderObserver:
+
+#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
+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;
+}
+#endif
+
+///////////////////////////////////////////////////////////////////////////////
+//// CustomFrameView, ButtonListener implementation:

[sadrul, 2014/04/30 15:46:54]

Two //

 
 void CustomFrameView::ButtonPressed(Button* sender, const ui::Event& event) {
   if (sender == close_button_)
@@ -275,7 +299,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 +337,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,71 +491,59 @@ const gfx::ImageSkia* CustomFrameView::GetFrameImage() const {
       frame_->IsActive() ? IDR_FRAME : IDR_FRAME_INACTIVE).ToImageSkia();
 }
 
+void CustomFrameView::LayoutButton(ImageButton* button, int x, int y) {
+  button->SetVisible(true);
+  button->SetImageAlignment(ImageButton::ALIGN_LEFT,
+                            ImageButton::ALIGN_BOTTOM);
+  gfx::Size button_size = button->GetPreferredSize();
+  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) {
+    button = GetImageButton(*it);
+    if (!button)
+      continue;
+    LayoutButton(button, next_button_x, caption_y);
+    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) {
+    button = GetImageButton(*it);
+    if (!button)
+      continue;
+    gfx::Size button_size = button->GetPreferredSize();
+    next_button_x -= button_size.width();
+    LayoutButton(button, next_button_x, caption_y);
+    next_button_x = button->x();
+    maximum_title_bar_x_ = next_button_x;
+  }
 }
 
 void CustomFrameView::LayoutTitleBar() {
+  DCHECK(maximum_title_bar_x_ != -1);

[flackr, 2014/04/29 15:58:28]

How about DCHECK_GE(maximum_title_bar_x_, 0);

[jonross, 2014/04/30 15:50:51]

Done.

   // The window title position is calculated based on the icon position, even
   // when there is no icon.
   gfx::Rect icon_bounds(IconBounds());
@@ -549,7 +562,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 -
       title_x), title_height);
 }
 
@@ -584,4 +597,28 @@ ImageButton* CustomFrameView::InitWindowCaptionButton(
   return button;
 }
 
+ImageButton* CustomFrameView::GetImageButton(views::FrameButton frame_button) {
+  ImageButton* button = NULL;
+  switch (frame_button) {
+    case views::FRAME_BUTTON_MINIMIZE: {
+      button = minimize_button_;
+      break;
+    }
+    case views::FRAME_BUTTON_MAXIMIZE: {
+      bool is_restored = !frame_->IsMaximized() && !frame_->IsMinimized();
+      button = is_restored ? maximize_button_ : restore_button_;
+      if (!should_show_maximize_button_) {
+        button->SetVisible(false);

[flackr, 2014/04/29 15:58:28]

Why change the button visibility when returning NULL? Shouldn't only buttons
which are returned be in the frame?

[jonross, 2014/04/30 15:50:51]

Added a comment. Basically we want this button not visible. So we don't want it
toggled to visible, or to be laid out.

Adapted from the original static layout

On 2014/04/29 15:58:28, flackr wrote:

+        return NULL;
+      }
+      break;
+    }
+    case views::FRAME_BUTTON_CLOSE: {
+      button = close_button_;
+      break;
+    }
+  }
+  return button;
+}
+
 }  // namespace views