Subject: FvwmIconMan colors, fixed!

From: Michael Han <[email-masked]>
Date: Sat, 26 Feb 2000 23:09:15 -0800

So it took me a long time to find it, but I found that Tim's second
commit, to remove nColorsets from FvwmIconMan is where the color bug
was introduced. It's caused by the following line in x.c:
create_manager_window():

    if (Colorset[man->colorsets[i]].pixmap) {

What I found with gdb is that for some bizarre reason, Colorset[-1]
doesn't raise an error and will just point to random, non-random data!
I say "random, non-random" data because it's random in the sense that
it's nothing I intended, non-random because using the same config, I
can always get the same unwanted data.

I'm not sure what the right thing to do here is. I tested my theory
out by changing the above line to:

    if (man->colorsets[i] > -1 && Colorset[man->colorsets[i]].pixmap) {

and finding my colors to be working properly again. I guess this also
explains the BadDrawable errors I reported in my last message, since
the condition here executes the following else:

    } else {
      man->pixmap[i] = None;
      XSetFillStyle(theDisplay, man->backContext[i], FillSolid);
    }

Anyway, I'm not sure why Tim got rid of the '% nColorsets' business,
though I assume it was for good reason. Perhaps my suggested fix is
the best way to do things, or perhaps there's a better way. Anyway,
here's the very trivial patch, in case it's wanted (also, in case you
needed line numbers ;-):

diff -u -r1.37 x.c
--- modules/FvwmIconMan/x.c     2000/01/12 21:38:42     1.37
+++ modules/FvwmIconMan/x.c     2000/02/27 07:03:07
@@ -918,7 +918,7 @@
     }
     if (man->pixmap[i] && man->pixmap[i] != ParentRelative)
       XFreePixmap(theDisplay, man->pixmap[i]);
-    if (Colorset[man->colorsets[i]].pixmap) {
+    if (man->colorsets[i] > -1 && Colorset[man->colorsets[i]].pixmap)
{
       man->pixmap[i] = CreateBackgroundPixmap(theDisplay, man->theWindow,
                        man->geometry.width, man->geometry.height,
                        &Colorset[man->colorsets[i]],

-- 
[email-masked]                       http://www.best.com/~mikehan/
coffee achiever                              San Francisco, California
A double negative is a no-no.
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Dominik Vogt <[email-masked]>
Date: Mon, 28 Feb 2000 11:50:54 +0100

On Sat, Feb 26, 2000 at 11:09:15PM -0800, [email-masked] wrote:
> though I assume it was for good reason.

Because the colour set number could only be < 0 or >= nColorsets
in case of a bug in the code.  Using (-1 % nColorsets) isn't right
either.

> Perhaps my suggested fix is the best way to do things, or perhaps
> there's a better way. Anyway, here's the very trivial patch, in
> case it's wanted (also, in case you needed line numbers ;-):

The fix is exactly the right thing to do.  A check for > -1 is
used throughout the code (most modules) to detect if a colour set
has been specified.  I'll apply the patch.

Bye

Dominik ^_^

-- 
Dominik Vogt, Agilent Technologies, Dept. BVS
Herrenberger Str.130, 71034 Boeblingen, Germany
phone: 07031/464-4596, fax: 07031/464-3883, [email-masked]
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Tim Phipps <[email-masked]>
Date: Mon, 28 Feb 2000 15:56:03 +0000

> Anyway, I'm not sure why Tim got rid of the '% nColorsets' business,

Much faster not to do the modulus on every access. And bugs like this
one are easier to spot without it.

Cheers,
Tim.

---

From: Michael Han <[email-masked]>
Date: Mon, 28 Feb 2000 09:56:38 -0800

Previously from Dominik Vogt
>has been specified.  I'll apply the patch.

Okay, good. Thanks. Incidentally, there's a second identical line
sitting at line 937(?) of the x.c in change_colorset(). I wasn't sure
if this is a bug that could be triggered, so I didn't mention it
before. I notice that none of the other references to
man->colorsets[i] seem to be protected by any sanity checks.
-- 
[email-masked]                         http://www.best.com/~mikehan/
coffee achiever                              San Francisco, California
"What was your username?" - BOFH
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Brad Giaccio <[email-masked]>
Date: Mon, 28 Feb 2000 22:55:25 -0500

> man->colorsets[i] seem to be protected by any sanity checks.

First off aperentlly I started the problem, so sorry Michael.  If I
would have tracked the problem down instead of just ignoring it in the
error handler we probablly would have found this sooner.

But hind sight is always 20/20.

Tim, why don't we come up with a condition we can agree on and split up
the modules to make sure all of them are checking for a valid number?

Brad

-- 
--- There are two kinds of knowledge, you either know the answer or
			you know where to find it
		      -Kane, Johnson, and anonymous
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Tim Phipps <[email-masked]>
Date: Tue, 29 Feb 2000 15:24:46 +0000

> > man->colorsets[i] seem to be protected by any sanity checks.

This block is only executed with a valid colorset so there is no need to
range check.

> the modules to make sure all of them are checking for a valid number?

I'd rather wait for bug reports. The symptoms are fairly unique: colours
are randomly wrong when not using colorsets.

Cheers,
Tim.