TabbedPane: refactor away manual layout and selected_tab_index_

ui/views/controls/tabbed_pane/tabbed_pane.cc

 // 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.
 
 #include "ui/views/controls/tabbed_pane/tabbed_pane.h"
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "third_party/skia/include/core/SkPaint.h"
 #include "third_party/skia/include/core/SkPath.h"
 #include "ui/accessibility/ax_view_state.h"
 #include "ui/base/default_style.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/font_list.h"
 #include "ui/native_theme/native_theme.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/tabbed_pane/tabbed_pane_listener.h"
 #include "ui/views/layout/box_layout.h"
+#include "ui/views/layout/fill_layout.h"
 #include "ui/views/layout/layout_manager.h"
 #include "ui/views/widget/widget.h"
 
 namespace {
 
 // TODO(markusheintz|msw): Use NativeTheme colors.
 const SkColor kTabTitleColor_Inactive = SkColorSetRGB(0x64, 0x64, 0x64);
 const SkColor kTabTitleColor_Active = SK_ColorBLACK;
 const SkColor kTabTitleColor_Hovered = SK_ColorBLACK;
 const SkColor kTabBorderColor = SkColorSetRGB(0xC8, 0xC8, 0xC8);
@@ skipping 17 matching lines @@
 class MdTab : public Tab {
  public:
   MdTab(TabbedPane* tabbed_pane, const base::string16& title, View* contents);
   ~MdTab() override;
 
   // Overridden from Tab:
   void OnStateChanged() override;
 
   // Overridden from View:
   void OnPaintBorder(gfx::Canvas* canvas) override;
+  gfx::Size GetPreferredSize() const override;

[msw, 2016/10/12 20:18:35]

nit: order above OnPaintBorder to match base class decl order.

[Elly Fong-Jones, 2016/10/12 21:23:05]

Done.

 
  private:
   DISALLOW_COPY_AND_ASSIGN(MdTab);
 };
 
 // The tab strip shown above the tab contents.
 class TabStrip : public View {
  public:
   // Internal class name.
   static const char kViewClassName[];
 
-  explicit TabStrip(TabbedPane* tabbed_pane);
+  TabStrip();
   ~TabStrip() override;
 
   // Overridden from View:
-  gfx::Size GetPreferredSize() const override;
-  void Layout() override;
   const char* GetClassName() const override;
-  void OnPaint(gfx::Canvas* canvas) override;
+  void OnPaintBorder(gfx::Canvas* canvas) override;
+
+  Tab* GetSelectedTab() const;
+  Tab* GetTabAtDeltaFromSelected(int delta) const;
+  Tab* GetTabAtIndex(int index) const;
+  int GetSelectedTabIndex() const;
 
  private:
-  TabbedPane* tabbed_pane_;
-
   DISALLOW_COPY_AND_ASSIGN(TabStrip);
 };
 
 // A subclass of TabStrip that implements the Harmony visual styling. This
 // class uses a BoxLayout to position tabs.
 class MdTabStrip : public TabStrip {
  public:
-  explicit MdTabStrip(TabbedPane* tabbed_pane);
+  MdTabStrip();
   ~MdTabStrip() override;
 
   // Overridden from View:
-  gfx::Size GetPreferredSize() const override;
-  void Layout() override;
-  void OnPaint(gfx::Canvas* canvas) override;
+  void OnPaintBorder(gfx::Canvas* canvas) override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MdTabStrip);
 };
 
 // static
 const char Tab::kViewClassName[] = "Tab";
 
 Tab::Tab(TabbedPane* tabbed_pane, const base::string16& title, View* contents)
     : tabbed_pane_(tabbed_pane),
       title_(new Label(
           title,
           ui::ResourceBundle::GetSharedInstance().GetFontListWithDelta(
               ui::kLabelFontSizeDelta,
               gfx::Font::NORMAL,
               kActiveWeight))),
       tab_state_(TAB_ACTIVE),
       contents_(contents) {
   // Calculate this now while the font list is guaranteed to be bold.
   preferred_title_size_ = title_->GetPreferredSize();
 
+  SetBorder(Border::CreateEmptyBorder(gfx::Insets(5, 10)));

[msw, 2016/10/12 20:18:35]

nit: please use constants defined here or bonus points for finding shared
ui/views/layout/layout_constants.h or similar.

[Elly Fong-Jones, 2016/10/12 21:23:05]

Done.

+  SetLayoutManager(new FillLayout);
+
   SetState(TAB_INACTIVE);
   AddChildView(title_);
 }
 
 Tab::~Tab() {}
 
 void Tab::SetSelected(bool selected) {
   contents_->SetVisible(selected);
   SetState(selected ? TAB_ACTIVE : TAB_INACTIVE);
 #if defined(OS_MACOSX)
@@ skipping 52 matching lines @@
       SetState(selected() ? TAB_ACTIVE : TAB_INACTIVE);
       break;
     default:
       break;
   }
   event->SetHandled();
 }
 
 gfx::Size Tab::GetPreferredSize() const {
   gfx::Size size(preferred_title_size_);
-  size.Enlarge(21, 9);
-  const int kTabMinWidth = 54;
-  if (size.width() < kTabMinWidth)
-    size.set_width(kTabMinWidth);
+  size.Enlarge(GetInsets().width(), GetInsets().height());
   return size;
 }
 
-void Tab::Layout() {
-  gfx::Rect bounds = GetLocalBounds();
-  bounds.Inset(0, 1, 0, 0);
-  bounds.ClampToCenteredSize(preferred_title_size_);
-  title_->SetBoundsRect(bounds);
-}
-
 const char* Tab::GetClassName() const {
   return kViewClassName;
 }
 
 void Tab::SetState(TabState tab_state) {
   if (tab_state == tab_state_)
     return;
   tab_state_ = tab_state;
   OnStateChanged();
   SchedulePaint();
 }
 
-bool Tab::ContainerHasFocus() {
-  return tabbed_pane_->HasFocus();
-}
-
 void Tab::OnFocus() {
   OnStateChanged();
   // When the tab gains focus, send an accessibility event indicating that the
   // contents are focused. When the tab loses focus, whichever new View ends up
   // with focus will send an AX_EVENT_FOCUS of its own, so there's no need to
   // send one in OnBlur().
   if (contents())
     contents()->NotifyAccessibilityEvent(ui::AX_EVENT_FOCUS, true);
   SchedulePaint();
 }
@@ skipping 65 matching lines @@
 
   // Draw the lighter-colored stroke first, then draw the heavier stroke over
   // the bottom of it. This is fine because the heavier stroke has 1.0 alpha, so
   // the lighter stroke won't show through.
   canvas->DrawRect(bounds, paint);
   canvas->FillRect(
       gfx::Rect(0, height() - kBorderStrokeWidth, width(), kBorderStrokeWidth),
       base_color);
 }
 
+gfx::Size MdTab::GetPreferredSize() const {
+  return gfx::Size(Tab::GetPreferredSize().width(), kHarmonyTabStripTabHeight);
+}
+
 // static
 const char TabStrip::kViewClassName[] = "TabStrip";
 
-TabStrip::TabStrip(TabbedPane* tabbed_pane) : tabbed_pane_(tabbed_pane) {}
+TabStrip::TabStrip() {
+  BoxLayout* layout = new BoxLayout(BoxLayout::kHorizontal, 9, 0, 0);

[msw, 2016/10/12 20:18:35]

ditto nit: 9 -> constant, shared if possible.

[Elly Fong-Jones, 2016/10/12 21:23:05]

Done.

+  layout->set_main_axis_alignment(BoxLayout::MAIN_AXIS_ALIGNMENT_START);
+  layout->set_cross_axis_alignment(BoxLayout::CROSS_AXIS_ALIGNMENT_END);
+  layout->SetDefaultFlex(0);
+  SetLayoutManager(layout);
+}
 
 TabStrip::~TabStrip() {}
 
-gfx::Size TabStrip::GetPreferredSize() const {
-  gfx::Size size;
-  for (int i = 0; i < child_count(); ++i) {
-    const gfx::Size child_size = child_at(i)->GetPreferredSize();
-    size.SetSize(size.width() + child_size.width(),
-                 std::max(size.height(), child_size.height()));
-  }
-  return size;
-}
-
-void TabStrip::Layout() {
-  const int kTabOffset = 9;
-  int x = kTabOffset;  // Layout tabs with an offset to the tabstrip border.
-  for (int i = 0; i < child_count(); ++i) {
-    gfx::Size ps = child_at(i)->GetPreferredSize();
-    child_at(i)->SetBounds(x, 0, ps.width(), ps.height());
-    x = child_at(i)->bounds().right();
-  }
-}
-
 const char* TabStrip::GetClassName() const {
   return kViewClassName;
 }
 
