Scroll to make the selected IME visible.

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/hover_highlight_view.h"
 #include "ash/common/system/tray/ime_info.h"
 #include "ash/common/system/tray/system_menu_button.h"
@@ skipping 244 matching lines @@
   views::ToggleButton* toggle_;
 
   DISALLOW_COPY_AND_ASSIGN(MaterialKeyboardStatusRowView);
 };
 
 ImeListView::ImeListView(SystemTrayItem* owner,
                          bool show_keyboard_toggle,
                          SingleImeBehavior single_ime_behavior)
     : TrayDetailsView(owner),
       last_item_selected_with_keyboard_(false),
-      should_focus_ime_after_selection_with_keyboard_(false) {
+      should_focus_ime_after_selection_with_keyboard_(false),
+      current_ime_view_(nullptr) {
   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() {}
 
 void ImeListView::Update(const IMEInfoList& list,
                          const IMEPropertyInfoList& property_list,
                          bool show_keyboard_toggle,
                          SingleImeBehavior single_ime_behavior) {
   ResetImeListView();
   ime_map_.clear();
   property_map_.clear();
   CreateScrollableList();
+  current_ime_view_ = nullptr;

[tdanderson, 2017/01/12 00:16:14]

Should this be placed in ResetImeListView() instead?

[Azure Wei, 2017/01/12 01:20:54]

Moved to ResetImeListView(). Thanks.

 
   // Appends IME list and IME properties.
   if (single_ime_behavior == ImeListView::SHOW_SINGLE_IME || list.size() > 1) {
     if (MaterialDesignController::IsSystemTrayMenuMaterial()) {
       AppendImeListAndProperties(list, property_list);
     } else {
       AppendIMEList(list);
       if (!property_list.empty())
         AppendIMEProperties(property_list);
     }
   }
 
   if (show_keyboard_toggle) {
     if (MaterialDesignController::IsSystemTrayMenuMaterial()) {
       PrependMaterialKeyboardStatus();
     } else {
       if (list.size() > 1 || !property_list.empty())
         AddScrollSeparator();
       AppendKeyboardStatus();
     }
   }
 
   Layout();
   SchedulePaint();
 
   if (should_focus_ime_after_selection_with_keyboard_ &&
       last_item_selected_with_keyboard_)
     FocusCurrentImeIfNeeded();
+  else if (current_ime_view_ != nullptr)

[tdanderson, 2017/01/12 00:16:15]

I don't think I understand why this is an 'else if' instead of an 'if'. Don't
you want to execute line 315 even if
(should_focus_ime_after_selection_with_keyboard_ &&
last_item_selected_with_keyboard_) is true?

Similarly, on line 458 I don't think I understand the need to early return if
the middle clause is true (should_focus_ime_after_selection_with_keyboard_ &&
last_item_selected_with_keyboard_). 

Can you please explain?

also, nit: just "if (current_ime_view_)" should be sufficient here

[Azure Wei, 2017/01/12 01:20:54]

If (should_focus_ime_after_selection_with_keyboard_ &&
last_item_selected_with_keyboard_), we need to focus to last item selected with
keyboard, which could be IME property. Some IME have more than 5 properties
(e.g. Japanese Input), thus the current IME and the selected property cannot be
shown at the same time.  

[tdanderson, 2017/01/12 21:27:33]

Acknowledged.

+    scroll_content()->ScrollRectToVisible(current_ime_view_->bounds());

[tdanderson, 2017/01/12 00:16:14]

please use {} after the if and else-if clauses since the 'if' boolean expression
spans more than one line

[Azure Wei, 2017/01/12 01:20:54]

Done.

 }
 
 void ImeListView::ResetImeListView() {
   // Children are removed from the view hierarchy and deleted in Reset().
   Reset();
   material_keyboard_status_view_ = nullptr;
   keyboard_status_ = nullptr;
 }
 
 void ImeListView::CloseImeListView() {
   last_selected_item_id_.clear();
+  current_ime_view_ = nullptr;
   last_item_selected_with_keyboard_ = false;
   GetWidget()->Close();
 }
 
 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 19 matching lines @@
     const IMEInfoList& list,
     const IMEPropertyInfoList& property_list) {
   DCHECK(ime_map_.empty());
   for (size_t i = 0; i < list.size(); i++) {
     views::View* ime_view =
         new ImeListItemView(owner(), this, list[i].short_name, list[i].name,
                             list[i].selected, gfx::kGoogleGreen700);
     scroll_content()->AddChildView(ime_view);
     ime_map_[ime_view] = list[i].id;
 
+    if (list[i].selected)
+      current_ime_view_ = ime_view;
+
     // In material design, the property items will be added after the current
     // selected IME item.
     if (list[i].selected && !property_list.empty()) {
       // Adds a separator on the top of property items.
       scroll_content()->AddChildView(
           TrayPopupUtils::CreateListItemSeparator(true));
 
       // Adds the property items.
       for (size_t i = 0; i < property_list.size(); i++) {
         ImeListItemView* property_view = new ImeListItemView(
@@ skipping 67 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();
     last_selected_item_id_.clear();
     last_item_selected_with_keyboard_ = false;
   }
 }
 
+void ImeListView::VisibilityChanged(View* starting_from, bool is_visible) {
+  if (!is_visible || (should_focus_ime_after_selection_with_keyboard_ &&
+                      last_item_selected_with_keyboard_) ||

[tdanderson, 2017/01/12 00:16:15]

This formatting seems strange, can you run 'git cl format ash' ?

[Azure Wei, 2017/01/12 01:20:54]

This is formatted by 'git cl format'.

+      current_ime_view_ == nullptr)

[tdanderson, 2017/01/12 00:16:14]

just "!current_ime_view_" instead of "== nullptr"

[Azure Wei, 2017/01/12 01:20:54]

Done.

+    return;

[tdanderson, 2017/01/12 00:16:14]

use {} since boolean expression spans more than one line

[Azure Wei, 2017/01/12 01:20:54]

Done.

+
+  scroll_content()->ScrollRectToVisible(current_ime_view_->bounds());

[tdanderson, 2017/01/12 00:16:14]

If I am switching to an IME that has a property list associated with it, based
on this line and line 368 it seems that only the IME's name will be scrolled
into view (and its property list will remain hidden). Is that true? Wouldn't we
want to show the whole thing (IME plus its properties)?

[Azure Wei, 2017/01/12 01:20:54]

It's hard to show the whole thing. For opt-in IME menu, at max 5 items could be
shown for the same time.

[tdanderson, 2017/01/12 21:27:33]

Acknowledged.

+}
+
 void ImeListView::FocusCurrentImeIfNeeded() {
   views::FocusManager* manager = GetFocusManager();
   if (!manager || manager->GetFocusedView() || last_selected_item_id_.empty())
     return;
 
   for (auto ime_map : ime_map_) {
     if (ime_map.second == last_selected_item_id_) {
       (ime_map.first)->RequestFocus();
       return;
     }
   }
 
   for (auto property_map : property_map_) {
     if (property_map.second == last_selected_item_id_) {
       (property_map.first)->RequestFocus();
       return;
     }
   }
 }
 
 }  // namespace ash