GNOME Bugzilla – Bug 706715
Add icons to search results in search popup
Last modified: 2013-08-27 23:34:43 UTC
Put icons in the search popup, if geocode-glib adds support for it.
Created attachment 253016 [details] [review] Add icons to search popup This patch depends on gobject-introspection being updated to incorporate the fix in: https://bugzilla.gnome.org/show_bug.cgi?id=706706 So it's possible to use g_icon_loadable_load_finish() in gjs.
Created attachment 253032 [details] [review] Add icons to search popup v2 Fix up padding some, to align icons better with magnifying glass in search entry.
Created attachment 253033 [details] [review] Add icons to search popup v2 [file async] And this patch avoids icon load_async and uses file read_async as a workaround.
Note: these patches are based on a repository where bug#706635 and bug#706636 are applied.
The Gir annotation updates have landed in gobject-introspection master now. In the glib bug linked Colin says that "[it] Will be in the 1.38.0 release."
None of the patches apply on current git master. Do they depend on other bugs? If not, please rebase them.
Yes, see comment above yours having pixbuf changes howto calculate cell height for scroll so I wanted this after scrollable. Can rebase against master and rebase scrollable popup against this tomorrow if you prefer. Sorry for confusion!
(In reply to comment #7) > Yes, see comment above yours having pixbuf changes howto calculate cell height > for scroll so I wanted this after scrollable. Can rebase against master and > rebase scrollable popup against this tomorrow if you prefer. Sorry for > confusion! Nah, just need to add those patches in 'depends' here.
Comment on attachment 253033 [details] [review] Add icons to search popup v2 [file async] With bug#706706 resolved, we don't need this.
Review of attachment 253032 [details] [review]: Looks good otherwise, especially the result. :) ::: src/mainWindow.js @@ +88,3 @@ _initSearchWidgets: function() { this._searchPopup = new SearchPopup.SearchPopup(10); + this._iconStore = {}; By cache, I meant a more permanent cache. :) However we can adding permanent caching later but please add to your todo already. @@ +263,3 @@ + let pixbuf; + + if (icon instanceof Gio.FileIcon) { I think these different implementations are better off in separate functions. @@ +264,3 @@ + + if (icon instanceof Gio.FileIcon) { + pixbuf = this._iconStore[icon.get_file().get_uri()]; This would be one place where use of properties is slightly cleaner: icon.file.get_uri().
(In reply to comment #10) > Review of attachment 253032 [details] [review]: > > Looks good otherwise, especially the result. :) > > ::: src/mainWindow.js > @@ +88,3 @@ > _initSearchWidgets: function() { > this._searchPopup = new SearchPopup.SearchPopup(10); > + this._iconStore = {}; > > By cache, I meant a more permanent cache. :) However we can adding permanent > caching later but please add to your todo already. Ok! > > @@ +263,3 @@ > + let pixbuf; > + > + if (icon instanceof Gio.FileIcon) { > > I think these different implementations are better off in separate functions. I created load_icon in Utils and split them up there. This way we only need to do model.set once. > > @@ +264,3 @@ > + > + if (icon instanceof Gio.FileIcon) { > + pixbuf = this._iconStore[icon.get_file().get_uri()]; > > This would be one place where use of properties is slightly cleaner: > icon.file.get_uri(). Agreed.
Created attachment 253223 [details] [review] Add icons to search popup v3 * use file property instead of get_file() * move icon loading to Utils and split up to different functions per instance type.
Created attachment 253224 [details] [review] Add icons to search popup v4 I should not submit patches before coffee. * actually use _themed_icons function * no use for Gio in mainwindow anymore.
Review of attachment 253224 [details] [review]: Looks good otherwise. ::: src/utils.js @@ +170,3 @@ +} + +function load_icon(icon, size, loadCompleteCallback) { nitpick: private functions below public ones please.
I took the liberty of making the minor adjustment for you.