[Chrome OS MD] Draw a 1px separator between 2 tray items

ash/common/system/tray/tray_background_view.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 "ash/common/system/tray/tray_background_view.h"
 
 #include "ash/common/material_design/material_design_controller.h"
 #include "ash/common/shelf/shelf_constants.h"
 #include "ash/common/shelf/wm_shelf.h"
 #include "ash/common/shelf/wm_shelf_util.h"
 #include "ash/common/shell_window_ids.h"
+#include "ash/common/system/status_area_widget.h"
 #include "ash/common/system/tray/system_tray.h"
 #include "ash/common/system/tray/tray_constants.h"
 #include "ash/common/system/tray/tray_event_filter.h"
 #include "ash/common/wm_lookup.h"
 #include "ash/common/wm_shell.h"
 #include "ash/common/wm_window.h"
 #include "grit/ash_resources.h"
 #include "ui/accessibility/ax_view_state.h"
 #include "ui/base/nine_image_painter_factory.h"
 #include "ui/compositor/layer.h"
 #include "ui/compositor/layer_animation_element.h"
 #include "ui/compositor/scoped_layer_animation_settings.h"
 #include "ui/events/event_constants.h"
 #include "ui/gfx/animation/tween.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_operations.h"
 #include "ui/gfx/nine_image_painter.h"
