GNOME Bugzilla – Bug 642502
clean up search entry event processing a bit
Last modified: 2011-02-24 14:38:13 UTC
some work towards making the search entry less of a weirdo special case, as part of general overview keynav. if you don't like the first patch, I already have an alternate set of patches without it
Created attachment 181043 [details] [review] viewSelector: merge SearchEntry into SearchTab The division of labor between the two was quite muddled. Rather than try to invent a clean distinction of what belongs where, just merge them together.
Created attachment 181044 [details] [review] viewSelector: remove the search entry's event grab The search entry was taking a sort of grab when it was in the focused-but-empty state, and would eat up most events for other actors (except, confusingly, for panel actors). The only bit of "modality" we really need here is that the entry is supposed to go back to the unfocused state if you click somewhere outside it when it was in the focused-but-empty state. So do that.
Created attachment 181045 [details] [review] viewSelector: don't process key presses from capture-event Rather than connecting to stage::capture-event and then trying to guess whether or not a given key-press should be handled by us or not, handle the end-search-on-Escape case from entry::key-press-event (since it only makes sense when the entry is focused anyway) and the start-search-on-printable-key case from stage::key-press-event (which will only get the events that no other actor wanted for itself). Similarly, do the exit-overview-on-Escape check from stage::key-press-event, rather than viewSelector.actor::key-press-event, so that it will work correctly even if the keyboard focus is somewhere else.
Review of attachment 181043 [details] [review]: The patch needs to be rebased on some changes in master. I'll attach a rebased version later if you don't mind. ::: js/ui/viewSelector.js @@ +98,3 @@ this._openSearchSystem = new Search.OpenSearchSystem(); + this._actor = new St.Entry({ name: 'searchEntry', I don't quite like this._actor - it's not really the search tab's "actor", just its title. Maybe make this _entry and rename this._entry to _text?
Review of attachment 181044 [details] [review]: It's a change of the designed behavior, but makes a lot of sense ...
(In reply to comment #4) > The patch needs to be rebased on some changes in master. I'll attach a rebased > version later if you don't mind. No need, I've already rebased it locally. > + this._actor = new St.Entry({ name: 'searchEntry', > > I don't quite like this._actor - it's not really the search tab's "actor", just > its title. Maybe make this _entry and rename this._entry to _text? Sure, makes sense.
Created attachment 181735 [details] [review] viewSelector: merge SearchEntry into SearchTab rebased
Created attachment 181736 [details] [review] viewSelector: remove the search entry's event grab rebased
Created attachment 181737 [details] [review] viewSelector: don't process key presses from capture-event rebased
Review of attachment 181045 [details] [review]: Code looks good, but there's one change in behavior I don't like - when clicking into the entry and hitting Escape (without entering a search term), the overview is left (instead of the current behavior of simply moving focus out of the entry). The reason for this is that the key-press handler of the entry is connected in SearchTab.show(), e.g. only when a search term is entered (and search results are displayed). I can't think of a good reason to not set up the signal handler in the constructor and leave it connected, which makes the entry behave as expected in that case.
Review of attachment 181735 [details] [review]: One additional comment with regard to the previous review: ::: js/ui/viewSelector.js @@ +102,3 @@ + * in the search entry when no search is + * active; it should not exceed ~30 + * characters. The comment style was actually weird on purpose, as the comment will show up as Translators: this is the text displayed * in the search entry when no search is * active; ... when formatted like that. (Feel free to ignore, it just looked a bit ugly to me)
Review of attachment 181736 [details] [review]: No changes wrt the previous review.
Review of attachment 181737 [details] [review]: Dto.
Created attachment 181753 [details] [review] viewSelector: don't process key presses from capture-event updated for comments
Review of attachment 181753 [details] [review]: Looks good.
Review of attachment 181753 [details] [review]: I just noticed that the patch breaks ctrl-pageUp/pageDown - as now the key focus is either on the stage or the search entry, the key-press handler for the view selector is pretty much useless. Can you merge it with the stage's key-press handler?
pushed with suggested fix Attachment 181735 [details] pushed as 5b8d3ba - viewSelector: merge SearchEntry into SearchTab Attachment 181736 [details] pushed as 3755783 - viewSelector: remove the search entry's event grab Attachment 181753 [details] pushed as d573549 - viewSelector: don't process key presses from capture-event