96339 - Display Resolution redundantly lists selection if selection is 72dpi or 96dpi

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect)

Description

[sairuh (rarely reading bugmail)]

seen using either modern or classic theme. will attach shots soon.

1. go to the Fonts pref panel.
2. select either 72dpi or 96dpi from the Display Resolution droplist.
3. click OK to dismiss prefs dialog.
4. go to the Fonts pref panel again and expand the Display Resolution droplist.

results: there's an additional entry for 72dpi [or, 96dpi] with a checkmark.

notes:

a. if i select either "System setting" or a user-defined resolution from the
Calibrate Resolution dialog [via "Other"], i don't see this "redunancy"
--instead the resolution appears as highlighted in the droplists, with no
checkmark.
b. if i don't save/dismiss prefs [ie, skip step (3)], the droplist doesn't show
the "redundancy", the selected item is highlighted without a checkmark.

Comment 1

[sairuh (rarely reading bugmail)]

Attachment: redundant entry with checkmark

Comment 2

[sairuh (rarely reading bugmail)]

Attachment: non-redundant entry highlighted *without* checkmark

Comment 3

[sairuh (rarely reading bugmail)]

whatever the fix, we should use one selection format, not both highlighting and
checkmarking.

Comment 4

[Gervase Markham [:gerv]]

The highlighting thing is done by the widget, and has nothing to do with me. I
thought I had added code to make this not happen, but maybe it broke. I'll look
at it again at some point.

Gerv

Comment 5

[Gervase Markham [:gerv]]

Patch is in bug 89842.

Gerv

Comment 6

[Gervase Markham [:gerv]]

Fixed.

Comment 7

[sairuh (rarely reading bugmail)]

fix implements checkmarks. when a numeric value [anything other than "system
setting" on unix] is selected, the selection appears [albeit repeated] beneath
the separator in the droplist with a checkmark.

linux, 2001.09.04.08-comm
winnt, 2001.09.04.00-comm
mac 9.1, 2001.09.04.11-comm

if this is not expected behavior, do reopen.

Comment 8

[Gervase Markham [:gerv]]

There should be no repeat. This still isn't quite fixed. :-(

Gerv

Comment 9

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Comment 10

[Gervase Markham [:gerv]]

This should fix it. Hwaara: any chance of a review? You'll note the added code
is basically stolen from SetFields a bit above, and the reason for the stealing
is explained in the comment.

Gerv

Comment 11

[Håkan Waara]

Comment on attachment 49702 [details] [diff] [review]
Patch v.1

>+    if( prefvalue != "!/!ERROR_UNDEFINED_PREF!/!" )

What string is that? Is that some kind of internal error?

If at all possible, could you have a global (like a #define in C++) variable that you compared with rather than hardcoding it?

>+        resolution = prefvalue;
>+    else
>+        resolution = 96; // If it all goes horribly wrong, fall back on 96.

Is 96 the default resolution?  Please make it fall back on a non-hardcoded global default.

Comment 12

[Gervase Markham [:gerv]]

Yes, that's an internal error.

> Is 96 the default resolution?  Please make it fall back on a non-hardcoded 
> global default. 

The default resolution is a pref, and varies from platform to platform. If we
can't get the prefs service, we are stuffed anyway.

Neither of these two queries relate to the changes made by this patch, BTW.

Gerv

Comment 13

[Håkan Waara]

Fine. r=hwaara

Comment 14

[Gervase Markham [:gerv]]

Blake: you super-reviewed the last patch in this area; could you do so again?
This is pretty small - six lines of code.

Gerv

Comment 15

[Blake Ross]

sr=blake

Comment 16

[Gervase Markham [:gerv]]

This was checked in on the 30th of September.

Gerv

Comment 17

[sairuh (rarely reading bugmail)]

no longer seeing the redundancy. thx for fixing this.

vrfy fixed using 2001.11.08.0x-comm bits on linux rh6.2, winnt and mac os 10.1.