+#include "ui/gfx/scoped_canvas.h"
 #include "ui/gfx/skia_util.h"
 #include "ui/gfx/transform.h"
 #include "ui/views/background.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/wm/core/window_animations.h"
 
 namespace {
 
 const int kAnimationDurationForPopupMs = 200;
 
@@ skipping 53 matching lines @@
 
   void set_alpha(int alpha) { alpha_ = alpha; }
 
  private:
   WmShelf* GetShelf() const { return tray_background_view_->shelf(); }
 
   void PaintMaterial(gfx::Canvas* canvas, views::View* view) const {
     SkPaint background_paint;
     background_paint.setFlags(SkPaint::kAntiAlias_Flag);
     background_paint.setColor(SkColorSetA(kShelfBaseColor, alpha_));
-    canvas->DrawRoundRect(view->GetLocalBounds(), kTrayRoundedBorderRadius,
-                          background_paint);
+    gfx::Rect bounds;
+
+    // The hit region are padded to the |view| as insets, so they are included
+    // in the local bounds. Remove these regions from view because hit region is
+    // invisible.
+    if (IsHorizontalAlignment(GetShelf()->GetAlignment())) {
+      bounds = gfx::Rect(view->GetLocalBounds().x() + kHitRegionPadding,
+                         view->GetLocalBounds().y(),
+                         view->GetLocalBounds().width() - kHitRegionPadding -
+                             kHitRegionPadding - kSeparatorWidth,
+                         view->GetLocalBounds().height());
+    } else {
+      bounds = gfx::Rect(view->GetLocalBounds().x(),
+                         view->GetLocalBounds().y() + kHitRegionPadding,
+                         view->GetLocalBounds().width(),
+                         view->GetLocalBounds().height() - kHitRegionPadding -
+                             kHitRegionPadding - kSeparatorWidth);
+    }
+    canvas->DrawRoundRect(bounds, kTrayRoundedBorderRadius, background_paint);
 
     if (tray_background_view_->draw_background_as_active()) {
       SkPaint highlight_paint;
       highlight_paint.setFlags(SkPaint::kAntiAlias_Flag);
       highlight_paint.setColor(kShelfButtonActivatedHighlightColor);
-      canvas->DrawRoundRect(view->GetLocalBounds(), kTrayRoundedBorderRadius,
-                            highlight_paint);
+      canvas->DrawRoundRect(bounds, kTrayRoundedBorderRadius, highlight_paint);
     }
   }
 
   void PaintNonMaterial(gfx::Canvas* canvas, views::View* view) const {
     const int kGridSizeForPainter = 9;
     const int kImages[kNumOrientations][kNumStates][kGridSizeForPainter] = {
         {
             // Horizontal
             IMAGE_GRID_HORIZONTAL(IDR_AURA_TRAY_BG_HORIZ),
             IMAGE_GRID_HORIZONTAL(IDR_AURA_TRAY_BG_HORIZ_ONBLACK),
@@ skipping 76 matching lines @@
   if (details.parent == this)
     PreferredSizeChanged();
 }
 
 void TrayBackgroundView::TrayContainer::UpdateLayout() {
   // Adjust the size of status tray dark background by adding additional
   // empty border.
   views::BoxLayout::Orientation orientation =
       IsHorizontalAlignment(alignment_) ? views::BoxLayout::kHorizontal
                                         : views::BoxLayout::kVertical;
-
+  views::BoxLayout* layout;

[yoshiki, 2016/08/18 08:39:08]

nit: I think we don't need to move it to here.

[yiyix, 2016/08/18 20:01:43]

Done.

   if (!ash::MaterialDesignController::IsShelfMaterial()) {
     // Additional padding used to adjust the user-visible size of status tray
     // dark background.
     const int padding = 3;
     SetBorder(views::Border::CreateEmptyBorder(gfx::Insets(padding) + margin_));
   } else {
-    SetBorder(views::Border::CreateEmptyBorder(margin_));
+    // Extend hit region horizontally or vertically depending on shelf
+    // alignment.
+    const gfx::Insets insets(
+        IsHorizontalAlignment(alignment_)
+            ? margin_ + gfx::Insets(0, kHitRegionPadding, 0,
+                                    kHitRegionPadding + kSeparatorWidth)
+            : margin_ + gfx::Insets(kHitRegionPadding, 0,
+                                    kHitRegionPadding + kSeparatorWidth, 0));
+    SetBorder(views::Border::CreateEmptyBorder(insets));

[yoshiki, 2016/08/18 08:39:08]

nit: I think adding |margin_| at last is simpler.

eg.
const gfx::Insets insets(
   ...
   ...);
SetBorder(views::Border::CreateEmptyBorder(margin_ + insets));

[varkha, 2016/08/18 16:33:24]

I think you can conditionally calculate insets and then create and set border in
one place to avoid code bloat.

[yiyix, 2016/08/18 20:01:43]

Done.

   }
 
-  views::BoxLayout* layout = new views::BoxLayout(orientation, 0, 0, 0);
+  layout = new views::BoxLayout(orientation, 0, 0, 0);
   layout->SetDefaultFlex(1);
   views::View::SetLayoutManager(layout);
   PreferredSizeChanged();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // TrayBackgroundView
 
 TrayBackgroundView::TrayBackgroundView(WmShelf* wm_shelf)
     : wm_shelf_(wm_shelf),
       tray_container_(NULL),
       shelf_alignment_(SHELF_ALIGNMENT_BOTTOM),
       background_(NULL),
       draw_background_as_active_(false),
+      is_separator_visible_(false),
       widget_observer_(new TrayWidgetObserver(this)) {
   DCHECK(wm_shelf_);
   set_notify_enter_exit_on_child(true);
 
   tray_container_ = new TrayContainer(shelf_alignment_);
   SetContents(tray_container_);
   tray_event_filter_.reset(new TrayEventFilter);
 
   SetPaintToLayer(true);
   layer()->SetFillsBoundsOpaquely(false);
@@ skipping 63 matching lines @@
     layer()->SetTransform(transform);
   } else {
     // Listen only to the hide animation. As we cannot turn off visibility
     // until the animation is over.
     animation.AddObserver(this);
     animation.SetTweenType(gfx::Tween::EASE_IN);
     layer()->SetOpacity(0.0f);
     layer()->SetVisible(false);
     HideTransformation();
   }
+  // Update the separator visibility accordingly.

[varkha, 2016/08/18 16:33:24]

nit: Please consider dropping this comment.

[yiyix, 2016/08/18 20:01:43]

Done.

+  wm_shelf_->GetStatusAreaWidget()->OnTrayVisibilityChanged(this);
 }
 
 const char* TrayBackgroundView::GetClassName() const {
   return kViewClassName;
 }
 
 void TrayBackgroundView::ChildPreferredSizeChanged(views::View* child) {
   PreferredSizeChanged();
 }
 
@@ skipping 174 matching lines @@
   // Nothing to do here.
 }
 
 void TrayBackgroundView::UpdateShelfItemBackground(int alpha) {
   if (background_) {
     background_->set_alpha(alpha);
     SchedulePaint();
   }
 }
 
+void TrayBackgroundView::SetSeparatorVisibility(bool is_shown) {
+  is_separator_visible_ = is_shown;

[yoshiki, 2016/08/18 08:39:08]

Shouldn't we call SchedulePaint() after setting the flag?

[yiyix, 2016/08/18 20:01:43]

Done.

+}
+
+void TrayBackgroundView::OnPaint(gfx::Canvas* canvas) {
+  if (!MaterialDesignController::IsShelfMaterial() ||
+      shelf()->GetBackgroundType() ==
+          ShelfBackgroundType::SHELF_BACKGROUND_DEFAULT ||
+      !is_separator_visible_) {
+    return;
+  }
+
+  const int height = kTrayItemSize;
+  const int width = kSeparatorWidth;
+  const int x = kTrayItemSize + kHitRegionPadding + kHitRegionPadding;

[yoshiki, 2016/08/18 08:39:08]

Using kTrayItemSize is wrong.

The width (or height if vertical) of the tray may expand if there are multiple
icons in a single tray. Please retrieve the actual size from WebNotificationTray
(=TrayBackgroundView) or it's container.

[yiyix, 2016/08/18 20:01:43]

In material design the tray has a fixed size is 32 by 32. So i used this
knowledge to find the place to draw the separator. I agree that this makes this
the drawing not as flexible and it is the reason that the line is drawn at the
wrong place in notification tray in the arc++ machine when you have 2 or 3
icons. I will fix it.

+  const int y = (GetShelfConstant(SHELF_SIZE) - kTrayItemSize) / 2;
+  gfx::ScopedCanvas scoped_canvas(canvas);
+  const float scale = canvas->UndoDeviceScaleFactor();
+  SkPaint paint;
+  paint.setColor(kSeparatorColor);
+  paint.setAntiAlias(true);
+  const bool horizontal_shelf = IsHorizontalAlignment(shelf()->GetAlignment());
+  const gfx::Rect bounds = horizontal_shelf ? gfx::Rect(x, y, width, height)
+                                            : gfx::Rect(y, x, height, width);
+  gfx::RectF rect(gfx::ScaleRect(gfx::RectF(bounds), scale));
+  canvas->DrawLine(horizontal_shelf ? rect.top_right() : rect.bottom_left(),
+                   rect.bottom_right(), paint);
+}
+
 }  // namespace ash