-void TabStrip::OnPaint(gfx::Canvas* canvas) {
-  OnPaintBackground(canvas);
-
-  // Draw the TabStrip border.
+void TabStrip::OnPaintBorder(gfx::Canvas* canvas) {
   SkPaint paint;
   paint.setColor(kTabBorderColor);
   paint.setStrokeWidth(kTabBorderThickness);
   SkScalar line_y = SkIntToScalar(height()) - (kTabBorderThickness / 2);
   SkScalar line_end = SkIntToScalar(width());
-  int selected_tab_index = tabbed_pane_->selected_tab_index();
+  int selected_tab_index = GetSelectedTabIndex();
   if (selected_tab_index >= 0) {
-    Tab* selected_tab = tabbed_pane_->GetTabAt(selected_tab_index);
+    Tab* selected_tab = GetTabAtIndex(selected_tab_index);
     SkPath path;
     SkScalar tab_height =
         SkIntToScalar(selected_tab->height()) - kTabBorderThickness;
     SkScalar tab_width =
         SkIntToScalar(selected_tab->width()) - kTabBorderThickness;
     SkScalar tab_start = SkIntToScalar(selected_tab->GetMirroredX());
     path.moveTo(0, line_y);
     path.rLineTo(tab_start, 0);
     path.rLineTo(0, -tab_height);
     path.rLineTo(tab_width, 0);
     path.rLineTo(0, tab_height);
     path.lineTo(line_end, line_y);
 
     SkPaint paint;
     paint.setStyle(SkPaint::kStroke_Style);
     paint.setColor(kTabBorderColor);
     paint.setStrokeWidth(kTabBorderThickness);
     canvas->DrawPath(path, paint);
   } else {
     canvas->sk_canvas()->drawLine(0, line_y, line_end, line_y, paint);
   }
 }
 
-MdTabStrip::MdTabStrip(TabbedPane* tabbed_pane) : TabStrip(tabbed_pane) {
+Tab* TabStrip::GetTabAtIndex(int index) const {
+  return static_cast<Tab*>(const_cast<View*>(child_at(index)));
+}
+
+int TabStrip::GetSelectedTabIndex() const {
+  for (int i = 0; i < child_count(); ++i)
+    if (GetTabAtIndex(i)->selected())
+      return i;
+  return -1;
+}
+
+Tab* TabStrip::GetSelectedTab() const {
+  int index = GetSelectedTabIndex();
+  return index >= 0 ? GetTabAtIndex(index) : nullptr;
+}
+
+Tab* TabStrip::GetTabAtDeltaFromSelected(int delta) const {
+  int index = (GetSelectedTabIndex() + delta) % child_count();
+  if (index < 0)
+    index += child_count();
+  return GetTabAtIndex(index);
+}
+
+MdTabStrip::MdTabStrip() {
   BoxLayout* layout =
       new BoxLayout(BoxLayout::kHorizontal, 0, kHarmonyTabStripVerticalPad, 0);
   layout->set_main_axis_alignment(BoxLayout::MAIN_AXIS_ALIGNMENT_CENTER);
   layout->set_cross_axis_alignment(BoxLayout::CROSS_AXIS_ALIGNMENT_STRETCH);
   layout->SetDefaultFlex(1);
   SetLayoutManager(layout);
 }
 
 MdTabStrip::~MdTabStrip() {}
 
-gfx::Size MdTabStrip::GetPreferredSize() const {
-  return gfx::Size(width(),
-                   kHarmonyTabStripVerticalPad * 2 + kHarmonyTabStripTabHeight);
-}
-
-// Let this class's LayoutManager handle the layout.
-void MdTabStrip::Layout() {
-  return View::Layout();
-}
-
-// The tab strip "border" is drawn as part of the tabs, so all this method needs
-// to do is paint the background.
-void MdTabStrip::OnPaint(gfx::Canvas* canvas) {
-  OnPaintBackground(canvas);
-}
+// The tab strip "border" is drawn as part of the tabs.
+void MdTabStrip::OnPaintBorder(gfx::Canvas* canvas) { }

[msw, 2016/10/12 20:18:35]

nit: {}

[Elly Fong-Jones, 2016/10/12 21:23:05]

Done.

 
 TabbedPane::TabbedPane()
     : listener_(NULL),
       tab_strip_(ui::MaterialDesignController::IsSecondaryUiMaterial()
-                     ? new MdTabStrip(this)
-                     : new TabStrip(this)),
-      contents_(new View()),
-      selected_tab_index_(-1) {
+                     ? new MdTabStrip
+                     : new TabStrip),
+      contents_(new View()) {
   AddChildView(tab_strip_);
   AddChildView(contents_);
 }
 
 TabbedPane::~TabbedPane() {}
 
+int TabbedPane::GetSelectedTabIndex() const {
+  return tab_strip_->GetSelectedTabIndex();
+}
+
 int TabbedPane::GetTabCount() {
   DCHECK_EQ(tab_strip_->child_count(), contents_->child_count());
   return contents_->child_count();
 }
 
 void TabbedPane::AddTab(const base::string16& title, View* contents) {
   AddTabAtIndex(tab_strip_->child_count(), title, contents);
 }
 
 void TabbedPane::AddTabAtIndex(int index,
                                const base::string16& title,
                                View* contents) {
   DCHECK(index >= 0 && index <= GetTabCount());
   contents->SetVisible(false);
 
   tab_strip_->AddChildViewAt(
       ui::MaterialDesignController::IsSecondaryUiMaterial()
           ? new MdTab(this, title, contents)
           : new Tab(this, title, contents),
       index);
   contents_->AddChildViewAt(contents, index);
-  // TODO(ellyjones): if index < selected_tab_index(), selected_tab_index() gets
-  // out of sync with which tab believes it is selected. This class should
-  // directly ask Tabs whether they are selected.
-  if (selected_tab_index() < 0)
+  if (!GetSelectedTab())
     SelectTabAt(index);
 
   PreferredSizeChanged();
 }
 
-void TabbedPane::SelectTabAt(int index) {
-  DCHECK(index >= 0 && index < GetTabCount());
-  if (index == selected_tab_index())
+void TabbedPane::SelectTab(Tab* new_selected_tab) {
+  Tab* old_selected_tab = tab_strip_->GetSelectedTab();
+  if (old_selected_tab == new_selected_tab)
     return;
 
-  Tab* old_selected_tab = GetSelectedTab();
-  Tab* new_selected_tab = GetTabAt(index);
   new_selected_tab->SetSelected(true);
-  selected_tab_index_ = index;
   if (old_selected_tab) {
     if (old_selected_tab->HasFocus())
       new_selected_tab->RequestFocus();
     old_selected_tab->SetSelected(false);
   }
   tab_strip_->SchedulePaint();
 
   FocusManager* focus_manager = new_selected_tab->contents()->GetFocusManager();
   if (focus_manager) {
     const View* focused_view = focus_manager->GetFocusedView();
     if (focused_view && contents_->Contains(focused_view) &&
         !new_selected_tab->contents()->Contains(focused_view))
       focus_manager->SetFocusedView(new_selected_tab->contents());
   }
 
   if (listener())
-    listener()->TabSelectedAt(index);
+    listener()->TabSelectedAt(tab_strip_->GetIndexOf(new_selected_tab));
 }
 
-void TabbedPane::SelectTab(Tab* tab) {
-  const int index = tab_strip_->GetIndexOf(tab);
-  if (index >= 0)
-    SelectTabAt(index);
+void TabbedPane::SelectTabAt(int index) {
+  Tab* tab = tab_strip_->GetTabAtIndex(index);
+  if (tab)
+    SelectTab(tab);
 }
 
 gfx::Size TabbedPane::GetPreferredSize() const {
   gfx::Size size;
   for (int i = 0; i < contents_->child_count(); ++i)
     size.SetToMax(contents_->child_at(i)->GetPreferredSize());
   size.Enlarge(0, tab_strip_->GetPreferredSize().height());
   return size;
 }
 
-Tab* TabbedPane::GetTabAt(int index) {
-  return static_cast<Tab*>(tab_strip_->child_at(index));
-}
-
 Tab* TabbedPane::GetSelectedTab() {
-  return selected_tab_index() >= 0 ? GetTabAt(selected_tab_index()) : nullptr;
+  return tab_strip_->GetSelectedTab();
 }
 
 bool TabbedPane::MoveSelectionBy(int delta) {
-  const int tab_count = GetTabCount();
-  if (tab_count <= 1)
+  if (contents_->child_count() <= 1)
     return false;
-  int next_selected_index = (selected_tab_index() + delta) % tab_count;
-  if (next_selected_index < 0)
-    next_selected_index += tab_count;
-  SelectTabAt(next_selected_index);
+  SelectTab(tab_strip_->GetTabAtDeltaFromSelected(delta));

[msw, 2016/10/12 20:18:35]

nit: it might make sense to just keep this inlined until its needed elsewhere.

[Elly Fong-Jones, 2016/10/12 21:23:05]

I de-inlined it so TabbedPane could stop dealing with tab indexes as far as
possible, and I think leaving it de-inlined serves that goal better.

   return true;
 }
 
 void TabbedPane::Layout() {
   const gfx::Size size = tab_strip_->GetPreferredSize();
   tab_strip_->SetBounds(0, 0, width(), size.height());
   contents_->SetBounds(0, tab_strip_->bounds().bottom(), width(),
                        std::max(0, height() - size.height()));
   for (int i = 0; i < contents_->child_count(); ++i)
     contents_->child_at(i)->SetSize(contents_->size());
@@ skipping 21 matching lines @@
 
 View* TabbedPane::GetSelectedTabContentView() {
   return GetSelectedTab() ? GetSelectedTab()->contents() : nullptr;
 }
 
 void TabbedPane::GetAccessibleState(ui::AXViewState* state) {
   state->role = ui::AX_ROLE_TAB_LIST;
 }
 
 }  // namespace views