GNOME Bugzilla – Bug 675220
Avoid repeatedly reloading icons in list views
Last modified: 2012-09-12 13:53:38 UTC
If you bind a cell renderer's icon-name property to something, then apparently it reloads the image from the icon theme constantly. This makes several tree views and dropdowns very sluggish, even on speedy computers with SSDs. http://git.gnome.org/browse/empathy/commit/?id=b0bd73c fixed this for one account chooser widget, but sadly Empathy has a number of widgets with this bug, several of which are also account choosers: • libempathy-gtk/empathy-account-selector-dialog.c: used exclusively by the “which account do you want to use to call this phone number?” dialog as far as I can tell. • src/empathy-accounts-dialog.c: its treeview ends up repeatedly loading both the protocol icons and the presence icons! • libempathy-gtk/empathy-protocol-chooser.c: same bug, but for available protocols. • probably others. I got bored of grepping :) I can't find an easy way to trigger the first dialog, but I have an untested fix for it I will attach. The second and third cases are noticeably sluggish on my not-underpowered laptop with an SSD, so could do with being fixed.
Created attachment 213177 [details] [review] AccountSelectorDialog: avoid repeatedly reloading icons (It seems remarkable that this dialog needs yet another implementation of an account selector.)
Review of attachment 213177 [details] [review]: empathy-account-selector-dialog.c:84:7: warning: implicit declaration of function 'empathy_pixbuf_from_icon_name' [-Wimplicit-function-declaration] Include empathy-ui-utils.h and please merge to gnome-3-4. I'll merge it back to master myself. thanks!
(In reply to comment #0) > • src/empathy-accounts-dialog.c: its treeview ends up repeatedly loading both > the protocol icons and the presence icons! I changed to use the pixbuf directly but we should cache them. > • libempathy-gtk/empathy-protocol-chooser.c: same bug, but for available > protocols. fixed.
Created attachment 213182 [details] [review] accounts-dialog: set pixbuf on the cell rather than the icon-name
Created attachment 213183 [details] [review] protocol-chooser: set pixbuf on the cell rather than the icon-name
Created attachment 213188 [details] [review] accounts-dialog: add icons cache Saves us from loading the same icons from the disk again and again.
Review of attachment 213182 [details] [review]: ::: src/empathy-accounts-dialog.c @@ +1179,2 @@ g_object_set (cell, + "pixbuf", pixbuf, This cannot be right? This is the pixbuf data func, and instead of loading the icon from disk by giving the icon name... you're loading a pixbuf from the icon name...? Surely this is exactly as bad, if not worse?
Review of attachment 213183 [details] [review]: This looks fine.
Review of attachment 213188 [details] [review]: ::: src/empathy-accounts-dialog.c @@ +1200,2 @@ icon_name = get_status_icon_for_account (dialog, account); + pixbuf = ensure_icon (dialog, icon_name); g_object_set takes a ref no? ensure_icon returns a new ref to the pixbuf and you give it to the cell renderer, but then you don't unref it? perhaps you should just make ensure_icon not return a new ref? @@ +1231,2 @@ icon_name = empathy_account_settings_get_icon_name (settings); + pixbuf = ensure_icon (dialog, icon_name); Same thing here.
Comment on attachment 213183 [details] [review] protocol-chooser: set pixbuf on the cell rather than the icon-name Attachment 213183 [details] pushed as 8430406 - protocol-chooser: set pixbuf on the cell rather than the icon-name
Review of attachment 213188 [details] [review]: The pixbuf is unrefed afterward in both cases. This code didn't change so it doesn't appear in the diff.
Review of attachment 213182 [details] [review]: ::: src/empathy-accounts-dialog.c @@ +1179,2 @@ g_object_set (cell, + "pixbuf", pixbuf, That's just the first part. The 3rd commit adds a cache to avoid re-loading the icon.
gogogogogogogogo
Attachment 213182 [details] pushed as 813d73b - accounts-dialog: set pixbuf on the cell rather than the icon-name Attachment 213188 [details] pushed as 279310f - accounts-dialog: add icons cache