Set ImeListView's scrollable range when updating

ash/common/system/chromeos/ime_menu/ime_list_view.cc

 // Copyright 2016 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/chromeos/ime_menu/ime_list_view.h"
 
 #include "ash/common/material_design/material_design_controller.h"
+#include "ash/common/system/tray/fixed_sized_scroll_view.h"
 #include "ash/common/system/tray/hover_highlight_view.h"
 #include "ash/common/system/tray/ime_info.h"
 #include "ash/common/system/tray/system_menu_button.h"
 #include "ash/common/system/tray/system_tray_delegate.h"
 #include "ash/common/system/tray/tray_constants.h"
 #include "ash/common/system/tray/tray_details_view.h"
 #include "ash/common/system/tray/tray_popup_header_button.h"
 #include "ash/common/system/tray/tray_popup_item_style.h"
 #include "ash/common/system/tray/tray_popup_utils.h"
 #include "ash/common/system/tray/tri_view.h"
@@ skipping 227 matching lines @@
   views::Label* label_;
 
   // ToggleButton to toggle keyboard on or off.
   views::ToggleButton* toggle_;
 
   DISALLOW_COPY_AND_ASSIGN(MaterialKeyboardStatusRowView);
 };
 
 ImeListView::ImeListView(SystemTrayItem* owner,
                          bool show_keyboard_toggle,
-                         SingleImeBehavior single_ime_behavior)
-    : TrayDetailsView(owner) {
+                         SingleImeBehavior single_ime_behavior,
+                         const gfx::Range& scrollable_range)
+    : TrayDetailsView(owner), scrollable_range_(scrollable_range) {
   SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
   IMEInfoList list;
   delegate->GetAvailableIMEList(&list);
   IMEPropertyInfoList property_list;
   delegate->GetCurrentIMEProperties(&property_list);
   Update(list, property_list, show_keyboard_toggle, single_ime_behavior);
 }
 
 ImeListView::~ImeListView() {}
 
@@ skipping 20 matching lines @@
   if (show_keyboard_toggle) {
     if (MaterialDesignController::IsSystemTrayMenuMaterial()) {
       PrependMaterialKeyboardStatus();
     } else {
       if (list.size() > 1 || !property_list.empty())
         AddScrollSeparator();
       AppendKeyboardStatus();
     }
   }
 
+  if (MaterialDesignController::IsSystemTrayMenuMaterial() &&
+      scrollable_range_.IsValid()) {
+    scroller()->ClipHeightTo(scrollable_range_.start(),
+                             scrollable_range_.end());
+  }
+
   Layout();

[tdanderson, 2016/12/06 23:38:19]

Would it be possible to move the code from lines 299-303 into
ImeListView::Layout() instead of having it here? That would be more consistent
with how TrayDetailsView::Layout() works (note it contains a similar call to
ClipHeightTo(), probably for the same reason as this CL).

Specifically, here is my idea:

* Have ImeDetailedView::Layout() just call up to TrayDetailsView::Layout().

* Implement ImeListView::Layout() as:

void ImeListView::Layout() {
  Range r = const gfx::Range height_range = GetImeListViewRange();
  scroller()->ClipHeightTo(r);
  TrayDetailsView::Layout();
}

Here the call up to TrayDetailsView::Layout() should be a no-op because you've
already clipped the height. Note you would also need to move
GetImeListViewRange() into this class, or at least make it accessible by this
class.

The advantages (assuming my idea works / makes sense) are that you don't need to
introduce the |scrollable_range_| member or modify the constructor of
ImeListView.

WDYT?

[Azure Wei, 2016/12/07 00:58:33]

GetImeListViewRange() is kind of opt-in menu specific behavior, instead of
adding it to ImeListView and making ImeDetailedView override it again, I add a
new class ImeMenuListView as subclass of ImeListView and then override Layout()
change the height range.

Thus, ImeListView and ImeDetailedView could keep the previous range to avoid
introducing any new bugs. And no need for the new |scrollable_range_| member.

Is that OK? PTAL.

   SchedulePaint();
 }
 
 void ImeListView::ResetImeListView() {
   // Children are removed from the view hierarchy and deleted in Reset().
-  Reset();
+  TrayDetailsView::Reset();

[tdanderson, 2016/12/06 23:38:19]

Adding TrayDetailsView:: shouldn't be necessary here.

[Azure Wei, 2016/12/07 00:58:33]

Reverted.

   material_keyboard_status_view_ = nullptr;
   keyboard_status_ = nullptr;
 }
 
 void ImeListView::AppendIMEList(const IMEInfoList& list) {
   DCHECK(ime_map_.empty());
   for (size_t i = 0; i < list.size(); i++) {
     HoverHighlightView* container =
         new SelectableHoverHighlightView(this, list[i].name, list[i].selected);
     scroll_content()->AddChildView(container);
@@ skipping 98 matching lines @@
 
 void ImeListView::HandleButtonPressed(views::Button* sender,
                                       const ui::Event& event) {
   if (material_keyboard_status_view_ &&
       sender == material_keyboard_status_view_->toggle()) {
     WmShell::Get()->ToggleIgnoreExternalKeyboard();
   }
 }
 
 }  // namespace ash