Last Comment Bug 114929 - gtk desktop icon should use the system default colormap, not the window's
: gtk desktop icon should use the system default colormap, not the window's
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Dan Dickerman
: John Morrison
Mentors:
Depends on:
Blocks: 123569
  Show dependency treegraph
 
Reported: 2001-12-12 14:30 PST by Dan Dickerman
Modified: 2005-09-26 07:33 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
showing the issue: screen grab of two icons with/without the correct cmap (1.56 KB, image/gif)
2002-06-11 15:23 PDT, Dan Dickerman
no flags Details
suggested diffs as a patch (1.40 KB, patch)
2002-06-11 15:41 PDT, Dan Dickerman
no flags Details | Diff | Review
revised whitespace version of previous patch (1.65 KB, patch)
2002-07-03 09:13 PDT, Dan Dickerman
bzbarsky: review+
shaver: superreview+
asa: approval1.8b5+
Details | Diff | Review

Description Dan Dickerman 2001-12-12 14:30:52 PST
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 2002-01-09 12:31:50 PST
adding myself & syd
Comment 2 Boris 'pi' Piwinger 2002-06-03 12:43:59 PDT
Is this still a problem in a current build?

pi
Comment 3 Dan Dickerman 2002-06-07 09:10:21 PDT
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 2002-06-11 15:23:25 PDT
Created attachment 87273 [details]
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 2002-06-11 15:41:14 PDT
Created attachment 87279 [details] [diff] [review]
suggested diffs as a patch

Fix as a patch, to afford easy fixing
Comment 6 Boris Zbarsky [:bz] 2002-07-02 21:18:18 PDT
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 2002-07-03 09:13:24 PDT
Created attachment 90086 [details] [diff] [review]
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 [:bz] 2002-07-03 12:12:29 PDT
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 [:bz] 2003-03-13 21:16:07 PST
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) 2004-03-09 10:05:28 PST
Comment on attachment 90086 [details] [diff] [review]
revised whitespace version of previous patch

Clearing old request
Comment 11 Boris Zbarsky [:bz] 2004-03-09 11:31:47 PST
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 [:bz] 2005-09-24 10:32:49 PDT
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) 2005-09-24 11:01:48 PDT
Comment on attachment 90086 [details] [diff] [review]
revised whitespace version of previous patch

sr=shaver!
Comment 14 Brendan Eich [:brendan] 2005-09-24 12:01:06 PDT
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 [:bz] 2005-09-25 08:32:43 PDT
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 [:bz] 2005-09-25 08:33:25 PDT
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 2005-09-25 09:26:07 PDT
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 [:bz] 2005-09-26 07:33:19 PDT
Fixed on 1.8 branch.

Note You need to log in before you can comment on or make changes to this bug.