114929 - gtk desktop icon should use the system default colormap, not the window's

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Dan Dickerman]

The problem shows-up on a display with multiple colormaps or multiple X Visuals,
where the colormap for the desktop does not match the colormap in use for the
browser window itself.  In my situation, for example, the desktop is a 8-plane
PseudoColor visual, and the window is either a private colormap 8-plane
PseudoColor or 24-plane TrueColor visual.  In either of these cases, the
colormap used for the window and that used for the icon should be different, but
(in milestone 0.9.6) Mozilla chooses the colormap for the icon based on that in
use by the window, causing the icon to appear with the wrong color choices on
the desktop -- usually either a vague outline of the icon, or otherwise random
color choices.

The code for nsWindow::SetIcon (widget/src/gtk/nsWindow.cpp) creates a pixmap
based on the current window's colormap, then passes this to gdk for use as the
desktop icon:
   w_pixmap = gdk_pixmap_create_from_xpm(mShell->window .... ) ...
   SetIcon(w_pixmap, w_mask)
     gdk_window_set_icon(...)

To force the use of the system default colormap for the desktop, the pixmap
could be created using the system default colormap, changing the call from
gdk_pixmap_create_from_xpm() to gdk_pixmap_colormap_create_from_xpm(NULL,
gdk_colormap_get_system()...).

Comment 1

[Jim Dunn]

adding myself & syd

Comment 2

[Boris 'pi' Piwinger]

Is this still a problem in a current build?

pi

Comment 3

[Dan Dickerman]

The behavior is unchanged in the Mozilla 1.0 build, and the code for
nsWindow::SetIcon() appears unchanged in widget/src/gtk/nsWindow.cpp in the
curent top-of-branch.  This is with gtk libraries v1.2.10; I haven't checked if
the behavior is the same in other gtk library revisions, but I would assume so.

Comment 4

[Dan Dickerman]

Attachment: showing the issue: screen grab of two icons with/without the correct cmap

Attachment shows two iconfied mozilla 1.0 browsers: the one on the left is what
I believe to be correct (with my suggested modifications), and the one on the
right is how it appears when the code from Moz 1.0 (and earlier) is used as
distributed.  The X server is configured to specify a default visual of 8-bit
PseudoColor, with the CDE desktop using this visual and taking up a good
portion of the default color map.  When Mozilla starts, gtk queries the server
for better color depth, and chooses the 24-bit TrueColor Visual for its own use
-- presumably the difference in color maps and colormap depths are causing the
differences seen here.

Comment 5

[Dan Dickerman]

Attachment: suggested diffs as a patch

Fix as a patch, to afford easy fixing

Comment 6

[Boris Zbarsky [:bzbarsky]]

Dan, would you mind moving whitespace around so that:

1)  all the arguments to the function are lined up with each other
2)  The 80-char line length is exceeded as little as possible?

Do that and r=bzbarsky

Comment 7

[Dan Dickerman]

Attachment: revised whitespace version of previous patch

whitespace so adjusted: started the long function name on the next line to fit
the guidelines, which does look cleaner in the end.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 90086 [details] [diff] [review]
revised whitespace version of previous patch

r=bzbarsky; I have requested sr (per http://mozilla.org/hacking/reviewers.html)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 90086 [details] [diff] [review]
revised whitespace version of previous patch

Chris, could you please sr this?  I mailed you way back when about it, and it
seems to have gotten lost in the bitbucket...

Comment 10

[Christopher Blizzard (:blizzard)]

Comment on attachment 90086 [details] [diff] [review]
revised whitespace version of previous patch

Clearing old request

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 90086 [details] [diff] [review]
revised whitespace version of previous patch

Excuse me?  This patch has NOT been checked in.    It's still valid as far as I
can tell.  It still needs sr.  You're the obvious person to sr it, being most
familiar with this code.  If you don't have time to do the sr (and haven't
since a year ago), please just let me know and I'll look for someone else...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 90086 [details] [diff] [review]
revised whitespace version of previous patch

OK.  I've spent over 3 years here trying to get review for a contributed
patch...  I don't think I care whether it gets review from a GTK person
anymore.  Shaver, could you sr, please?

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 90086 [details] [diff] [review]
revised whitespace version of previous patch

sr=shaver!

Comment 14

[Brendan Eich [:brendan]]

blizzard's clearly not module owner -- I'd have fired him already, but he quit
long ago.  He has not appointed a successor.  Does anyone have suggestions for a
new owner?

/be

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.  Dan, our apologies that this took so long to deal with.  I
promise you that most modules in the Mozilla tree are in much much better shape
as far as having someone actually responsible for them.  :(

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 90086 [details] [diff] [review]
revised whitespace version of previous patch

I think we should take this on the 1.8b5 branch... It's very low-risk and helps
a good bit with not looking really weird in the cases when colormaps actually
matter.

Comment 17

[Dan Dickerman]

Hey no worries on the timescale: since this bug was filed, our first and second child were born (June 2002 
and June 2004), so we've been a little too busy to fuss with the finer points of colormap issues.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch.