*** Bug 609133 has been marked as a duplicate of this bug. ***

Created attachment 153120 [details] [review]
change to load some icons lazily

The change reduces the cpu use by half on login when not autoconnecting or using an ethernet wired connection, it probably needs some extra tweaking but I wanted to discuss the approch there before continuing on that. Do you think that makes sense? Should it be done for every icons?

It should probably be done for most icons except for at least a "fallback" icon.  Sometimes icon loading *does* fail, if the user forgets to run gtk-update-icon-cache or whatever, and then we need to make sure we display something in the notification area even if it's some sort of error icon I guess.

I'd modify a few things:

1) find some suitable fallback icon and load that during applet init, and only if loading this fallback icon fails, show the warning dialog and quit.  Move the warning dialog back to applet init and do a:

if (   !applet->no_connection_icon
    || !applet->wired_icon
    || !applet->adhoc_icon
   ...
    <show warning dialog and quit>

and then maybe modify the nma_icon_load() function to take a "gboolean fallback" argument that if the load fails, return the fallback icon instead.

This also means getting rid of applet->icons_loaded (yay!) since it's useless in this scenario.


1) we should actually check each icon and load it if (NULL || icon == fallback_icon).  There could be cases where an icon load fails, but with the existing patch it would never get retried.  To handle this more easily, I'd suggest changing nma_icon_load() to be more like:

gboolean
nma_icon_check_and_load (const char *name, GdkPixbuf **icon, NMApplet *applet, gboolean fallback);
{
    GdkPixbuf *pixbuf;
    GError *error = NULL;

    g_return_val_if_fail (name != NULL, FALSE);
    g_return_val_if_fail (icon != NULL, FALSE);
    g_return_val_if_fail (applet != NULL, FALSE);

    /* icon already loaded successfully */
    if (*icon && *icon != applet->fallback_icon)
        return TRUE;

    *icon = gtk_icon_theme_load_icon (applet->icon_theme, name, applet->icon_size, 0, &error);
    if (!*icon) {
        <print error->code && error->message>
        if (fallback)
            *icon = applet->fallback_icon;
    }
    return *icon ? TRUE : FALSE;
}

Then in the applet init code where we want to ensure we have the icons, we do:

gboolean icons_loaded = TRUE;

if (!nma_icon_check_and_load ("nm-no-connection", &applet->no_connection_icon, applet, FALSE))
    icons_loaded = FALSE;
if (!nma_icon_check_and_load ("nm-device-wired", &applet->wired_icon, applet, FALSE))
    icons_loaded = FALSE;
...

if (!icons_loaded) {
    <show warning dialog and quit>
}

and of course in the other bits of the code we just ignore the return value but pass TRUE for fallback since we accept fallback icons after the applet is already running.

This way we also save some code by not requiring the if (!applet->foobar_icon) bits all over the place.

Also, could you use the diff '-p' option to note which C function the hunk applies to?  Thanks!

One more comment; instead of the warning dialog, let's just abort() using g_assert() or something.  The warning dialog is useless since the user can't do anything about it, and we'd rather get automated crash reports when icon loading fails (ideally with the contents of ~/.xsession-errors along with the crash report).

Additionally, we can't recurse the mainloop while in a signal handler that's attempting to show a window (which might happen when the status icon gets resized) so the dialog can't be used in many cases anyway.

So feel free to rip the dialog-showing code out and just abort if the required icons can't be loaded at startup.  Having the dialog code in nma_icon_load() is wrong.

See https://bugzilla.redhat.com/show_bug.cgi?id=559470 for a backtrace of what happens when we try to display the dialog during a status icon resize...

Thank you for the review and sorry for the delay to reply I have been travelling then busy with other work tasks, I'm looking again at that now, one question though:

> nma_icon_load() function to take a "gboolean
> fallback" argument that if the load fails, return the fallback icon instead.

is there any case where you don't want the fallback to be used? I would say either you abort when an icon is not found since the installation is broken or you fallback to use a "broken icon" icon fallback for all missing icons. 
The fallback could be a stock gtk error icon one (should always be available since it's a gtk one)

Created attachment 153882 [details] [review]
change to load icons lazily updated

Update version of the change. The fallback icon used is gtk-dialog-error for now waiting on a better suggestion if somebody has one.

Created attachment 153884 [details] [review]
change to load icons lazily updated

the update clean some leftovers from the first version

Created attachment 154038 [details] [review]
change to load icons lazily updated

updated version to fix some spacing issues

I realized there were some cleanups I could do to my original suggestions to make the code cleaner, so I rolled those into this patch.  But note that when you C&P'd the section for wireless icons to applet-device-wired.c you added the gdk_pixbuf_copy() bits which would cause a memory leak, so I fixed that too.  Please test current master, thanks!

aca38ba8c0b94a40bbac8d2b42462492182339cb

Thank you for the review and fixing the issues