GNOME Bugzilla – Bug 731545
Move out the initialization of the searchPopup
Last modified: 2014-06-20 02:21:08 UTC
and searchEntry to a separate class called PlaceEntry and use that instead. This would be nice for the routing patches, but please don't make one block on the other. I'll happily rebase either one if the other gets merged first.
Created attachment 278307 [details] [review] SearchPopup: forward popover properties Take in a full properties object and forward that to GtkPopover instead of hardcoding the properties to pass. This makes searchPopup a bit more flexible in other settings.
Created attachment 278308 [details] [review] Add PlaceEntry, a SearchEntry for places PlaceEntry encapsulates all the search entry stuff in mainWindow in a reusable way.
Created attachment 278309 [details] [review] MainWindow: use PlaceEntry Move over to using PlaceEntry in the HeaderBar.
Review of attachment 278308 [details] [review]: ::: src/placeEntry.js @@ +18,3 @@ + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + * Author: Mattias Bengtsson <mattias.jc.bengtsson@gmail.com> This and the copyright notice should mention you too Jonas. Just realized.
Review of attachment 278309 [details] [review]: Nice refactor :) ::: src/mainWindow.js @@ +56,1 @@ 'search-completion', Does the search-completion still needs to be in mainWindow? @@ +74,3 @@ + let placeEntry = this._createPlaceEntry(ui.searchCompletion) + ui.headerBar.set_custom_title(placeEntry); + placeEntry.has_focus = true; Can't this be set when creating the PlaceEntry? has_focus: true ? @@ +86,3 @@ + _createPlaceEntry: function(completion) { + completion.set_model(this._placeStore); + completion.set_match_func(PlaceStore.completionMatchFunc); The completion stuff can be done from place-entry.ui / placeEntry.js?
Review of attachment 278307 [details] [review]: looks fine.
Review of attachment 278308 [details] [review]: I like it! ::: src/place-entry.ui @@ +21,3 @@ + </child> + </object> +</interface> Ok so you create this file, but do not add it or use it. I guess you forgot? We should be able to not have the completion in main-window.ui right? And instead have it on the entry, avoiding duplication? ::: src/placeEntry.js @@ +30,3 @@ +const SearchPopup = imports.searchPopup; + +const PlaceEntry = new Lang.Class({ Should it be PlaceSearchEntry? Not sure.
Review of attachment 278309 [details] [review]: Thanks! :) ::: src/mainWindow.js @@ +56,1 @@ 'search-completion', No, it should probably just be created in code, since that's the only gtkbuilder-stuff left anyways. @@ +74,3 @@ + let placeEntry = this._createPlaceEntry(ui.searchCompletion) + ui.headerBar.set_custom_title(placeEntry); + placeEntry.has_focus = true; Yeah I tried that, but apparently setting has_focus before the child is added to a parent does nothing. @@ +86,3 @@ + _createPlaceEntry: function(completion) { + completion.set_model(this._placeStore); + completion.set_match_func(PlaceStore.completionMatchFunc); Yeah, I'll just create it in code instead.
Review of attachment 278308 [details] [review]: ::: src/place-entry.ui @@ +21,3 @@ + </child> + </object> +</interface> Yeah, either actually use this or just construct it in code. I'll fix this. ::: src/placeEntry.js @@ +30,3 @@ +const SearchPopup = imports.searchPopup; + +const PlaceEntry = new Lang.Class({ Yeah, makes sense.
Created attachment 278380 [details] [review] Add PlaceEntry, a SearchEntry for places PlaceEntry encapsulates all the search entry stuff in mainWindow in a reusable way. - Construct the completion inside placeentry (via ui-file since it was easier). - Some small cleanups
Review of attachment 278308 [details] [review]: ::: src/placeEntry.js @@ +30,3 @@ +const SearchPopup = imports.searchPopup; + +const PlaceEntry = new Lang.Class({ Actually I think we can just go with the shorter name. It's easy enough to understand and I hate reading code with unnecessarily long function- / variable names. Will change if you decide it's important though. :)
Created attachment 278381 [details] [review] MainWindow: use PlaceEntry Move over to using PlaceEntry in the HeaderBar. - Move the completion code to placeEntry
Created attachment 278383 [details] [review] Add PlaceEntry, a SearchEntry for places PlaceEntry encapsulates all the search entry stuff in mainWindow in a reusable way. - Tweak alignment between searchEntry and popover.
Created attachment 278466 [details] [review] MainWindow: use PlaceEntry Move over to using PlaceEntry in the HeaderBar. - let the PlaceEntry constructor only take a props object.
Created attachment 278508 [details] [review] Add PlaceEntry, a SearchEntry for places PlaceEntry encapsulates all the search entry stuff in mainWindow in a reusable way. - refine the width alignment, use the real allocation instead of the preferred width/height.
Review of attachment 278466 [details] [review]: (o/
Review of attachment 278508 [details] [review]: \o)
Attachment 278307 [details] pushed as ef095d2 - SearchPopup: forward popover properties Attachment 278466 [details] pushed as 8d3a73b - MainWindow: use PlaceEntry Attachment 278508 [details] pushed as 09d1dd3 - Add PlaceEntry, a SearchEntry for places
\o/