Refer to icons by names - #257
Conversation
mvo5
left a comment
mvo5
left a comment
There was a problem hiding this comment.
Thank you! I love the direction, not having to do this manually anymore is great. Some small inline comments.
| GdkPixbuf *pix; | ||
| pix = RGPackageStatus::pkgStatus.getPixbuf(pkg); | ||
| g_value_set_object(value, pix); | ||
| std::string icon_name = RGPackageStatus::pkgStatus.getIconName(pkg); |
There was a problem hiding this comment.
So gtk_pkg_list_get_value() gets called a lot (really a lot at least when I wrote it maybe gtk changed and its less so now idk). So as written this is a new std::string allocation per call, I think we don't need this, we could have something like std::string names[N_STATUS_COUNT] (similar to the previous StatusPixbuf array) and then just return "const char *" into that. This would avoid the allocation make it very cheap again. Normally I would not care about one allocation but this is (or used to be) a very hot path.
There was a problem hiding this comment.
I've changed this to pre-allocated strings. BTW, this is a hot path because GtkTreeModel eagerly creates all objects, Gtk4 has GtkColumnView which renders just a visible part of the list so, reduces allocations to a minimum.
There was a problem hiding this comment.
Ohhhhh, I love to hear that this is better now in gtk4, I had a really hard time with the old way (it was like this from gtk1-3 iirc).
| return false; | ||
| } | ||
| gtk_image_set_from_pixbuf(GTK_IMAGE(pix), value); | ||
| gtk_image_set_from_icon_name(GTK_IMAGE(pix), value, GTK_ICON_SIZE_BUTTON); |
There was a problem hiding this comment.
I'm not sure but I think this changes the icon size, the old code was using 22px (initPixbufs) and this is 16px now(?). But maybe we always scaled it down already so it does not matter, idk. Maybe worth a comment in the commit message.
There was a problem hiding this comment.
Icons are 16x16 so, no visible change here.
There was a problem hiding this comment.
If you mean "tango-icons" which are indeed 22x22, then I don't see a way why they should be displayed. I see "hicolor" ones all the time. They seem to not be used anywhere in the code.
2 participants
Do not load icons into pixbufs and allow Gtk to manage them.
The code is back-ported from Gtk4 branch #200
Supersedes #252