|
|
Created:
11 years, 4 months ago by James Su Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, Ben Goodger (Google) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionImplements unimplemented methods of AutocompleteEditViewGtk and fixes a regression issue.
This CL implements two unimplemented methods of AutocompleteEditViewGtk: OnRevertTemporaryText() and IsSelectAll().
The implementation are mostly copied from autocomplete_edit_view_mac.mm.
Some grammer errors and a valgrind warning introduced by CL 165457 are also fixed.
This CL was just updated to fix a regression caused by CL 165457 (issue 19631).
BUG=19631
: Tabbing on Omnibox enters in to search UI mode
TEST=Input something in omnibox and make sure some text is selected and popup view is opened, change current selection in popup view by pressing up/down then press escape to see if omnibox is reverted to its original content and selection.
TEST=Make sure www.google.com is one of the search engine, open www.google.com then move the focus back to omnibox and press tab to see if the focus is moved into web page.
Patch Set 1 #Patch Set 2 : '' #
Total comments: 7
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 2
Patch Set 5 : '' #
Total comments: 4
Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 1
Messages
Total messages: 16 (0 generated)
Hi, Please help review this CL. Regards James Su
http://codereview.chromium.org/172041/diff/1004/5 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (left): http://codereview.chromium.org/172041/diff/1004/5#oldcode334 Line 334: EmphasizeURLComponents(); it looks like this doesn't get called now -- is this intentional? http://codereview.chromium.org/172041/diff/1004/5 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/172041/diff/1004/5#newcode343 Line 343: gtk_text_buffer_select_range(text_buffer_, &insert, &bound); this looks like it will update the PRIMARY X selection. if this isn't what the user expects, you should call StartUpdatingHighlightedText() before and FinishUpdatingHighlightedText() after so the X selection won't be updated. http://codereview.chromium.org/172041/diff/1004/5#newcode368 Line 368: return gtk_text_iter_equal(&start, &sel_start) && do you need to worry about the order of the selection here (i.e. right-to-left vs. left-to-right)? in other words, should this be: return (gtk_text_iter_equal(&start, &sel_start) && gtk_text_iter_equal(&end, &sel_end)) || (gtk_text_iter_equal(&start, &sel_end) && gtk_text_iter_equal(&end, &sel_start)); ?
CL updated. Please help review again. http://codereview.chromium.org/172041/diff/1004/5 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (left): http://codereview.chromium.org/172041/diff/1004/5#oldcode334 Line 334: EmphasizeURLComponents(); On 2009/08/17 20:33:50, Daniel Erat wrote: > it looks like this doesn't get called now -- is this intentional? It's called in TextChanged(). Though I just found a TextChanged() call was missing in OnRevertTemporaryText(). http://codereview.chromium.org/172041/diff/1004/5 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/172041/diff/1004/5#newcode343 Line 343: gtk_text_buffer_select_range(text_buffer_, &insert, &bound); On 2009/08/17 20:33:50, Daniel Erat wrote: > this looks like it will update the PRIMARY X selection. if this isn't what the > user expects, you should call StartUpdatingHighlightedText() before and > FinishUpdatingHighlightedText() after so the X selection won't be updated. Yes, it will update the PRIMARY x selection. However I think {Start|Finish}Updating... shall be called by caller of SetTextAndSelectedRange() rather than here. The old code of OnTemporaryTextMaybeChanged() didn't call {Start|Finish}... around SetWindowTextAndCaretPos(), so I'm wondering if it's necessary to to call them in both OnTemporaryTextMaybeChanged() and OnRevertTemporaryText(). What's your opinion? http://codereview.chromium.org/172041/diff/1004/5#newcode368 Line 368: return gtk_text_iter_equal(&start, &sel_start) && On 2009/08/17 20:33:50, Daniel Erat wrote: > do you need to worry about the order of the selection here (i.e. right-to-left > vs. left-to-right)? in other words, should this be: > > return (gtk_text_iter_equal(&start, &sel_start) && > gtk_text_iter_equal(&end, &sel_end)) || > (gtk_text_iter_equal(&start, &sel_end) && > gtk_text_iter_equal(&end, &sel_start)); > > ? We don't need to worry about it. gtk_text_buffer_get_selection_bounds() will sort sel_start and sel_end in ascending order.
Hi, I just updated this CL to fix issue 19631. Please help review again. I'm sorry that my changes made this piece of code so complicated and caused so many regressions. Hope it's the last CL to fix regressions. Regards James Su
http://codereview.chromium.org/172041/diff/2001/2002 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/172041/diff/2001/2002#newcode609 Line 609: char_inserted_ = 0; This would seem to indicate there was a bug in your previous logic. This was probably not a false positive, and I don't really like this fix or the comment. Can you figure out what was wrong with the previous code?
Hi, I just updated the CL. Control key support has been added. Regards James Su http://codereview.chromium.org/172041/diff/2001/2002 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/172041/diff/2001/2002#newcode609 Line 609: char_inserted_ = 0; On 2009/08/19 03:41:33, Dean McNamee wrote: > This would seem to indicate there was a bug in your previous logic. This was > probably not a false positive, and I don't really like this fix or the comment. > Can you figure out what was wrong with the previous code? In the previous code, new_line_inserted and tab_inserted is initialized outside if block, so they might be initialized with an uninitialized char_inserted_ variable, if tab_pressed and enter_pressed are both false. However in this case, new_line_inserted and tab_inserted won't be used at all because tab_pressed and enter_pressed will be checked first. Seems that in the new code, this line can be moved back to above if block without any problem, because new_line_inserted and tab_inserted are removed.
Hi, Let me explain the reason of this CL again: As I said before, a key event must be sent to GtkTextView before handling it in our code. A key event can only be handled if GtkTextView didn't handle it. But for Tab and Enter key, sending them to GtkTextView may alter the text content even if they are not handled by IME. Before this CL, I used backup and restore trick to prevent the content from being changed. But I found that it doesn't work for Tab key, because backup/restore trick may change the keyboard hint status, which then may change the behavior of Tab key from changing focus to enabling search. In this new CL, property "accepts-tab" of |text_view_| is disabled, so that Tab key won't alter the content anymore. Instead, "move-focus" signal will be emitted by GtkTextView, when a Tab key is not handled by IME. So that by intercepting "move-focus" signal, we can know if a Tab key press event is handled by IME without altering the content. And Tab to search feature can be triggered in this signal handler. Regards James Su On 2009/08/19 05:12:23, James Su wrote: > Hi, > I just updated the CL. Control key support has been added. > > Regards > James Su > > http://codereview.chromium.org/172041/diff/2001/2002 > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): > > http://codereview.chromium.org/172041/diff/2001/2002#newcode609 > Line 609: char_inserted_ = 0; > On 2009/08/19 03:41:33, Dean McNamee wrote: > > This would seem to indicate there was a bug in your previous logic. This was > > probably not a false positive, and I don't really like this fix or the > comment. > > Can you figure out what was wrong with the previous code? > > In the previous code, new_line_inserted and tab_inserted is initialized outside > if block, so they might be initialized with an uninitialized char_inserted_ > variable, if tab_pressed and enter_pressed are both false. However in this case, > new_line_inserted and tab_inserted won't be used at all because tab_pressed and > enter_pressed will be checked first. > > Seems that in the new code, this line can be moved back to above if block > without any problem, because new_line_inserted and tab_inserted are removed.
I didn't check all of the details, but overall I understand what you're doing, and I guess it is what we need to do for now. A few small comments, after these I think it will be ok. http://codereview.chromium.org/172041/diff/2009/3002 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/172041/diff/2009/3002#newcode624 Line 624: if (enter_pressed && (char_inserted_ == '\n' || char_inserted_ == '\r')) { I like it like this (compared to the old code). http://codereview.chromium.org/172041/diff/2009/3002#newcode662 Line 662: model_->OnControlKeyChanged(false); This isn't actually correct, is it? What if I do. left control down right control down right control up I still have control down, don't I? http://codereview.chromium.org/172041/diff/2009/3003 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.h (right): http://codereview.chromium.org/172041/diff/2009/3003#newcode398 Line 398: // Indicates if Tab key is currently pressed. Not if it is pressed, if it was pressed? We reset it somewhere (not on key up?). Maybe you should name this |tab_was_pressed_|, and indicate that it's only used in the key press handler, to detect a tab during sync dispatch of move-focus?
http://codereview.chromium.org/172041/diff/1004/5 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/172041/diff/1004/5#newcode343 Line 343: gtk_text_buffer_select_range(text_buffer_, &insert, &bound); On 2009/08/18 02:13:54, James Su wrote: > On 2009/08/17 20:33:50, Daniel Erat wrote: > > this looks like it will update the PRIMARY X selection. if this isn't what > the > > user expects, you should call StartUpdatingHighlightedText() before and > > FinishUpdatingHighlightedText() after so the X selection won't be updated. > > Yes, it will update the PRIMARY x selection. However I think > {Start|Finish}Updating... shall be called by caller of SetTextAndSelectedRange() > rather than here. > The old code of OnTemporaryTextMaybeChanged() didn't call {Start|Finish}... > around SetWindowTextAndCaretPos(), so I'm wondering if it's necessary to to call > them in both OnTemporaryTextMaybeChanged() and OnRevertTemporaryText(). > > What's your opinion? Depends on when it gets called. :-) But the old code just used gtk_text_buffer_place_cursor(), which I don't think updates PRIMARY, and you're using gtk_text_buffer_select_range(), which does. When does this get used, when I start using the Up and Down keys to highlight omnibox suggestions and then hit Escape? If so, you probably shouldn't update PRIMARY, since (I think) the text could've been highlighted via autocomplete, which we don't want to update PRIMARY. I'll play around with this change locally to test this, but would appreciate any advice you can give about when your new code is triggered (autocomplete_edit.cc is not proving very inviting).
On Thu, Aug 20, 2009 at 5:03 PM, <derat@chromium.org> wrote: > Depends on when it gets called. :-) =A0But the old code just used > gtk_text_buffer_place_cursor(), which I don't think updates PRIMARY, and > you're using gtk_text_buffer_select_range(), which does. > > When does this get used, when I start using the Up and Down keys to > highlight omnibox suggestions and then hit Escape? =A0If so, you probably > shouldn't update PRIMARY, since (I think) the text could've been > highlighted via autocomplete, which we don't want to update PRIMARY. > I'll play around with this change locally to test this, but would > appreciate any advice you can give about when your new code is triggered > (autocomplete_edit.cc is not proving very inviting). Yeah, there are issues with this in its current state, where you don't wrap calls to this in the methods to avoid updating the PRIMARY selection. For example: 1. Highlight some text in an xterm to update PRIMARY 2. Hit Ctrl-L and start typing "www.google.com" in the omnibox 3. Inline autocomplete should kick in, but PRIMARY is untouched 4. Hit down a few times and then Escape 5. The highlighted autocomplete text is restored, but now the xterm loses PRIMARY and you end up with "oogle.com/" or whatever in it > http://codereview.chromium.org/172041 >
Hi, CL has been updated. Please help review. I think it should be ok to keep the control key handling as it, which is in sync with Windows version. Regards James Su http://codereview.chromium.org/172041/diff/2009/3002 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/172041/diff/2009/3002#newcode662 Line 662: model_->OnControlKeyChanged(false); Though it's not actually correct, Chrome on Windows has almost the same behavior. The only difference is Windows will send ctrl press event repeatedly if you hold ctrl down, while Linux won't. On 2009/08/20 18:09:21, Dean McNamee wrote: > This isn't actually correct, is it? What if I do. > > left control down > right control down > right control up > > I still have control down, don't I?
Thanks, I think this'll work. I'll give it a try tomorrow morning. On 2009/08/21 02:56:58, James Su wrote: > Hi, > CL has been updated. Please help review. I think it should be ok to keep the > control key handling as it, which is in sync with Windows version. > > Regards > James Su > > http://codereview.chromium.org/172041/diff/2009/3002 > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): > > http://codereview.chromium.org/172041/diff/2009/3002#newcode662 > Line 662: model_->OnControlKeyChanged(false); > Though it's not actually correct, Chrome on Windows has almost the same > behavior. The only difference is Windows will send ctrl press event repeatedly > if you hold ctrl down, while Linux won't. > > On 2009/08/20 18:09:21, Dean McNamee wrote: > > This isn't actually correct, is it? What if I do. > > > > left control down > > right control down > > right control up > > > > I still have control down, don't I?
Just sync with the latest trunk, no other change. Regards James Su
LGTM Thanks, the PRIMARY selection works as I'd expect in this latest version. On Fri, Aug 21, 2009 at 3:28 AM, <suzhe@chromium.org> wrote: > Just sync with the latest trunk, no other change. > > Regards > James Su > > http://codereview.chromium.org/172041 >
LGTM. I still think our ctrl handling is wrong. Even though Windows has similar logic, since it is getting repeated key presses for ctrl it's less of an issue. It is currently incorrect on Linux, we will show and do different things. I think this needs to be addressed, but you can do it in a follow up patch. Thanks http://codereview.chromium.org/172041/diff/2021/2022 File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/172041/diff/2021/2022#newcode666 Line 666: model_->OnControlKeyChanged(false); I am still not happy with this code, but I will let you fix it in a follow up.
Ok, I'll look into the Control issue later. Thanks James Su On 2009/08/21 18:21:34, Dean McNamee wrote: > LGTM. > > I still think our ctrl handling is wrong. Even though Windows has similar > logic, since it is getting repeated key presses for ctrl it's less of an issue. > It is currently incorrect on Linux, we will show and do different things. I > think this needs to be addressed, but you can do it in a follow up patch. > > Thanks > > http://codereview.chromium.org/172041/diff/2021/2022 > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): > > http://codereview.chromium.org/172041/diff/2021/2022#newcode666 > Line 666: model_->OnControlKeyChanged(false); > I am still not happy with this code, but I will let you fix it in a follow up. |