Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7217008:
Use input method to control visibility of virtual keyboard (Closed)
|
Created:
9 years, 6 months ago by Peng Modified:
9 years, 5 months ago CC:
chromium-reviews, dhollowa, anicolao, Yusuke Sato Visibility:
Public. |
DescriptionUse input method to control visibility of virtual keyboard.
BUG=None
TEST=Test in Chrome OS and Linux desktop
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92146
Patch Set 1 #Patch Set 2 : Update #
Total comments: 1
Patch Set 3 : Update #
Total comments: 9
Patch Set 4 : Update #
Total comments: 22
Patch Set 5 : update #Patch Set 6 : Fix a typo #
Total comments: 10
Patch Set 7 : Update #
Total comments: 5
Patch Set 8 : Update #Patch Set 9 : Rebase #
Messages
Total messages: 25 (0 generated)
http://codereview.chromium.org/7217008/diff/14/views/ime/input_method.cc File views/ime/input_method.cc (right): http://codereview.chromium.org/7217008/diff/14/views/ime/input_method.cc#newc... views/ime/input_method.cc:12: Hi Suzhe, I added some functions into InputMethod interface. And I am also thinking if we could merge InputMethodBase into InputMethod too. Looks all input method implementation will inherit from InputMethodBase.
+yusukes
Just to point out, as a sidenote: the keyboard at the login screen (TouchLoginView) will also need to be updated. Ideally, TLV and TouchBrowserFrameView should share as much of the code as possible so that they always have the same behaviour (they have different heights 360vs300 at the moment, for example). http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method_base.cc File views/ime/input_method_base.cc (right): http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method_base.... views/ime/input_method_base.cc:46: } I think this seeped through from 7222007? http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method_base.... views/ime/input_method_base.cc:59: if (view == focused_view()) Is there a difference between checking 'view == focused_view()' and 'IsViewFocused(view)'? (InputMethodGtk seems to always use the latter)
I love how much simpler this makes TouchBrowserFrameView! As for TouchLoginView: I thought Ryan was going to refactor that code so it was no longer duplicated? I'll start a separate thread about that. http://codereview.chromium.org/7217008/diff/4002/chrome/browser/ui/touch/fram... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (left): http://codereview.chromium.org/7217008/diff/4002/chrome/browser/ui/touch/fram... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:278: // Save the state of the focused field so that the keyboard visibility Is the IME TextInputTypeChanged notification somehow able to provide us with all of the functionality we used to have here? i.e when switching to a tab with a focused field, will we get a TextInputTypeChanged message with the appropriate type for that field? http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method.cc File views/ime/input_method.cc (right): http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method.cc#ne... views/ime/input_method.cc:38: (*iter)->TextInputTypeChanged(view, GetTextInputType()); Perhaps you should call GetTextInputType only once outside of the loop? http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method_ibus.cc File views/ime/input_method_ibus.cc (right): http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method_ibus.... views/ime/input_method_ibus.cc:418: // so just return true here. This seems dangerous. Is there no way to ensure the propagation of the signal without always saying we're active?
My tow points: 1. InputMethod is an interface, so it should always be kept pure virtual. Functionalities that can be shared among all input method implementations should go into InputMethodBase. 2. Can we add a notification type for this purpose instead of adding things into InputMethod interface? We can send the notification in InputMethodBase::OnTextInputTypeChanged() method. http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method_base.cc File views/ime/input_method_base.cc (right): http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method_base.... views/ime/input_method_base.cc:78: TextInputTypeChanged(focused_view()); Changing focus may not necessarily cause text input type change.
My tow points: 1. InputMethod is an interface, so it should always be kept pure virtual. Functionalities that can be shared among all input method implementations should go into InputMethodBase. 2. Can we add a notification type for this purpose instead of adding things into InputMethod interface? We can send the notification in InputMethodBase::OnTextInputTypeChanged() method.
On 2011/06/22 03:43:26, James Su wrote: > My tow points: > 1. InputMethod is an interface, so it should always be kept pure virtual. > Functionalities that can be shared among all input method implementations should > go into InputMethodBase. I see. I am thinking if we could change it to a base class. I am not sure what is the benefit of keep an additional interface here. > > 2. Can we add a notification type for this purpose instead of adding things into > InputMethod interface? We can send the notification in > InputMethodBase::OnTextInputTypeChanged() method. I considered it before. But currently views does not depends on the any code in content folder. I am not sure if we could use it in views.
http://codereview.chromium.org/7217008/diff/4002/chrome/browser/ui/touch/fram... File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (left): http://codereview.chromium.org/7217008/diff/4002/chrome/browser/ui/touch/fram... chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:278: // Save the state of the focused field so that the keyboard visibility On 2011/06/22 01:56:02, bryeung wrote: > Is the IME TextInputTypeChanged notification somehow able to provide us with all > of the functionality we used to have here? i.e when switching to a tab with a > focused field, will we get a TextInputTypeChanged message with the appropriate > type for that field? Yes. But currently it only supports three types (NONE,TEXT,PASSWORD) in page http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method_base.cc File views/ime/input_method_base.cc (right): http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method_base.... views/ime/input_method_base.cc:78: TextInputTypeChanged(focused_view()); On 2011/06/22 03:05:15, James Su wrote: > Changing focus may not necessarily cause text input type change. Probably the name is not good. It not only notify the type changed but also some other input related changes. Maybe rename it to InputStateChanged?
On 2011/06/22 04:17:08, Peng wrote: > On 2011/06/22 03:43:26, James Su wrote: > > My tow points: > > 1. InputMethod is an interface, so it should always be kept pure virtual. > > Functionalities that can be shared among all input method implementations > should > > go into InputMethodBase. > > I see. I am thinking if we could change it to a base class. I am not sure what > is the benefit of keep an additional interface here. > > > > > 2. Can we add a notification type for this purpose instead of adding things > into > > InputMethod interface? We can send the notification in > > InputMethodBase::OnTextInputTypeChanged() method. > > I considered it before. But currently views does not depends on the any code in > content folder. I am not sure if we could use it in views. I see. Then how about to use a separated singleton class for this purpose, just like FocusManager::WidgetFocusManager class?
http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method_base.cc File views/ime/input_method_base.cc (right): http://codereview.chromium.org/7217008/diff/4002/views/ime/input_method_base.... views/ime/input_method_base.cc:78: TextInputTypeChanged(focused_view()); On 2011/06/22 04:17:39, Peng wrote: > On 2011/06/22 03:05:15, James Su wrote: > > Changing focus may not necessarily cause text input type change. > Probably the name is not good. It not only notify the type changed but also some > other input related changes. > > Maybe rename it to InputStateChanged? > IMHO, the current text input type is the only state affecting the virtual keyboard state. The virtual keyboard should not be changed at all if the text input type is not changed when moving focus from one text input box to another. If it's not true, then I think it's better to let the virtual keyboard monitor the focus change state separately in addition to the text input type state, rather than using a mixed state.
On 2011/06/22 07:33:16, James Su wrote:
> On 2011/06/22 04:17:08, Peng wrote:
> > On 2011/06/22 03:43:26, James Su wrote:
> > > My tow points:
> > > 1. InputMethod is an interface, so it should always be kept pure virtual.
> > > Functionalities that can be shared among all input method implementations
> > should
> > > go into InputMethodBase.
> >
> > I see. I am thinking if we could change it to a base class. I am not sure
what
> > is the benefit of keep an additional interface here.
> >
> > >
> > > 2. Can we add a notification type for this purpose instead of adding
things
> > into
> > > InputMethod interface? We can send the notification in
> > > InputMethodBase::OnTextInputTypeChanged() method.
> >
> > I considered it before. But currently views does not depends on the any code
> in
> > content folder. I am not sure if we could use it in views.
> I see. Then how about to use a separated singleton class for this purpose,
just
> like FocusManager::WidgetFocusManager class?
It could be a solution. But chrome could have several InputMethods. Only one
InputMethodManager can not trace types of several InputMethod instances easily.
I have another solution. I think it has better design, but it needs big change.
1. Create an InputMethodManager class. Every Widgets hold an InputMethodManager
pointer instead of InputMethod.
2. The InputMethodManager holds an InputMethod pointer (It could be
InputMethod{IBus,Gtk,Win and etc).
3. Widgets and Views use InputMethodManager to communicate with InputMethod.
4. Remove InputMethodBase, move most logic into InputMethodManager. All
InputMethod implementations just need inherit from InputMethod directly.
5. Simplify InputMethod interface. Some functions are not necessary anymore.
On 2011/06/22 20:56:42, Peng wrote:
> On 2011/06/22 07:33:16, James Su wrote:
> > On 2011/06/22 04:17:08, Peng wrote:
> > > On 2011/06/22 03:43:26, James Su wrote:
> > > > My tow points:
> > > > 1. InputMethod is an interface, so it should always be kept pure
virtual.
> > > > Functionalities that can be shared among all input method
implementations
> > > should
> > > > go into InputMethodBase.
> > >
> > > I see. I am thinking if we could change it to a base class. I am not sure
> what
> > > is the benefit of keep an additional interface here.
> > >
> > > >
> > > > 2. Can we add a notification type for this purpose instead of adding
> things
> > > into
> > > > InputMethod interface? We can send the notification in
> > > > InputMethodBase::OnTextInputTypeChanged() method.
> > >
> > > I considered it before. But currently views does not depends on the any
code
> > in
> > > content folder. I am not sure if we could use it in views.
> > I see. Then how about to use a separated singleton class for this purpose,
> just
> > like FocusManager::WidgetFocusManager class?
>
> It could be a solution. But chrome could have several InputMethods. Only one
> InputMethodManager can not trace types of several InputMethod instances
easily.
One such instance is enough, because there is only one focused view at one time
and we only need to know the focused view's text input type.
>
> I have another solution. I think it has better design, but it needs big
change.
> 1. Create an InputMethodManager class. Every Widgets hold an
InputMethodManager
> pointer instead of InputMethod.
> 2. The InputMethodManager holds an InputMethod pointer (It could be
> InputMethod{IBus,Gtk,Win and etc).
> 3. Widgets and Views use InputMethodManager to communicate with InputMethod.
> 4. Remove InputMethodBase, move most logic into InputMethodManager. All
> InputMethod implementations just need inherit from InputMethod directly.
> 5. Simplify InputMethod interface. Some functions are not necessary anymore.
On 2011/06/23 01:38:34, James Su wrote:
> On 2011/06/22 20:56:42, Peng wrote:
> > On 2011/06/22 07:33:16, James Su wrote:
> > > On 2011/06/22 04:17:08, Peng wrote:
> > > > On 2011/06/22 03:43:26, James Su wrote:
> > > > > My tow points:
> > > > > 1. InputMethod is an interface, so it should always be kept pure
> virtual.
> > > > > Functionalities that can be shared among all input method
> implementations
> > > > should
> > > > > go into InputMethodBase.
> > > >
> > > > I see. I am thinking if we could change it to a base class. I am not
sure
> > what
> > > > is the benefit of keep an additional interface here.
> > > >
> > > > >
> > > > > 2. Can we add a notification type for this purpose instead of adding
> > things
> > > > into
> > > > > InputMethod interface? We can send the notification in
> > > > > InputMethodBase::OnTextInputTypeChanged() method.
> > > >
> > > > I considered it before. But currently views does not depends on the any
> code
> > > in
> > > > content folder. I am not sure if we could use it in views.
> > > I see. Then how about to use a separated singleton class for this purpose,
> > just
> > > like FocusManager::WidgetFocusManager class?
> >
> > It could be a solution. But chrome could have several InputMethods. Only one
> > InputMethodManager can not trace types of several InputMethod instances
> easily.
> One such instance is enough, because there is only one focused view at one
time
> and we only need to know the focused view's text input type.
But currently the view in a widget will not lost the focus, when the widget is
blurred. (I don't know if it is a bug). And every browser window has an
separated virtual keyboard instance. (Every TouchBrowserFrameView has a
separated VirtualKeyboardContainer). All those virtual keyboard instance need
listen on the type change of the input method belonged to its widget. So it is
not easy to use a single instance to control all virtual keyboards. (BTW, Maybe
we will change to single keyboard instance later).
>
> >
> > I have another solution. I think it has better design, but it needs big
> change.
> > 1. Create an InputMethodManager class. Every Widgets hold an
> InputMethodManager
> > pointer instead of InputMethod.
> > 2. The InputMethodManager holds an InputMethod pointer (It could be
> > InputMethod{IBus,Gtk,Win and etc).
> > 3. Widgets and Views use InputMethodManager to communicate with InputMethod.
> > 4. Remove InputMethodBase, move most logic into InputMethodManager. All
> > InputMethod implementations just need inherit from InputMethod directly.
> > 5. Simplify InputMethod interface. Some functions are not necessary anymore.
On 2011/06/23 03:19:38, Peng wrote:
> On 2011/06/23 01:38:34, James Su wrote:
> > On 2011/06/22 20:56:42, Peng wrote:
> > > On 2011/06/22 07:33:16, James Su wrote:
> > > > On 2011/06/22 04:17:08, Peng wrote:
> > > > > On 2011/06/22 03:43:26, James Su wrote:
> > > > > > My tow points:
> > > > > > 1. InputMethod is an interface, so it should always be kept pure
> > virtual.
> > > > > > Functionalities that can be shared among all input method
> > implementations
> > > > > should
> > > > > > go into InputMethodBase.
> > > > >
> > > > > I see. I am thinking if we could change it to a base class. I am not
> sure
> > > what
> > > > > is the benefit of keep an additional interface here.
> > > > >
> > > > > >
> > > > > > 2. Can we add a notification type for this purpose instead of adding
> > > things
> > > > > into
> > > > > > InputMethod interface? We can send the notification in
> > > > > > InputMethodBase::OnTextInputTypeChanged() method.
> > > > >
> > > > > I considered it before. But currently views does not depends on the
any
> > code
> > > > in
> > > > > content folder. I am not sure if we could use it in views.
> > > > I see. Then how about to use a separated singleton class for this
purpose,
> > > just
> > > > like FocusManager::WidgetFocusManager class?
> > >
> > > It could be a solution. But chrome could have several InputMethods. Only
one
> > > InputMethodManager can not trace types of several InputMethod instances
> > easily.
> > One such instance is enough, because there is only one focused view at one
> time
> > and we only need to know the focused view's text input type.
>
> But currently the view in a widget will not lost the focus, when the widget is
> blurred. (I don't know if it is a bug). And every browser window has an
> separated virtual keyboard instance. (Every TouchBrowserFrameView has a
> separated VirtualKeyboardContainer). All those virtual keyboard instance need
> listen on the type change of the input method belonged to its widget. So it is
> not easy to use a single instance to control all virtual keyboards. (BTW,
Maybe
> we will change to single keyboard instance later).
There is always at most one view can get keyboard focus in the system regardless
of the view's focus state inside its window, so the correct design is to use a
singleton object to trace the text input type of the focused view.
Though the current virtual keyboard design that having a virtual keyboard
instance for each browser window does not look right to me, multiple virtual
keyboard instances should still use the same singleton object to trace the
focused text input type, but only the virtual keyboard of the focused browser
window should response to the change of the focused text input type.
>
> >
> > >
> > > I have another solution. I think it has better design, but it needs big
> > change.
> > > 1. Create an InputMethodManager class. Every Widgets hold an
> > InputMethodManager
> > > pointer instead of InputMethod.
> > > 2. The InputMethodManager holds an InputMethod pointer (It could be
> > > InputMethod{IBus,Gtk,Win and etc).
> > > 3. Widgets and Views use InputMethodManager to communicate with
InputMethod.
>
> > > 4. Remove InputMethodBase, move most logic into InputMethodManager. All
> > > InputMethod implementations just need inherit from InputMethod directly.
> > > 5. Simplify InputMethod interface. Some functions are not necessary
anymore.
HI James, I updated the CL. Please review it. Thanks. BTW, This time, the CL only includes a new InputMethodManger class. It does not include code for showing/hiding the VK. Because Sadrul is working on another CL http://codereview.chromium.org/7302015/ related to VK. When that CL is landed, I will create a new CL for it. On 2011/06/23 04:17:30, James Su wrote: > On 2011/06/23 03:19:38, Peng wrote: > > On 2011/06/23 01:38:34, James Su wrote: > > > On 2011/06/22 20:56:42, Peng wrote: > > > > On 2011/06/22 07:33:16, James Su wrote: > > > > > On 2011/06/22 04:17:08, Peng wrote: > > > > > > On 2011/06/22 03:43:26, James Su wrote: > > > > > > > My tow points: > > > > > > > 1. InputMethod is an interface, so it should always be kept pure > > > virtual. > > > > > > > Functionalities that can be shared among all input method > > > implementations > > > > > > should > > > > > > > go into InputMethodBase. > > > > > > > > > > > > I see. I am thinking if we could change it to a base class. I am not > > sure > > > > what > > > > > > is the benefit of keep an additional interface here. > > > > > > > > > > > > > > > > > > > > 2. Can we add a notification type for this purpose instead of adding > > > > things > > > > > > into > > > > > > > InputMethod interface? We can send the notification in > > > > > > > InputMethodBase::OnTextInputTypeChanged() method. > > > > > > > > > > > > I considered it before. But currently views does not depends on the > any > > > code > > > > > in > > > > > > content folder. I am not sure if we could use it in views. > > > > > I see. Then how about to use a separated singleton class for this > purpose, > > > > just > > > > > like FocusManager::WidgetFocusManager class? > > > > > > > > It could be a solution. But chrome could have several InputMethods. Only > one > > > > InputMethodManager can not trace types of several InputMethod instances > > > easily. > > > One such instance is enough, because there is only one focused view at one > > time > > > and we only need to know the focused view's text input type. > > > > But currently the view in a widget will not lost the focus, when the widget is > > blurred. (I don't know if it is a bug). And every browser window has an > > separated virtual keyboard instance. (Every TouchBrowserFrameView has a > > separated VirtualKeyboardContainer). All those virtual keyboard instance need > > listen on the type change of the input method belonged to its widget. So it is > > not easy to use a single instance to control all virtual keyboards. (BTW, > Maybe > > we will change to single keyboard instance later). > There is always at most one view can get keyboard focus in the system regardless > of the view's focus state inside its window, so the correct design is to use a > singleton object to trace the text input type of the focused view. > Though the current virtual keyboard design that having a virtual keyboard > instance for each browser window does not look right to me, multiple virtual > keyboard instances should still use the same singleton object to trace the > focused text input type, but only the virtual keyboard of the focused browser > window should response to the change of the focused text input type. > > > > > > > > > > > > > > > I have another solution. I think it has better design, but it needs big > > > change. > > > > 1. Create an InputMethodManager class. Every Widgets hold an > > > InputMethodManager > > > > pointer instead of InputMethod. > > > > 2. The InputMethodManager holds an InputMethod pointer (It could be > > > > InputMethod{IBus,Gtk,Win and etc). > > > > 3. Widgets and Views use InputMethodManager to communicate with > InputMethod. > > > > > > 4. Remove InputMethodBase, move most logic into InputMethodManager. All > > > > InputMethod implementations just need inherit from InputMethod directly. > > > > 5. Simplify InputMethod interface. Some functions are not necessary > anymore.
http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_base.cc File views/ime/input_method_base.cc (right): http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_base... views/ime/input_method_base.cc:52: manager->OnTextInputTypeChanged(GetTextInputType(), widget_); nit: 1. how about to combine these two lines? The temporary variable is not necessary here. 2. should we only call OnTextInputTypeChanged() if the text input type is really changed? http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_base... views/ime/input_method_base.cc:58: manager->OnTextInputTypeChanged(GetTextInputType(), widget_); ditto http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_base... views/ime/input_method_base.cc:64: manager->OnTextInputTypeChanged(GetTextInputType(), widget_); ditto http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_base... views/ime/input_method_base.cc:126: } move these code into FocusWillChange(). It may not be called by derived classes if they override it. http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... File views/ime/input_method_manager.h (right): http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:15: class View; nit: remove this line. http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:18: class TextInputTypeChangedListener { nit: How about call it TextInputTypeObserver? And add some comment to describe the purpose of this interface? http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:20: virtual void TextInputTypeChanged(ui::TextInputType type, Widget* widget) = 0; nit: Add some comment? e.g. clarify in what situation this method will be called. (I assume that this method will only be called if |widget| has focus, or is getting/losing focus, right?) http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:26: class InputMethodManager { How about to rename this class to something more specific, e.g. TextInputTypeTracker? There is another InputMethodManager in chromeos, using the same name here is a little bit confusing. http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:33: void AddTextInputTypeChangedListener(TextInputTypeChangedListener* listener); nit: AddTextInputTypeObserver(...); http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:34: void RemoveTextInputTypeChangedListener( ditto http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:37: void OnTextInputTypeChanged(ui::TextInputType type, Widget* widget); nit: add comment to clarify whom will call this method.
http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_base.cc File views/ime/input_method_base.cc (right): http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_base... views/ime/input_method_base.cc:52: manager->OnTextInputTypeChanged(GetTextInputType(), widget_); On 2011/07/07 02:03:03, James Su wrote: > nit: > 1. how about to combine these two lines? The temporary variable is not necessary. Done. > here. > 2. should we only call OnTextInputTypeChanged() if the text input type is really > changed? When focus widget is changed, the vk should be notified even if the input type is same. So it could reset the keyboard state. For example clear caplock and etc. So I think it necessary to send call it here. Do you have any suggestion? http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_base... views/ime/input_method_base.cc:58: manager->OnTextInputTypeChanged(GetTextInputType(), widget_); On 2011/07/07 02:03:03, James Su wrote: > ditto Done. http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_base... views/ime/input_method_base.cc:64: manager->OnTextInputTypeChanged(GetTextInputType(), widget_); On 2011/07/07 02:03:03, James Su wrote: > ditto Done. http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_base... views/ime/input_method_base.cc:126: } On 2011/07/07 02:03:03, James Su wrote: > move these code into FocusWillChange(). It may not be called by derived classes > if they override it. Done. http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... File views/ime/input_method_manager.h (right): http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:15: class View; On 2011/07/07 02:03:03, James Su wrote: > nit: remove this line. Done. http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:18: class TextInputTypeChangedListener { On 2011/07/07 02:03:03, James Su wrote: > nit: How about call it TextInputTypeObserver? > And add some comment to describe the purpose of this interface? Done. http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:20: virtual void TextInputTypeChanged(ui::TextInputType type, Widget* widget) = 0; On 2011/07/07 02:03:03, James Su wrote: > nit: Add some comment? e.g. clarify in what situation this method will be > called. (I assume that this method will only be called if |widget| has focus, or > is getting/losing focus, right?) Done. http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:33: void AddTextInputTypeChangedListener(TextInputTypeChangedListener* listener); On 2011/07/07 02:03:03, James Su wrote: > nit: AddTextInputTypeObserver(...); Done. http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:34: void RemoveTextInputTypeChangedListener( On 2011/07/07 02:03:03, James Su wrote: > ditto Done. http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_mana... views/ime/input_method_manager.h:37: void OnTextInputTypeChanged(ui::TextInputType type, Widget* widget); On 2011/07/07 02:03:03, James Su wrote: > nit: add comment to clarify whom will call this method. Done.
http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_base.cc File views/ime/input_method_base.cc (right): http://codereview.chromium.org/7217008/diff/24001/views/ime/input_method_base... views/ime/input_method_base.cc:52: manager->OnTextInputTypeChanged(GetTextInputType(), widget_); On 2011/07/07 17:56:54, Peng wrote: > On 2011/07/07 02:03:03, James Su wrote: > > nit: > > 1. how about to combine these two lines? The temporary variable is not > necessary. > Done. > > > here. > > 2. should we only call OnTextInputTypeChanged() if the text input type is > really > > changed? > > When focus widget is changed, the vk should be notified even if the input type > is same. So it could reset the keyboard state. For example clear caplock and > etc. So I think it necessary to send call it here. Do you have any suggestion? If it's necessary then it's better to state it in comment clearly. http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... File views/ime/text_input_type_tracker.cc (right): http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... views/ime/text_input_type_tracker.cc:7: #include "base/logging.h" nit: is it necessary? http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... views/ime/text_input_type_tracker.cc:35: TextInputTypeTracker* nit: no line break here. http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... File views/ime/text_input_type_tracker.h (right): http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... views/ime/text_input_type_tracker.h:19: // This function will be called, when the type of focused |widget| is changed nit: when the text input type of ... http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... views/ime/text_input_type_tracker.h:20: // or the the widget is losing focus. nit: or the widget is getting/losing focus. http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... views/ime/text_input_type_tracker.h:27: class TextInputTypeTracker { nit: Please add comment to describe this class.
http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... File views/ime/text_input_type_tracker.cc (right): http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... views/ime/text_input_type_tracker.cc:7: #include "base/logging.h" On 2011/07/08 02:15:04, James Su wrote: > nit: is it necessary? Done. http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... views/ime/text_input_type_tracker.cc:35: TextInputTypeTracker* On 2011/07/08 02:15:04, James Su wrote: > nit: no line break here. Done. http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... File views/ime/text_input_type_tracker.h (right): http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... views/ime/text_input_type_tracker.h:19: // This function will be called, when the type of focused |widget| is changed On 2011/07/08 02:15:04, James Su wrote: > nit: when the text input type of ... Done. http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... views/ime/text_input_type_tracker.h:20: // or the the widget is losing focus. On 2011/07/08 02:15:04, James Su wrote: > nit: or the widget is getting/losing focus. Done. http://codereview.chromium.org/7217008/diff/19018/views/ime/text_input_type_t... views/ime/text_input_type_tracker.h:27: class TextInputTypeTracker { On 2011/07/08 02:15:04, James Su wrote: > nit: Please add comment to describe this class. Done.
LGTM. http://codereview.chromium.org/7217008/diff/29001/views/ime/text_input_type_t... File views/ime/text_input_type_tracker.cc (right): http://codereview.chromium.org/7217008/diff/29001/views/ime/text_input_type_t... views/ime/text_input_type_tracker.cc:19: void TextInputTypeTracker::OnTextInputTypeChanged(ui::TextInputType type, nit: align arguments.
Hi Sadurl, Could you review this CL. Do you think it is OK to work with your CL? Thanks. On 2011/07/11 01:36:37, James Su wrote: > LGTM. > > http://codereview.chromium.org/7217008/diff/29001/views/ime/text_input_type_t... > File views/ime/text_input_type_tracker.cc (right): > > http://codereview.chromium.org/7217008/diff/29001/views/ime/text_input_type_t... > views/ime/text_input_type_tracker.cc:19: void > TextInputTypeTracker::OnTextInputTypeChanged(ui::TextInputType type, > nit: align arguments.
Yep. This CL should make the keyboard widget simpler. I haven't checked in the keyboard widget yet (waiting on some changes in views-desktop). But please check this CL in when you are happy with it, and I will update the keyboard widget accordingly. LGTM (with a note below) http://codereview.chromium.org/7217008/diff/29001/views/ime/input_method_gtk.cc File views/ime/input_method_gtk.cc (right): http://codereview.chromium.org/7217008/diff/29001/views/ime/input_method_gtk.... views/ime/input_method_gtk.cc:160: InputMethodBase::OnTextInputTypeChanged(view); I wonder if the notification should be done at the end (i.e. after all the states have been updated for the change of text-input) (here and in IBus and Win). http://codereview.chromium.org/7217008/diff/29001/views/ime/text_input_type_t... File views/ime/text_input_type_tracker.h (right): http://codereview.chromium.org/7217008/diff/29001/views/ime/text_input_type_t... views/ime/text_input_type_tracker.h:44: // Note: The widget should has focus or is losing focus. have
http://codereview.chromium.org/7217008/diff/29001/views/ime/text_input_type_t... File views/ime/text_input_type_tracker.cc (right): http://codereview.chromium.org/7217008/diff/29001/views/ime/text_input_type_t... views/ime/text_input_type_tracker.cc:19: void TextInputTypeTracker::OnTextInputTypeChanged(ui::TextInputType type, On 2011/07/11 01:36:37, James Su wrote: > nit: align arguments. Done. http://codereview.chromium.org/7217008/diff/29001/views/ime/text_input_type_t... File views/ime/text_input_type_tracker.h (right): http://codereview.chromium.org/7217008/diff/29001/views/ime/text_input_type_t... views/ime/text_input_type_tracker.h:44: // Note: The widget should has focus or is losing focus. On 2011/07/11 13:56:06, sadrul wrote: > have Done.
Change committed as 92146 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
