After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 706715 - Add icons to search results in search popup
Add icons to search results in search popup
Status: RESOLVED FIXED
Product: gnome-maps
Classification: Applications
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-maps-maint
gnome-maps-maint
Depends on: 706291 706635 706636
Blocks: 706891
 
 
Reported: 2013-08-24 16:07 UTC by Jonas Danielsson
Modified: 2013-08-27 23:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add icons to search popup (5.22 KB, patch)
2013-08-24 16:11 UTC, Jonas Danielsson
none Details | Review
Add icons to search popup v2 (5.23 KB, patch)
2013-08-24 23:47 UTC, Jonas Danielsson
needs-work Details | Review
Add icons to search popup v2 [file async] (5.26 KB, patch)
2013-08-24 23:53 UTC, Jonas Danielsson
none Details | Review
Add icons to search popup v3 (5.26 KB, patch)
2013-08-27 07:26 UTC, Jonas Danielsson
none Details | Review
Add icons to search popup v4 (5.16 KB, patch)
2013-08-27 07:32 UTC, Jonas Danielsson
committed Details | Review

Description Jonas Danielsson 2013-08-24 16:07:52 UTC
Put icons in the search popup, if geocode-glib adds support for it.
Comment 1 Jonas Danielsson 2013-08-24 16:11:29 UTC
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.
Comment 2 Jonas Danielsson 2013-08-24 23:47:26 UTC
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.
Comment 3 Jonas Danielsson 2013-08-24 23:53:09 UTC
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.
Comment 4 Jonas Danielsson 2013-08-25 00:06:23 UTC
Note: these patches are based on a repository where bug#706635 and bug#706636 are applied.
Comment 5 Jonas Danielsson 2013-08-26 08:23:12 UTC
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."
Comment 6 Zeeshan Ali 2013-08-26 16:48:20 UTC
None of the patches apply on current git master. Do they depend on other bugs? If not, please rebase them.
Comment 7 Jonas Danielsson 2013-08-26 18:48:17 UTC
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!
Comment 8 Zeeshan Ali 2013-08-26 19:42:05 UTC
(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 9 Zeeshan Ali 2013-08-26 22:47:25 UTC
Comment on attachment 253033 [details] [review]
Add icons to search popup v2 [file async]

With bug#706706 resolved, we don't need this.
Comment 10 Zeeshan Ali 2013-08-26 23:08:23 UTC
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().
Comment 11 Jonas Danielsson 2013-08-27 07:25:10 UTC
(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.
Comment 12 Jonas Danielsson 2013-08-27 07:26:15 UTC
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.
Comment 13 Jonas Danielsson 2013-08-27 07:32:38 UTC
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.
Comment 14 Zeeshan Ali 2013-08-27 23:25:00 UTC
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.
Comment 15 Zeeshan Ali 2013-08-27 23:34:39 UTC
I took the liberty of making the minor adjustment for you.