Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2624193002:
Scroll to make the selected IME visible. (Closed)
|
Created:
3 years, 11 months ago by Azure Wei Modified:
3 years, 11 months ago Reviewers:
tdanderson CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nona+watch_chromium.org, oshima+watch_chromium.org, kalyank Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWhen showing IME list view (opt-in IME menu or system IME menu), scroll
the list content to make the current selected IME/input tool visible.
This CL only makes it scroll with opt-in IME menu.
BUG=675516
TEST=Verified on local build.
Review-Url: https://codereview.chromium.org/2624193002
Cr-Commit-Position: refs/heads/master@{#444954}
Committed: https://chromium.googlesource.com/chromium/src/+/ffa5d9bdfbbcf4242213f8c48e5af56ae543b5de
Patch Set 1 #Patch Set 2 #
Total comments: 19
Patch Set 3 #
Messages
Total messages: 20 (10 generated)
Description was changed from ========== Focuses to the current selected IME in system. BUG=675516 TEST=Verified on local build. ========== to ========== When showing IME list view (opt-in IME menu or system IME menu), scroll the list content to make the current selected IME/input tool visible. BUG=675516 TEST=Verified on local build. ==========
azurewei@chromium.org changed reviewers: + tdanderson@chromium.org
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please review this CL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please see my comments below. https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:285: current_ime_view_ = nullptr; Should this be placed in ResetImeListView() instead? https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:314: else if (current_ime_view_ != nullptr) 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 https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:315: scroll_content()->ScrollRectToVisible(current_ime_view_->bounds()); please use {} after the if and else-if clauses since the 'if' boolean expression spans more than one line https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:459: last_item_selected_with_keyboard_) || This formatting seems strange, can you run 'git cl format ash' ? https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:460: current_ime_view_ == nullptr) just "!current_ime_view_" instead of "== nullptr" https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:461: return; use {} since boolean expression spans more than one line https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:463: scroll_content()->ScrollRectToVisible(current_ime_view_->bounds()); 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)? https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.h (right): https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.h:2: // Use of this source code is governed by a BSD-style license that can be Did you manually verify this on both the opt-in IME menu and the IME detailed view?
Description was changed from ========== When showing IME list view (opt-in IME menu or system IME menu), scroll the list content to make the current selected IME/input tool visible. BUG=675516 TEST=Verified on local build. ========== to ========== When showing IME list view (opt-in IME menu or system IME menu), scroll the list content to make the current selected IME/input tool visible. This CL only makes it scroll with opt-in IME menu. BUG=675516 TEST=Verified on local build. ==========
https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:285: current_ime_view_ = nullptr; On 2017/01/12 00:16:14, tdanderson wrote: > Should this be placed in ResetImeListView() instead? Moved to ResetImeListView(). Thanks. https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:314: else if (current_ime_view_ != nullptr) On 2017/01/12 00:16:15, tdanderson wrote: > 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 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. https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:315: scroll_content()->ScrollRectToVisible(current_ime_view_->bounds()); On 2017/01/12 00:16:14, tdanderson wrote: > please use {} after the if and else-if clauses since the 'if' boolean expression > spans more than one line Done. https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:459: last_item_selected_with_keyboard_) || On 2017/01/12 00:16:15, tdanderson wrote: > This formatting seems strange, can you run 'git cl format ash' ? This is formatted by 'git cl format'. https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:460: current_ime_view_ == nullptr) On 2017/01/12 00:16:14, tdanderson wrote: > just "!current_ime_view_" instead of "== nullptr" Done. https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:461: return; On 2017/01/12 00:16:14, tdanderson wrote: > use {} since boolean expression spans more than one line Done. https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:463: scroll_content()->ScrollRectToVisible(current_ime_view_->bounds()); On 2017/01/12 00:16:14, tdanderson wrote: > 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)? It's hard to show the whole thing. For opt-in IME menu, at max 5 items could be shown for the same time. https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.h (right): https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.h:2: // Use of this source code is governed by a BSD-style license that can be On 2017/01/12 00:16:15, tdanderson wrote: > Did you manually verify this on both the opt-in IME menu and the IME detailed > view? This CL only works with opt-in IME menu. For IME detailed, ImeListView could not get event like VisibilityChanged() event, I'm still working on it.
https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:314: else if (current_ime_view_ != nullptr) On 2017/01/12 01:20:54, Azure Wei wrote: > On 2017/01/12 00:16:15, tdanderson wrote: > > 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 > > 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. Acknowledged. https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:463: scroll_content()->ScrollRectToVisible(current_ime_view_->bounds()); On 2017/01/12 01:20:54, Azure Wei wrote: > On 2017/01/12 00:16:14, tdanderson wrote: > > 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)? > > It's hard to show the whole thing. For opt-in IME menu, at max 5 items could be > shown for the same time. Acknowledged. https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.h (right): https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.h:2: // Use of this source code is governed by a BSD-style license that can be On 2017/01/12 01:20:54, Azure Wei wrote: > On 2017/01/12 00:16:15, tdanderson wrote: > > Did you manually verify this on both the opt-in IME menu and the IME detailed > > view? > > This CL only works with opt-in IME menu. For IME detailed, ImeListView could not > get event like VisibilityChanged() event, I'm still working on it. I would strongly prefer a solution which worked for both the opt-in menu as well as the IME detailed view, rather than coming up with two separate solutions. A more general approach to solving this could be to have both surfaces listen for any time the current IME changes (e.g., whatever logic changes the IME in response to Ctrl+Shift+Space would invoke OnIMEChanged() or similar on its listeners - such an interface may already exist). Once invoked, you could call ScrollRectToVisible() on the current IME. Please spend a bit of time and see if you can come up with any ideas here and let me know. If you don't have any luck then I can take another look at this CL tomorrow and we can land it for just the opt-in menu. All things considered, I don't think that using Ctrl+Shift+Space *while* the IME menu is open would be a particularly important use case; it seems the benefit of Ctrl+Shift+Space is that it can be used as a very quick way of switching between IMEs without needing to open any menus.
On 2017/01/12 21:27:33, tdanderson wrote: > https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... > File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): > > https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... > ash/common/system/chromeos/ime_menu/ime_list_view.cc:314: else if > (current_ime_view_ != nullptr) > On 2017/01/12 01:20:54, Azure Wei wrote: > > On 2017/01/12 00:16:15, tdanderson wrote: > > > 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 > > > > 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. > > Acknowledged. > > https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... > ash/common/system/chromeos/ime_menu/ime_list_view.cc:463: > scroll_content()->ScrollRectToVisible(current_ime_view_->bounds()); > On 2017/01/12 01:20:54, Azure Wei wrote: > > On 2017/01/12 00:16:14, tdanderson wrote: > > > 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)? > > > > It's hard to show the whole thing. For opt-in IME menu, at max 5 items could > be > > shown for the same time. > > Acknowledged. > > https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... > File ash/common/system/chromeos/ime_menu/ime_list_view.h (right): > > https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... > ash/common/system/chromeos/ime_menu/ime_list_view.h:2: // Use of this source > code is governed by a BSD-style license that can be > On 2017/01/12 01:20:54, Azure Wei wrote: > > On 2017/01/12 00:16:15, tdanderson wrote: > > > Did you manually verify this on both the opt-in IME menu and the IME > detailed > > > view? > > > > This CL only works with opt-in IME menu. For IME detailed, ImeListView could > not > > get event like VisibilityChanged() event, I'm still working on it. > > I would strongly prefer a solution which worked for both the opt-in menu as well > as the IME detailed view, rather than coming up with two separate solutions. > > A more general approach to solving this could be to have both surfaces listen > for any time the current IME changes (e.g., whatever logic changes the IME in > response to Ctrl+Shift+Space would invoke OnIMEChanged() or similar on its > listeners - such an interface may already exist). Once invoked, you could call > ScrollRectToVisible() on the current IME. > > Please spend a bit of time and see if you can come up with any ideas here and > let me know. If you don't have any luck then I can take another look at this CL > tomorrow and we can land it for just the opt-in menu. All things considered, I > don't think that using Ctrl+Shift+Space *while* the IME menu is open would be a > particularly important use case; it seems the benefit of Ctrl+Shift+Space is > that it can be used as a very quick way of switching between IMEs without > needing to open any menus. Hi, friendly ping regarding my previous comment. Did you get a chance to look at what would be involved with fixing this for the IME detailed view?
On 2017/01/16 22:45:27, tdanderson wrote: > On 2017/01/12 21:27:33, tdanderson wrote: > > > https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... > > File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): > > > > > https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... > > ash/common/system/chromeos/ime_menu/ime_list_view.cc:314: else if > > (current_ime_view_ != nullptr) > > On 2017/01/12 01:20:54, Azure Wei wrote: > > > On 2017/01/12 00:16:15, tdanderson wrote: > > > > 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 > > > > > > 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. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... > > ash/common/system/chromeos/ime_menu/ime_list_view.cc:463: > > scroll_content()->ScrollRectToVisible(current_ime_view_->bounds()); > > On 2017/01/12 01:20:54, Azure Wei wrote: > > > On 2017/01/12 00:16:14, tdanderson wrote: > > > > 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)? > > > > > > It's hard to show the whole thing. For opt-in IME menu, at max 5 items could > > be > > > shown for the same time. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... > > File ash/common/system/chromeos/ime_menu/ime_list_view.h (right): > > > > > https://codereview.chromium.org/2624193002/diff/20001/ash/common/system/chrom... > > ash/common/system/chromeos/ime_menu/ime_list_view.h:2: // Use of this source > > code is governed by a BSD-style license that can be > > On 2017/01/12 01:20:54, Azure Wei wrote: > > > On 2017/01/12 00:16:15, tdanderson wrote: > > > > Did you manually verify this on both the opt-in IME menu and the IME > > detailed > > > > view? > > > > > > This CL only works with opt-in IME menu. For IME detailed, ImeListView could > > not > > > get event like VisibilityChanged() event, I'm still working on it. > > > > I would strongly prefer a solution which worked for both the opt-in menu as > well > > as the IME detailed view, rather than coming up with two separate solutions. > > > > A more general approach to solving this could be to have both surfaces listen > > for any time the current IME changes (e.g., whatever logic changes the IME in > > response to Ctrl+Shift+Space would invoke OnIMEChanged() or similar on its > > listeners - such an interface may already exist). Once invoked, you could call > > ScrollRectToVisible() on the current IME. > > > > Please spend a bit of time and see if you can come up with any ideas here and > > let me know. If you don't have any luck then I can take another look at this > CL > > tomorrow and we can land it for just the opt-in menu. All things considered, I > > don't think that using Ctrl+Shift+Space *while* the IME menu is open would be > a > > particularly important use case; it seems the benefit of Ctrl+Shift+Space is > > that it can be used as a very quick way of switching between IMEs without > > needing to open any menus. > > Hi, friendly ping regarding my previous comment. Did you get a chance > to look at what would be involved with fixing this for the > IME detailed view? I still haven't found out how to fix this for IME detailed view yet. Please review this CL first if you got time. Thanks. (Sorry that I didn't get time to work on chrome so just notified your ping).
LGTM. I have filed crbug.com/682890 for the detailed view version of this bug and assigned to you.
On 2017/01/20 00:10:30, tdanderson wrote: > LGTM. I have filed crbug.com/682890 for the detailed view version of this bug > and assigned to you. Thanks for tracking this. I'll take care of crbug.com/682890.
The CQ bit was checked by azurewei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484875920679740,
"parent_rev": "9a263789936233caabe908fbd02c256ac96ffbad", "commit_rev":
"ffa5d9bdfbbcf4242213f8c48e5af56ae543b5de"}
Message was sent while issue was closed.
Description was changed from ========== When showing IME list view (opt-in IME menu or system IME menu), scroll the list content to make the current selected IME/input tool visible. This CL only makes it scroll with opt-in IME menu. BUG=675516 TEST=Verified on local build. ========== to ========== When showing IME list view (opt-in IME menu or system IME menu), scroll the list content to make the current selected IME/input tool visible. This CL only makes it scroll with opt-in IME menu. BUG=675516 TEST=Verified on local build. Review-Url: https://codereview.chromium.org/2624193002 Cr-Commit-Position: refs/heads/master@{#444954} Committed: https://chromium.googlesource.com/chromium/src/+/ffa5d9bdfbbcf4242213f8c48e5a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/ffa5d9bdfbbcf4242213f8c48e5a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
