GNOME Bugzilla – Bug 737841
Convert the searchPopup to use GtkListBox
Last modified: 2014-11-21 06:43:55 UTC
Lets be a bit modern and use the GtkListBox widget (added in 3.10) in our searchPopup instead of the model-view based treeView. It is pretty hard to style a treeview. So we would need to create an ui file for a search result row, and add them to the listbox in updateResult. Maybe it can even look pretty?
Created attachment 290698 [details] [review] Convert SearchPopup to GtkListBox
Created attachment 290699 [details] Cast of new popup
looks good!
Review of attachment 290698 [details] [review]: Nice! My comments below. ::: src/placeFormatter.js @@ +56,3 @@ + }).bind(this)).join(', '); + }).bind(this)).join(', '); + }, getRowsAsString? not sure though. Or getRowsSeparatedByACommaCharacter :-| ::: src/search-popup-row.ui @@ +26,3 @@ + </child> + <child> + <object class="GtkImage" id="type-icon"> Maybe typeIcon to follow the new style. ::: src/searchPopup.js @@ +58,3 @@ + this._details.max_width_chars = maxChars; + this._details.label = formatter.toString(); + this._icon.icon_name = this.place.icon.to_string(); Maybe this._icon.gicon = this.place.icon. @@ +98,3 @@ + if (row) + this.emit('selected', row.place); + }).bind(this)); How this affect to keyboard navigation? because once you select something with keyboard, that row is activated. Maybe a better way to do this is to use 'row-activated' signal and set 'activate-on-single-click' property to True. @@ +133,3 @@ + this._list.forall(function(row) { + row.destroy(); + }); Good to know, we are doing listBox.forall(listBox.remove.bind(listBox)); in sidebar.js and I was copying it to my check-in code. Perhaps Gtk+ deserves a "clear" method :) @@ +141,3 @@ + searchString: searchString, + maxChars: this._maxChars }); + this._list.add(row); This is the best part of not use treeview ;)
Review of attachment 290698 [details] [review]: Thanks for review! ::: src/placeFormatter.js @@ +56,3 @@ + }).bind(this)).join(', '); + }).bind(this)).join(', '); + }, :) "There are only two hard problems in Computer Science: cache invalidation and naming things." -- Phil Karlton How about getDetails? ::: src/search-popup-row.ui @@ +26,3 @@ + </child> + <child> + <object class="GtkImage" id="type-icon"> Yes, and also I think this file should be place-list-row.ui and it could be reusued. ::: src/searchPopup.js @@ +58,3 @@ + this._details.max_width_chars = maxChars; + this._details.label = formatter.toString(); + this._icon.icon_name = this.place.icon.to_string(); Yep! @@ +98,3 @@ + if (row) + this.emit('selected', row.place); + }).bind(this)); Yeah that sounds sane, thanks. @@ +133,3 @@ + this._list.forall(function(row) { + row.destroy(); + }); Yeah, feel free to open a bug with a patch against gtkcontainer :) @@ +141,3 @@ + searchString: searchString, + maxChars: this._maxChars }); + this._list.add(row); (o/
Created attachment 290891 [details] [review] Convert SearchPopup to GtkListBox
Created attachment 290990 [details] [review] Convert SearchPopup to GtkListBox