Closed Bug 77828 Opened 23 years ago Closed 23 years ago

Color prefs not working

Categories

(SeaMonkey :: Preferences, defect)

Product:
SeaMonkey
Component:
Preferences
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.1

People

(Reporter: bugzilla, Assigned: bnesse)

References

Details

Attachments

(2 files)

Possible patch to nsPresContext (small impact)
3.15 KB, patch
Details | Diff | Splinter Review
Patch corrected for sfraser's comments
3.14 KB, patch
Details | Diff | Splinter Review 
simon sees this on a recent mac debug build from today --and it occurs
regardless of theme, and on both an exisiting/migrated profile and a new
profile. he cannot repro this on a recent win32 build, however.

i'm currently using a 4/25 comm verif builds for some other testing, so won't be
able to test this till tomorrow's verif bits are out.
note to self: also check linux builds, to see if this is really mac only.
Keywords: nsbeta1, regression
Maybe color prefs got broken by bnesse?

I see this in a current linux build too.
Hardware: Macintosh → All
Link colors also changed in the last day or so, and now look lighter, and 
somewhat washed-out. What changed?

It's most likely mine...
Assignee: mcafee → bnesse
Ugh. This is a result of the decision to remove the "color pref" methods. Even 
though GetColorDWordPref returned an uint, it was fetching a char string and 
sprinting into the uint.

There's a couple of ways we could quick fix this, but what we should do is 
determine how we really want color prefs handled. I believe conrad and possibly 
valeski had thoughts on this. cc'ing them.

Text and Background color prefs are broken as well. tested on winNT
[2001.04.27.09] and linux [2001.04.27.05].
Severity: normal → major
Keywords: nsCatFood
OS: Mac System 9.x → All
Summary: link colors not working → Color prefs not working
I think we should have a color pref accessor. We used to and it wrote the color
in the #RGB format which is nice and XP. Color prefs were changed to just use an
int and there is no migration from the old format to the new, hence the
breakage. I'd stick with the previous #RGB string format so we can still read
existing pref files and add a color pref accessor.
I'm not opposed to conrad's suggestion, I just recall being told that it should
go away. If we add it back in, do we go with the GetColorPref() version which
returns r, g, and b parameters or the GetColorDWordPref version which returns an
NS_RGB format value?
How many users of GetColorDWordPref did there used to be? Couldn't we end up
with copies of MakeColorPref all over the place? I'd think that the RGB color is
a pretty common pref type. As far as whether it's as one ARGB long or r, g, and
b params, hmm. The 2nd, while a little more laborious to use, is more clear and
less dependent on whether it's ARGB, RGBA, or whatever.
nsPresContect is the only consumer who was using it. And it only uses the
Getter, not the Setter. If indeed this functionality is desirable, it certainly
belongs in prefs to avoid just the situation you describe.

It could be implemented as a complex value instead of as a standalone method if
that's more desirable. We could even return an object that could contain the
information in multiple formats I suppose.
*** Bug 76853 has been marked as a duplicate of this bug. ***
*** Bug 78115 has been marked as a duplicate of this bug. ***
patch looks reasonable to me - as I recall, nsPresContext was the only consumer
of color prefs. Another option is to use a ComplexPrefs system, but I think
that's overkill here.
sr=alecf on the patch
True, nsPresContext is the only consumer of color prefs in our code, but
embeddors need to access color prefs in order to get/set them with their own UI.
I know that they were unclear as to how to do this - what the format was,
whether they had to convert it to a string and use SetCharPref, etc. If there
was such a thing as an nsISupportsColor and they could use it with
get/setComplexPref, it might be easier for them. That might be good at some
point - nothing against the current patch.
we could make a nsIColor interface (or maybe there's one already?)

interface nsIColor {
    attribute short red;
    attribute short green;
    attribute short blue;
}


Do we really want to force heap allocations and refCounting just to track colors?

nope, I think it's overkill...consumers are already dealing with heap
allocations however, since they were using strings.
quite frankly, I think they should just be using 32-bit signed long integers,
but NOO.. we had to come up with some dorky sscanf trick.

Here's a thought...what if we fixed pref migration to switch the strings into
integer values, thus changing the pref type. old consumers (i.e. the ones
addressed in this patch) should handle both types in the function that bnesse
created...

then we can tell people (embedders) that they can add pref defaults by saying
user_pref("some.color.pref", 0xff00ff);
to define their colors, and use GetIntPref() to retrieve them.
*** Bug 78394 has been marked as a duplicate of this bug. ***
*** Bug 78493 has been marked as a duplicate of this bug. ***
> user_pref("some.color.pref", 0xff00ff);

That would reduce the human-readability of prefs.js, which -- especially 
because of the number of GUI-less prefs we have -- I think is kinda important. 
(Cf. the lengthy discussions in other bugs about the names for some prefs, when 
the names only matter to humans.) Feel free to ignore me if you think I'm being 
too trivial ...
you're being too trivial. this is prefs.js. it's a internal file. if you're
mucking with it then you better understand enough to know that 0x<numbers> is a
hex number.
i would personally be fine with a solution involving prefs.js.  my impression is
that the average user would not be mucking around in there anyway.  something
informative like
user_pref(browser.document.foreground,"#ffffff");
would be fine for probably anyone who knows html. 0xffffff is of course the
standard, but if you're worrying about the users...  i would think that anyone
who doesn't even know basic html would probably not be manually editing prefs
anyway.

hasn't been any activity here in a few days.  what's up? =)

it's getting somewhat tedious having to select text in mail when i want to read
it... ;-)
personally, I'm fine w/ hex values for colors. the more convenient we can make
the values for colors the better, but allocating new mem should be avoided.
It seems that, though we may not know exactly how we want to support color
preferences, we don't want to create a color object. The current scheme is
essentially converting a integer hex value into a string value and back (behind
the scenes), which in my mind isn't really a gain in any way.

The question is, should we go with the patch as it stands (I'll add some
comments to it in way of explaination) or should we, as Alec suggested, change
prefs migration to convert it to an integer value?

If we go with the second option it would probably make sense to rewrite the
patch to support both string and int types (or to convert strings to ints) as we
would have backward compatibility issues with old profiles.
I think we should check in the patch as is, leaving this bug open.. then we come
up with a second patch which:
- supports both strings and integers
- fixes the migration code to change the strings to integers when migrating a
4.x profile
- change the defaults in all.js to use integers
Ok, in that case, can I get an r= from someone?
+static void MakeColorPref(const char *colstr, nscolor *colorref);
You don't need a prototype for a routine that is declared |static|.

Also, since an nscolor is just a PRUint32, it would be clearer if MakeColorPref 
just returned an nscolor. With those changes, r=sfraser

Ah yes, I returned in the param because my original implementation returned a
nsresult. I'll change it.
r/sr=alecf
Fix checked in. Leaving bug open as per Alec's suggestion but downgrading 
severity and clearing keywords.
Severity: major → normal
Keywords: nsbeta1, nsCatFood, regression
*** Bug 79482 has been marked as a duplicate of this bug. ***
Closing this bug as per my managers request. Bug 79724 has been opened to track
any additional work.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla0.9.1
while trying to vrfy this, i encountered bug 81553. however, i can change the
colors, so am marking this one verified [we can track other issues in other
bugs].

winnt - 2001.05.17.10
mac and linux - 2001.05.17.08
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: