GNOME Bugzilla – Bug 706636
Add scrolled window to SearchPopup
Last modified: 2013-08-27 23:34:04 UTC
If we want to display more results than we can comfortably show in the popup, we should have a ScrolledWindow.
Created attachment 252816 [details] [review] SearchPopup: add scrolled window
Created attachment 252817 [details] [review] SearchPopup: add scrolled window V2 Some spillage from future stack/spinner work :)
Review of attachment 252817 [details] [review]: Looks good otherwise ::: src/searchPopup.js @@ +92,3 @@ }, + showResults: function() { why not just override show?
(In reply to comment #3) > Review of attachment 252817 [details] [review]: > > Looks good otherwise > > ::: src/searchPopup.js > @@ +92,3 @@ > }, > > + showResults: function() { > > why not just override show? My thinking was because when I add the spinner there will be another way of showing the search popup. Either you show in when you want to show results or when you want to show the spinner. But since the spinner isn't here, maybe override show is the way to go.
Created attachment 252880 [details] [review] SearchPopup: Add scrolled window v3
Review of attachment 252880 [details] [review]: ::: src/searchPopup.js @@ +92,3 @@ }, + show: function() { Don't you want to chain-up?
Created attachment 252895 [details] [review] SearchPopup: Add scrolled window v4
Created attachment 252952 [details] [review] SearchPopup: Add scrolled window v5 Needed to define treeView before chaining up, or treeView would be undefined on the first show call.
Created attachment 252983 [details] [review] SearchPopup: Add scrolled window v6 Remove silly new newline
Review of attachment 252983 [details] [review]: Looks good otherwise. ::: src/mainWindow.js @@ -252,3 @@ location]); }); - this._searchPopup.setModel(model); Why is this call getting removed?
(In reply to comment #10) > Review of attachment 252983 [details] [review]: > > Looks good otherwise. > > ::: src/mainWindow.js > @@ -252,3 @@ > location]); > }); > - this._searchPopup.setModel(model); > > Why is this call getting removed? Well, it does nothing. We set the model in _initSearchWidgets and then just clear it/update it when searching. The call to set_model on the gtk_tree_view will just return if the model you are trying to set is the same as the one currently set. But. It might not belong in this bug. We could have a separate patch for it.
Created attachment 253221 [details] [review] SearchPopup: add scrolled window v7 * Does not remove setting of search popup model after searching.
Created attachment 253222 [details] [review] MainWindow: No need to reset model on SearchPopup
Review of attachment 253222 [details] [review]: ::: src/mainWindow.js @@ -252,3 @@ location]); }); - this._searchPopup.setModel(model); Thanks for separating it out. Does anything use SearchPopup.setModel() then? If not, please remove it in this patch.
Review of attachment 253221 [details] [review]: ACK
(In reply to comment #14) > Review of attachment 253222 [details] [review]: > > ::: src/mainWindow.js > @@ -252,3 @@ > location]); > }); > - this._searchPopup.setModel(model); > > Thanks for separating it out. Does anything use SearchPopup.setModel() then? If > not, please remove it in this patch. Yes it is used in _initSearchWidgets to set the newly created model that we later add and remove search results to.
Review of attachment 253222 [details] [review]: ack
Attachment 253222 [details] pushed as e44df61 - MainWindow: No need to reset model on SearchPopup