Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2031433002:
Views::Textfield: Decouple handling of menu and text editing commands. (Closed)
|
Created:
4 years, 6 months ago by karandeepb Modified:
4 years, 6 months ago CC:
chromium-reviews, tfarina, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, nona+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@refactor2_virtual_calls Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionViews::Textfield: Decouple handling of menu and text editing commands.
Currently, all text editing commands are routed through IsCommandIdEnabled() and
ExecuteCommand(int command_id, int event_flags) which are
ui::SimpleMenuModel::Delegate overrides. These were probably just meant to be
used for menu actions, but are currently also being used for text editing
commands.
This CL modifies IsCommandIdEnabled and
ExecuteCommand(int command_id, int event_flags) in views::Textfield and its
subclass OmniboxViewViews, to only handle menu actions. To handle text editing
commands, IsEditCommandEnabled (overriden from ui::TextInputClient) is used.
Analogous to ExecuteCommand, a protected virtual method ExecuteEditCommand is
added to views::Textfield to execute text editing commands. Also, since it's
confusing to have both an overloaded and overriden ExecuteCommand method, the
overloaded version is removed.
A helper function bool `IsMenuCommand(int)` is also added which will be updated
in a follow-up (crrev.com/2027133002/) to return a text editing command enum
rather than a bool. (i.e. `ui::TextEditCommand
GetTextEditCommandFromMenuCommand(int command_id)`).
This is first in series of CLs to replace resource ids with a
ui::TextEditCommand enum for text editing commands in views::Textfield.
Link to complete patch - http://crrev.com/2029733003
BUG=586985
Committed: https://crrev.com/8ca173a988171b43b6686335335f816a0ff27403
Cr-Commit-Position: refs/heads/master@{#400909}
Patch Set 1 #Patch Set 2 : #
Total comments: 7
Patch Set 3 : Rebase. #Patch Set 4 : Address review comments. #
Total comments: 6
Patch Set 5 : #Patch Set 6 : Address review comments. #
Total comments: 5
Patch Set 7 : Rebase. #Patch Set 8 : DestroyTouchSelection calls. #Patch Set 9 : #
Total comments: 7
Patch Set 10 : Address review comments. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 30 (13 generated)
Description was changed from ========== Refactor3 ========== to ========== Views::Textfield: Decouple handling of menu and text editing commands. Currently, all text editing commands are routed through IsCommandIdEnabled() and ExecuteCommand(int command_id, int event_flags) which are ui::SimpleMenuModel::Delegate overrides. These were probably just meant to be used for menu actions, but are currently also being used for text editing commands. This CL modifies IsCommandIdEnabled and ExecuteCommand(int command_id, int event_flags) in views::Textfield and its subclass OmniboxViewViews, to only handle menu actions. To handle text editing commands, IsEditCommandEnabled (overriden from ui::TextInputClient) is used. Analogous to ExecuteCommand, a protected virtual method ExecuteEditCommand is added to views::Textfield to execute text editing commands. This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. BUG=586985 ==========
tapted@chromium.org changed reviewers: + tapted@chromium.org
I would also say that it's confusing having Textfield both overload and override a method called `ExecuteCommand`.. https://codereview.chromium.org/2031433002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2031433002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:927: case IDS_APP_PASTE: Is Paste not handled by Textfield::IsCommandIdEnabled already? it also disables the command when the clipboard is empty. https://codereview.chromium.org/2031433002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:935: if (!IsEditCommandEnabled(command_id)) could this DCHECK? (if we can't we should probably add a comment, since it looks a bit unusual) https://codereview.chromium.org/2031433002/diff/20001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2031433002/diff/20001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:196: // Todo why are these not mapped-- for omnibox? hm - I have no clue :/. https://codereview.chromium.org/2031433002/diff/20001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1278: Textfield::ExecuteEditCommand(command_id); I think this needs a comment. A reviewer might get here and wonder about the whole purpose of the changes.
Description was changed from ========== Views::Textfield: Decouple handling of menu and text editing commands. Currently, all text editing commands are routed through IsCommandIdEnabled() and ExecuteCommand(int command_id, int event_flags) which are ui::SimpleMenuModel::Delegate overrides. These were probably just meant to be used for menu actions, but are currently also being used for text editing commands. This CL modifies IsCommandIdEnabled and ExecuteCommand(int command_id, int event_flags) in views::Textfield and its subclass OmniboxViewViews, to only handle menu actions. To handle text editing commands, IsEditCommandEnabled (overriden from ui::TextInputClient) is used. Analogous to ExecuteCommand, a protected virtual method ExecuteEditCommand is added to views::Textfield to execute text editing commands. This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. BUG=586985 ========== to ========== Views::Textfield: Decouple handling of menu and text editing commands. Currently, all text editing commands are routed through IsCommandIdEnabled() and ExecuteCommand(int command_id, int event_flags) which are ui::SimpleMenuModel::Delegate overrides. These were probably just meant to be used for menu actions, but are currently also being used for text editing commands. This CL modifies IsCommandIdEnabled and ExecuteCommand(int command_id, int event_flags) in views::Textfield and its subclass OmniboxViewViews, to only handle menu actions. To handle text editing commands, IsEditCommandEnabled (overriden from ui::TextInputClient) is used. Analogous to ExecuteCommand, a protected virtual method ExecuteEditCommand is added to views::Textfield to execute text editing commands. Also, since it's confusing to have both an overloaded and overriden ExecuteCommand method, the overloaded version is removed. This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. BUG=586985 ==========
PTAL Thanks. https://codereview.chromium.org/2031433002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2031433002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:927: case IDS_APP_PASTE: On 2016/06/06 07:42:27, tapted wrote: > Is Paste not handled by Textfield::IsCommandIdEnabled already? it also disables > the command when the clipboard is empty. Added the clipboard condition. Yeah it is. But Omnibox uses a custom GetClipboardText() implementation https://cs.chromium.org/chromium/src/chrome/browser/ui/omnibox/clipboard_util... https://codereview.chromium.org/2031433002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:935: if (!IsEditCommandEnabled(command_id)) On 2016/06/06 07:42:27, tapted wrote: > could this DCHECK? (if we can't we should probably add a comment, since it looks > a bit unusual) A normal if condition looks safer to me. Why does it look unusual though, because it's a protected method? https://codereview.chromium.org/2031433002/diff/20001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2031433002/diff/20001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1278: Textfield::ExecuteEditCommand(command_id); On 2016/06/06 07:42:27, tapted wrote: > I think this needs a comment. A reviewer might get here and wonder about the > whole purpose of the changes. Can't really think of a good comment to make. Something like "Forward |command_id| to ExecuteEditCommand for execution" just describes the code. I think these changes should be fairly clear in light of the next CL - https://codereview.chromium.org/2027133002
https://codereview.chromium.org/2031433002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2031433002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:196: // Todo why are these not mapped-- for omnibox? maybe remove this for now? Otherwise, we should do a proper TODO and cite a bug. https://codereview.chromium.org/2031433002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1243: return IsMenuCommand(command_id) && So.. I'm not sure whether `IsMenuCommand() adds much. I think you could equally write this as DCHECK(IsMenuCommand(command_id)); But then IsMenuCommand would only be used for dev builds. But there's more to it - Textfield::UpdateContextMenu() {..} also does if (controller_) controller_->UpdateContextMenu(context_menu_contents_.get()); So the controller can add things, but the controller can't update IsMenuCommand() - does this mean everything it added will be disabled? (I guess not if this method is overridden again, but now we're imblanced wrt the controller) Perhaps instead something like // Only commands in the context menu go through this. DCHECK(context_menu_contents_); DCHECK(context_menu_contents_->GetIndexOfCommandId(command_id) >= 0); But I think we can also just omit checking and call IsEditCommandEnabled directly. But perhaps comment here instead. https://codereview.chromium.org/2031433002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1278: Textfield::ExecuteEditCommand(command_id); Something like // Items selected from the context menu go through this. Textfield only adds edit commands. If the controller added things in UpdateContextMenu(), an ExecuteCommand() override may also be needed.
Patchset #6 (id:100001) has been deleted
PTAL Trent. Thanks. https://codereview.chromium.org/2031433002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2031433002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:196: // Todo why are these not mapped-- for omnibox? On 2016/06/08 03:56:45, tapted wrote: > maybe remove this for now? > > Otherwise, we should do a proper TODO and cite a bug. Done. https://codereview.chromium.org/2031433002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1243: return IsMenuCommand(command_id) && On 2016/06/08 03:56:45, tapted wrote: > So.. I'm not sure whether `IsMenuCommand() adds much. I think you could equally > write this as > > DCHECK(IsMenuCommand(command_id)); > > But then IsMenuCommand would only be used for dev builds. > > But there's more to it - Textfield::UpdateContextMenu() {..} also does > if (controller_) > controller_->UpdateContextMenu(context_menu_contents_.get()); > > So the controller can add things, but the controller can't update > IsMenuCommand() - does this mean everything it added will be disabled? (I guess > not if this method is overridden again, but now we're imblanced wrt the > controller) > > Perhaps instead something like > > // Only commands in the context menu go through this. > DCHECK(context_menu_contents_); > DCHECK(context_menu_contents_->GetIndexOfCommandId(command_id) >= 0); > > But I think we can also just omit checking and call IsEditCommandEnabled > directly. But perhaps comment here instead. Have added a comment in UpdateContextMenu. https://codereview.chromium.org/2031433002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1278: Textfield::ExecuteEditCommand(command_id); On 2016/06/08 03:56:45, tapted wrote: > Something like > > // Items selected from the context menu go through this. Textfield only adds > edit commands. If the controller added things in UpdateContextMenu(), an > ExecuteCommand() override may also be needed. Have added a comment in UpdateContextMenu.
https://codereview.chromium.org/2031433002/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2031433002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1242: return IsMenuCommand(command_id) && I'm still dubious about the merit of the `IsMenuCommand` check. If there's no bugs by omitting it, then the benefit is that we don't have the maintenance problem of keeping IsMenuCommand() in sync with UpdateContextMenu. Per my prior comment. I think an improvement on this would be: // Only commands in the context menu go through this. DCHECK(context_menu_contents_); DCHECK(context_menu_contents_->GetIndexOfCommandId(command_id) >= 0); return Textfield::IsEditCommandEnabled(command_id); Is there a reason not to do this? https://codereview.chromium.org/2031433002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1878: // IsCommandIdEnabled() as appropriate, for the commands added. I don't think it makes sense for UpdateContextMenu to be on TextFieldController, if it needs to be paired with overrides on Textfield. OmniboxViewViews::UpdateContextMenu is the only override, which also inherits from Textfield already, so it would be easy to move. See what msw thinks.
https://codereview.chromium.org/2031433002/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2031433002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1242: return IsMenuCommand(command_id) && On 2016/06/09 04:10:03, tapted wrote: > I'm still dubious about the merit of the `IsMenuCommand` check. If there's no > bugs by omitting it, then the benefit is that we don't have the maintenance > problem of keeping IsMenuCommand() in sync with UpdateContextMenu. > > Per my prior comment. I think an improvement on this would be: > > // Only commands in the context menu go through this. > DCHECK(context_menu_contents_); > DCHECK(context_menu_contents_->GetIndexOfCommandId(command_id) >= 0); > return Textfield::IsEditCommandEnabled(command_id); > > Is there a reason not to do this? IsMenuCommand is replaced by ui::TextEditCommand GetTextEditCommandFromMenuCommand(int command_id) in the next CL (https://codereview.chromium.org/2027133002/), i.e. I'll need a way to map a menu action to a ui::TextEditCommand. So we'll always need to keep it in sync with UpdateContextMenu. I can remove it in this CL but we'll need an analogous function in the next anyway. https://codereview.chromium.org/2031433002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1878: // IsCommandIdEnabled() as appropriate, for the commands added. On 2016/06/09 04:10:03, tapted wrote: > I don't think it makes sense for UpdateContextMenu to be on TextFieldController, > if it needs to be paired with overrides on Textfield. > OmniboxViewViews::UpdateContextMenu is the only override, which also inherits > from Textfield already, so it would be easy to move. See what msw thinks. Yeah, I agree. Will wait for owner's review on this.
lgtm https://codereview.chromium.org/2031433002/diff/120001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2031433002/diff/120001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1242: return IsMenuCommand(command_id) && On 2016/06/09 04:51:42, karandeepb wrote: > On 2016/06/09 04:10:03, tapted wrote: > > I'm still dubious about the merit of the `IsMenuCommand` check. If there's no > > bugs by omitting it, then the benefit is that we don't have the maintenance > > problem of keeping IsMenuCommand() in sync with UpdateContextMenu. > > > > Per my prior comment. I think an improvement on this would be: > > > > // Only commands in the context menu go through this. > > DCHECK(context_menu_contents_); > > DCHECK(context_menu_contents_->GetIndexOfCommandId(command_id) >= 0); > > return Textfield::IsEditCommandEnabled(command_id); > > > > Is there a reason not to do this? > > IsMenuCommand is replaced by ui::TextEditCommand > GetTextEditCommandFromMenuCommand(int command_id) in the next CL > (https://codereview.chromium.org/2027133002/), i.e. I'll need a way to map a > menu action to a ui::TextEditCommand. So we'll always need to keep it in sync > with UpdateContextMenu. I can remove it in this CL but we'll need an analogous > function in the next anyway. Gotcha - it makes a lot more sense with that context. This is worth mentioning. E.g., in the CL description, Adds `bool IsMenuCommand(int)` which be updated in a follow-up CL to return a text editing command from the enum rather than a bool. (i.e. `ui::TextEditCommand GetTextEditCommandFromMenuCommand(int command_id)`).
Description was changed from ========== Views::Textfield: Decouple handling of menu and text editing commands. Currently, all text editing commands are routed through IsCommandIdEnabled() and ExecuteCommand(int command_id, int event_flags) which are ui::SimpleMenuModel::Delegate overrides. These were probably just meant to be used for menu actions, but are currently also being used for text editing commands. This CL modifies IsCommandIdEnabled and ExecuteCommand(int command_id, int event_flags) in views::Textfield and its subclass OmniboxViewViews, to only handle menu actions. To handle text editing commands, IsEditCommandEnabled (overriden from ui::TextInputClient) is used. Analogous to ExecuteCommand, a protected virtual method ExecuteEditCommand is added to views::Textfield to execute text editing commands. Also, since it's confusing to have both an overloaded and overriden ExecuteCommand method, the overloaded version is removed. This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. BUG=586985 ========== to ========== Views::Textfield: Decouple handling of menu and text editing commands. Currently, all text editing commands are routed through IsCommandIdEnabled() and ExecuteCommand(int command_id, int event_flags) which are ui::SimpleMenuModel::Delegate overrides. These were probably just meant to be used for menu actions, but are currently also being used for text editing commands. This CL modifies IsCommandIdEnabled and ExecuteCommand(int command_id, int event_flags) in views::Textfield and its subclass OmniboxViewViews, to only handle menu actions. To handle text editing commands, IsEditCommandEnabled (overriden from ui::TextInputClient) is used. Analogous to ExecuteCommand, a protected virtual method ExecuteEditCommand is added to views::Textfield to execute text editing commands. Also, since it's confusing to have both an overloaded and overriden ExecuteCommand method, the overloaded version is removed. A helper function bool `IsMenuCommand(int)` is also added which will be updated in a follow-up (crrev.com/2027133002/) to return a text editing command enum rather than a bool. (i.e. `ui::TextEditCommand GetTextEditCommandFromMenuCommand(int command_id)`). This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. BUG=586985 ==========
On 2016/06/09 05:02:01, tapted wrote: > lgtm > > https://codereview.chromium.org/2031433002/diff/120001/ui/views/controls/text... > File ui/views/controls/textfield/textfield.cc (right): > > https://codereview.chromium.org/2031433002/diff/120001/ui/views/controls/text... > ui/views/controls/textfield/textfield.cc:1242: return IsMenuCommand(command_id) > && > On 2016/06/09 04:51:42, karandeepb wrote: > > On 2016/06/09 04:10:03, tapted wrote: > > > I'm still dubious about the merit of the `IsMenuCommand` check. If there's > no > > > bugs by omitting it, then the benefit is that we don't have the maintenance > > > problem of keeping IsMenuCommand() in sync with UpdateContextMenu. > > > > > > Per my prior comment. I think an improvement on this would be: > > > > > > // Only commands in the context menu go through this. > > > DCHECK(context_menu_contents_); > > > DCHECK(context_menu_contents_->GetIndexOfCommandId(command_id) >= 0); > > > return Textfield::IsEditCommandEnabled(command_id); > > > > > > Is there a reason not to do this? > > > > IsMenuCommand is replaced by ui::TextEditCommand > > GetTextEditCommandFromMenuCommand(int command_id) in the next CL > > (https://codereview.chromium.org/2027133002/), i.e. I'll need a way to map a > > menu action to a ui::TextEditCommand. So we'll always need to keep it in sync > > with UpdateContextMenu. I can remove it in this CL but we'll need an analogous > > function in the next anyway. > > Gotcha - it makes a lot more sense with that context. This is worth mentioning. > E.g., in the CL description, > > Adds `bool IsMenuCommand(int)` which be updated in a follow-up CL to return a > text editing command from the enum rather than a bool. (i.e. > `ui::TextEditCommand GetTextEditCommandFromMenuCommand(int command_id)`). Done. Thanks.
Patchset #9 (id:180001) has been deleted
PTAL msw@. This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Here is the link to a CL with all the changes included - https://codereview.chromium.org/2029733003/. https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1276: if (!Textfield::IsCommandIdEnabled(command_id)) This was a virtual call earlier. However doing Textfield::IsCommandIdEnabled looks more correct to me. Thoughts? https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1879: // IsCommandIdEnabled() as appropriate, for the commands added. It seems weird that the controller can update the context menu, but we don't have a way to forward IsCommandIdEnabled and ExecuteCommand to the controller. Hence if a controller wants to add commands to the context menu, it will also have to inherit from Textfield (like Omnibox does). Does it make more sense to remove UpdateContextMenu from TextfieldController and give subclasses a way to update the context menu instead.
karandeepb@chromium.org changed reviewers: + msw@chromium.org
PTAL msw@. This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Here is the link to a CL with all the changes included - https://codereview.chromium.org/2029733003/. https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1276: if (!Textfield::IsCommandIdEnabled(command_id)) This was a virtual call earlier. However doing Textfield::IsCommandIdEnabled looks more correct to me. Thoughts? https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1879: // IsCommandIdEnabled() as appropriate, for the commands added. It seems weird that the controller can update the context menu, but we don't have a way to forward IsCommandIdEnabled and ExecuteCommand to the controller. Hence if a controller wants to add commands to the context menu, it will also have to inherit from Textfield (like Omnibox does). Does it make more sense to remove UpdateContextMenu from TextfieldController and give subclasses a way to update the context menu instead.
Description was changed from ========== Views::Textfield: Decouple handling of menu and text editing commands. Currently, all text editing commands are routed through IsCommandIdEnabled() and ExecuteCommand(int command_id, int event_flags) which are ui::SimpleMenuModel::Delegate overrides. These were probably just meant to be used for menu actions, but are currently also being used for text editing commands. This CL modifies IsCommandIdEnabled and ExecuteCommand(int command_id, int event_flags) in views::Textfield and its subclass OmniboxViewViews, to only handle menu actions. To handle text editing commands, IsEditCommandEnabled (overriden from ui::TextInputClient) is used. Analogous to ExecuteCommand, a protected virtual method ExecuteEditCommand is added to views::Textfield to execute text editing commands. Also, since it's confusing to have both an overloaded and overriden ExecuteCommand method, the overloaded version is removed. A helper function bool `IsMenuCommand(int)` is also added which will be updated in a follow-up (crrev.com/2027133002/) to return a text editing command enum rather than a bool. (i.e. `ui::TextEditCommand GetTextEditCommandFromMenuCommand(int command_id)`). This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. BUG=586985 ========== to ========== Views::Textfield: Decouple handling of menu and text editing commands. Currently, all text editing commands are routed through IsCommandIdEnabled() and ExecuteCommand(int command_id, int event_flags) which are ui::SimpleMenuModel::Delegate overrides. These were probably just meant to be used for menu actions, but are currently also being used for text editing commands. This CL modifies IsCommandIdEnabled and ExecuteCommand(int command_id, int event_flags) in views::Textfield and its subclass OmniboxViewViews, to only handle menu actions. To handle text editing commands, IsEditCommandEnabled (overriden from ui::TextInputClient) is used. Analogous to ExecuteCommand, a protected virtual method ExecuteEditCommand is added to views::Textfield to execute text editing commands. Also, since it's confusing to have both an overloaded and overriden ExecuteCommand method, the overloaded version is removed. A helper function bool `IsMenuCommand(int)` is also added which will be updated in a follow-up (crrev.com/2027133002/) to return a text editing command enum rather than a bool. (i.e. `ui::TextEditCommand GetTextEditCommandFromMenuCommand(int command_id)`). This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Link to complete patch - http://crrev.com/2029733003 BUG=586985 ==========
lgtm with a nit. https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1276: if (!Textfield::IsCommandIdEnabled(command_id)) On 2016/06/09 10:47:10, karandeepb wrote: > This was a virtual call earlier. However doing Textfield::IsCommandIdEnabled > looks more correct to me. Thoughts? Yeah, this is probably more correct; subclasses should override IsCommandIdEnabled *and* ExecuteCommand to enable additional commands, etc. https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1276: if (!Textfield::IsCommandIdEnabled(command_id)) optional nit: invert this check and inline the ExecuteEditCommand call. https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1879: // IsCommandIdEnabled() as appropriate, for the commands added. On 2016/06/09 10:47:10, karandeepb wrote: > It seems weird that the controller can update the context menu, but we don't > have a way to forward IsCommandIdEnabled and ExecuteCommand to the controller. > Hence if a controller wants to add commands to the context menu, it will also > have to inherit from Textfield (like Omnibox does). Does it make more sense to > remove UpdateContextMenu from TextfieldController and give subclasses a way to > update the context menu instead. That's a good point. Making Textfield::UpdateContextMenu virtual, and letting OmniboxViewViews override that would be nice, but we'd need to move the |context_menu_runner_.reset...| statement elsewhere. It's not critical, but that'd be a nice fix after you're done with this set of CLs.
https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1276: if (!Textfield::IsCommandIdEnabled(command_id)) On 2016/06/10 00:07:50, msw wrote: > optional nit: invert this check and inline the ExecuteEditCommand call. Done. https://codereview.chromium.org/2031433002/diff/200001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1879: // IsCommandIdEnabled() as appropriate, for the commands added. On 2016/06/10 00:07:50, msw wrote: > On 2016/06/09 10:47:10, karandeepb wrote: > > It seems weird that the controller can update the context menu, but we don't > > have a way to forward IsCommandIdEnabled and ExecuteCommand to the controller. > > Hence if a controller wants to add commands to the context menu, it will also > > have to inherit from Textfield (like Omnibox does). Does it make more sense to > > remove UpdateContextMenu from TextfieldController and give subclasses a way to > > update the context menu instead. > > That's a good point. Making Textfield::UpdateContextMenu virtual, and letting > OmniboxViewViews override that would be nice, but we'd need to move the > |context_menu_runner_.reset...| statement elsewhere. It's not critical, but > that'd be a nice fix after you're done with this set of CLs. WIll do this in a follow-up after this set of CLs land.
karandeepb@chromium.org changed reviewers: + sky@chromium.org
+sky@ for ui/views/view_unittest.cc review.
LGTM
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2031433002/#ps220001 (title: "Address review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031433002/220001
Message was sent while issue was closed.
Description was changed from ========== Views::Textfield: Decouple handling of menu and text editing commands. Currently, all text editing commands are routed through IsCommandIdEnabled() and ExecuteCommand(int command_id, int event_flags) which are ui::SimpleMenuModel::Delegate overrides. These were probably just meant to be used for menu actions, but are currently also being used for text editing commands. This CL modifies IsCommandIdEnabled and ExecuteCommand(int command_id, int event_flags) in views::Textfield and its subclass OmniboxViewViews, to only handle menu actions. To handle text editing commands, IsEditCommandEnabled (overriden from ui::TextInputClient) is used. Analogous to ExecuteCommand, a protected virtual method ExecuteEditCommand is added to views::Textfield to execute text editing commands. Also, since it's confusing to have both an overloaded and overriden ExecuteCommand method, the overloaded version is removed. A helper function bool `IsMenuCommand(int)` is also added which will be updated in a follow-up (crrev.com/2027133002/) to return a text editing command enum rather than a bool. (i.e. `ui::TextEditCommand GetTextEditCommandFromMenuCommand(int command_id)`). This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Link to complete patch - http://crrev.com/2029733003 BUG=586985 ========== to ========== Views::Textfield: Decouple handling of menu and text editing commands. Currently, all text editing commands are routed through IsCommandIdEnabled() and ExecuteCommand(int command_id, int event_flags) which are ui::SimpleMenuModel::Delegate overrides. These were probably just meant to be used for menu actions, but are currently also being used for text editing commands. This CL modifies IsCommandIdEnabled and ExecuteCommand(int command_id, int event_flags) in views::Textfield and its subclass OmniboxViewViews, to only handle menu actions. To handle text editing commands, IsEditCommandEnabled (overriden from ui::TextInputClient) is used. Analogous to ExecuteCommand, a protected virtual method ExecuteEditCommand is added to views::Textfield to execute text editing commands. Also, since it's confusing to have both an overloaded and overriden ExecuteCommand method, the overloaded version is removed. A helper function bool `IsMenuCommand(int)` is also added which will be updated in a follow-up (crrev.com/2027133002/) to return a text editing command enum rather than a bool. (i.e. `ui::TextEditCommand GetTextEditCommandFromMenuCommand(int command_id)`). This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Link to complete patch - http://crrev.com/2029733003 BUG=586985 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Views::Textfield: Decouple handling of menu and text editing commands. Currently, all text editing commands are routed through IsCommandIdEnabled() and ExecuteCommand(int command_id, int event_flags) which are ui::SimpleMenuModel::Delegate overrides. These were probably just meant to be used for menu actions, but are currently also being used for text editing commands. This CL modifies IsCommandIdEnabled and ExecuteCommand(int command_id, int event_flags) in views::Textfield and its subclass OmniboxViewViews, to only handle menu actions. To handle text editing commands, IsEditCommandEnabled (overriden from ui::TextInputClient) is used. Analogous to ExecuteCommand, a protected virtual method ExecuteEditCommand is added to views::Textfield to execute text editing commands. Also, since it's confusing to have both an overloaded and overriden ExecuteCommand method, the overloaded version is removed. A helper function bool `IsMenuCommand(int)` is also added which will be updated in a follow-up (crrev.com/2027133002/) to return a text editing command enum rather than a bool. (i.e. `ui::TextEditCommand GetTextEditCommandFromMenuCommand(int command_id)`). This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Link to complete patch - http://crrev.com/2029733003 BUG=586985 ========== to ========== Views::Textfield: Decouple handling of menu and text editing commands. Currently, all text editing commands are routed through IsCommandIdEnabled() and ExecuteCommand(int command_id, int event_flags) which are ui::SimpleMenuModel::Delegate overrides. These were probably just meant to be used for menu actions, but are currently also being used for text editing commands. This CL modifies IsCommandIdEnabled and ExecuteCommand(int command_id, int event_flags) in views::Textfield and its subclass OmniboxViewViews, to only handle menu actions. To handle text editing commands, IsEditCommandEnabled (overriden from ui::TextInputClient) is used. Analogous to ExecuteCommand, a protected virtual method ExecuteEditCommand is added to views::Textfield to execute text editing commands. Also, since it's confusing to have both an overloaded and overriden ExecuteCommand method, the overloaded version is removed. A helper function bool `IsMenuCommand(int)` is also added which will be updated in a follow-up (crrev.com/2027133002/) to return a text editing command enum rather than a bool. (i.e. `ui::TextEditCommand GetTextEditCommandFromMenuCommand(int command_id)`). This is first in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Link to complete patch - http://crrev.com/2029733003 BUG=586985 Committed: https://crrev.com/8ca173a988171b43b6686335335f816a0ff27403 Cr-Commit-Position: refs/heads/master@{#400909} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/8ca173a988171b43b6686335335f816a0ff27403 Cr-Commit-Position: refs/heads/master@{#400909} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
