121258 - gtk2 needs to have icons hooked up

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Christopher Blizzard (:blizzard)]

gtk2 needs to have icon code hooked up.

Comment 1

[Jeff Qiu]

I am working on this bug now.

Chris, what is your idea? Do you think we should load different icons for
different windows, like load chrome/icons/default/editorWindow for a composer?
Or we should just port gtk1 code here and set the same icon
(mozilla/icons/mozicon16.xpm & mozicon50.xpm) for all mozilla windows?

Comment 2

[Christopher Blizzard (:blizzard)]

Interesting.  I never noticed that the SetIcon() call on gtk wasn't using the
name spec that was passed in as part of the call.  We have the ability to set
the icon to a different value.  We might as well do that here.

Have a look at the code in the windows front end code as to how to turn the
resource into a path name.  From there, you should be able to load the image and
set the icon.  There are probably easy to use gtk2 apis for that, too.

Comment 3

[Jeff Qiu]

Attachment: default.xpm, a mozilla icon for all windows.

Put it to mozilla/widget/src/gtk2/

Comment 4

[Jeff Qiu]

Attachment: Set different icons for different windows

Set different icons for different windows using the name spec that was passed
in.

If there is no match xpm file for the window, default.xpm will be used as its
icon.

Comment 5

[Jeff Qiu]

Attachment: icons.tar.gz, includes some xpm icons for test

Copy the file to mozilla/dist/bin/chrome, then "tar xvzf icons.tar.gz". Run
your mozilla and enjoy. You won't need to find your mailnews window from a
bunch of navigator windows anymore.

P.S: these icons are copied from Netscape 6 windows version, just for test
purpose.

Comment 6

[Jeff Qiu]

Attachment: patch V2

Use the native file path.
Chris, could you review it?

Comment 7

[Christopher Blizzard (:blizzard)]

Comment on attachment 89397 [details] [diff] [review]
patch V2

We need to add the .xpm files to the packages-unix files.

>+    // Global default icon
>+    static GdkPixmap *default_iconPixmap = nsnull;
>+    static GdkBitmap *default_mask = nsnull;

Please don't mid underscores and mixed case in the same variable name.	It
doesn't flow well.  Instead of using function level statics, can you add a
static to the class?  I don't usually use function level statics.

>+
>+    GtkStyle *w_style;
>+    w_style = gtk_widget_get_style (mShell);
>+
>+    // Start at the app chrome directory.
>+    nsCOMPtr<nsIFile> chromeDir;
>+    if (NS_FAILED(NS_GetSpecialDirectory(NS_APP_CHROME_DIR,
>+                                         getter_AddRefs(chromeDir)))) {
>+        return NS_ERROR_FAILURE;
>+    }

I much prefer a style like this:

rv = NS_GetSpecialDirectory(...);
if (NS_FAILED(rv))
  return rv;

that way the error is propagated, and it's much more readable and consistent
with the rest of the style in the module.

>+        nsCOMPtr<nsILocalFile> defaultPathConverter;
>+        if (NS_SUCCEEDED(NS_NewLocalFile(
>+                             defaultPath,
>+                             PR_TRUE,
>+                             getter_AddRefs(defaultPathConverter)))) {

Once again, please use a return value, as above.

>+            nsCAutoString aPath;
>+            defaultPathConverter->GetNativePath(aPath);
>+
>+            default_iconPixmap = gdk_pixmap_create_from_xpm(
>+                                     mShell->window,
>+                                     &default_mask,
>+                                     &(w_style->bg[GTK_STATE_NORMAL]),
>+                                     aPath.get());

I would rather that you use something like:

default_iconPixmap =
    gdk_pixmap_create_from_xpm(mShell->window,
			       &default_mask, ...);

>+    GdkPixmap *iconPixmap = nsnull;
>+    GdkBitmap *w_mask = nsnull;

Same thing as above, please pick a style and stick with it.

>+
>+    nsCOMPtr<nsILocalFile> pathConverter;
>+    if (NS_SUCCEEDED(NS_NewLocalFile(iconPath,
>+                                     PR_TRUE,
>+                                     getter_AddRefs(pathConverter)))) {

Same as above with the return value.

>+        iconPixmap = gdk_pixmap_create_from_xpm(
>+                                 mShell->window,
>+                                 &w_mask,
>+                                 &(w_style->bg[GTK_STATE_NORMAL]),
>+                                 aPath.get());

Same as above with indenting.

>+    if (!iconPixmap) {
>+        if (default_iconPixmap) {
>+            gdk_window_set_icon(mShell->window, (GdkWindow*)nsnull,
>+                                default_iconPixmap, default_mask);
>+            return NS_OK;
>+        }
>+        return NS_ERROR_FAILURE;
>+    }
>+
>+    gdk_window_set_icon(mShell->window, (GdkWindow*)nsnull, iconPixmap, w_mask);
>+
>+    return NS_OK;

I think this is broken.  It should be:

if (iconPixmap) {
  gdk_window_set_icon(... iconPixmap ...);
}
else if (default_iconPixmap) {
  gdk_window_set_icon(... default_iconPixmap ...);
}

the set_icon call is always called with the iconPixmap, even if iconPixmap is
NULL in the code above.

Comment 8

[Jeff Qiu]

Attachment: patch v3

Chris, thank you for your review, very helpful.
I'll pay more attention to coding style.

Comment 9

[Christopher Blizzard (:blizzard)]

I went and looked at the documentation and I think that we should be using
gtk_window_set_icon_list() instead of gdk_window_set_icon().  Have a look at the
documentation to see what I'm talking about.

http://developer.gnome.org/doc/API/2.0/gdk/gdk-windows.html#gdk-window-set-icon
http://developer.gnome.org/doc/API/2.0/gtk/gtkwindow.html#gtk-window-set-icon-list

Comment 10

[Jeff Qiu]

Attachment: Final patch, hopefully

Use gtk_window_set_default_icon_list() & gtk_window_set_icon ()

Comment 11

[Jeff Qiu]

--> me

Comment 12

[Jeff Qiu]

--> Bolian

Comment 13

[Christopher Blizzard (:blizzard)]

Checked in after minor cleanup.  Thanks!

Comment 14

[Alan Cramer]

How about some of the icons at
http://genghis.winamp.com/~steve/archive/?dir=mozilla ?