Solaris 2.7/SPARC, 2001-05-20-08-trunk build with Sun Workshop 6 Update 2 EA2 + gcc 2.95.1 and Linux x86/gcc 2.95.2: Due a recent change in the prefs GUI it isn't possible anymore to set "browser.display.screen_resolution" to "0" (which means: use Xserver's DPI value). The prefs widget only offers "72dpi", "96dpi" and "other" - but "other" does not work. This breaks high-resolution displays because the fonts have the wrong size on such displays. I recommend to add a 4th choice: "Use/match Xserver's DPI" (which should be IMHO the default... - otherwise Mozilla won't adopt to Xserver's resolution automagically which is very anoying when working on different machines with different DPI values... ;-(( ).

Component should be "Preferences", no? Otherwise I concur: While the new UI at least leaves the setting alone, if it was set to "0" before; if you ever accidentaly or for testing purposes set your dpi override to another value, you can't go back to the "use X server's dpi" mode.

Setting component to "preferences", increasing "severity" because the default setting of Mozilla breaks our Xterminals ("0" was quite good for all platforms (assuming the Xserver's DPI is correct - which is usually done by our admins or automagically by config tools))... Asa... I'd like to get a fix for both issues in 0.9.2 - is that possible ?

-> preferences

gerv just checked in new pref dpi ui, and overlooked this option.? How about adding a text field next to option button? that way you could type in a number or use the pulldown. Adding german, over to gerv.

Yeah, OK. This wasn't in the spec - but that may well have been an oversight. I'm very much against complicating the UI with a text field. I suggest: Add another option to the dropdown, visible only on Unix. This would be titled "Ask X server" or "Automatic" and set the pref to a value of 0. This is a simple fix, assuming someone can tell me the Javascript equivalent of #ifdef XP_UNIX - we just need to decide the name of the item. mpt? Gerv

Gervase Markham: I would prefer your solution with adding a new item to the dropdown. The name of this item should somehow define what is does and _where_ it obtains the info from... ohhh, well... any ideas (what about "Same as Xserver") ?

'Automatic' (or perhaps ask 'windowing system' if you want something that's pedantic and bound to confuse macos users). since it should work for windows or macos.

Isn't it possible to get two different strings for two different platforms ? MacOS&WinLuser get "automatic" and Unix/Linux get their beloved "Same as Xserver"...

I like "Automatic" - it gives people a sense of security to know Mozilla is sorting it out for them, and there's no need to mess with the control. Users have no need to know where Mozilla gets the info from. It's also short and snappy. The point is not whether it _should_ work for MacOS, but whether it does. If setting the pref to 0 *right now* Does The Right Thing on MacOS, then we'll have to use some generic wording. Making the wording platform-dependent would be a pain. However, if the support is not enabled on Mac OS (and if it is, it's the first I've heard of it), then we don't want to give them this option, as it won't work. Gerv

"Automatic" is a bad wording - it is not predictable - I don't know, how the logic works. If it's Unix only, I'd use "Ask X server", otherwise "Same as OS" ot "Ask OS". > it gives people a sense of security to know Mozilla A false sense, because often the DPI value of the X server is misconfigured.

> because often the DPI value of the X server is misconfigured. Unfortunately yes... but a far worse idea is to work around this with own inventions... newer history of X11 toolkits is full of such bad ideas... do not make the same mistake, please...

"Ask X Server" it is (unless mpt has strong opinions.) Patch will arrive on Thursday. Gerv

Windows also has an OS setting for dpi, though Mac OS does not. So I don't think you can say `Ask X Server', since that won't make sense for Windows users (and nor would it mean anything to a majority of Unix users, a few years from now). I'd prefer `System setting (xyz dpi)', where xyz was the current value from the OS. I also think this value should be the default for new profiles on Windows and Unix. As Roland suggested, getting settings from the OS is good. If the X server is misconfigured, then perhaps the support calls generated (`how do I fix my fonts?') will inspire the sysadmin to configure the X server properly. :-)

> but "other" does not work. If you mean the dialog does not come up at all when you choose `Other ...', or something similarly horked, please file a separate bug for that and CC me.

> I also think this value should be the default for new profiles on Windows and > Unix. Agreed. > If the X server is misconfigured Note that even if the X server is misconfigured, the value is given is usually still better than our default of 96.

Sadly, as far as I know, we have no way of getting the system setting - basically the internals take the special value of 0 and make the relevant calls to the platform's subsystem. But we can do "System setting" certainly. Before we do this, though, we need to be certain on which platforms we can actually ask the OS. I'll dive into the code tomorrow and try and work it out. Gerv

The current logic is at: http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsDeviceContextGTK.cpp#140 It refuses any value < 96 that the X server gives it, which I presume is some hack to sort things if the X server is wrongly-configured. We do exactly the same on BeOS, Photon (whatever that is) and QT. On Mac, the relevant logic that may or may not get the platform-specific value is #if 0'd out: http://lxr.mozilla.org/seamonkey/source/gfx/src/mac/nsDeviceContextMac.cpp#1047 Pierre added this logic in 1999 saying "it may be useful later" but it has never been enabled. I'm not quite sure what's going on here: http://lxr.mozilla.org/seamonkey/source/gfx/src/xlib/nsDeviceContextXlib.cpp#120 Can anyone explain? Windows seems to ignore this pref completely. We need a simple fix for this for 0.9.1. The simple fix is to add the following line to pref-fonts.xul: <menuitem value="0" label="System setting"/> (properly localised, of course.) As we are running out of time, if you object to this fix, and can think of a better one which has a chance of getting past approval, speak now! :-) Gerv

Houston, we have a problem. Setting it to 0 means that I, at any rate, am unable to open any new windows - depending on the window, you either get nothing at all (prefs) or a frame with unpainted contents (e.g. History.) Checking back, this bug happened when you set the old text field to 0, so it's nothing I've done. Given that, if it were common (and I have a fairly boring Red Hat 7.1 install) this would be a reasonably major regression, we need to work out what to do next. However, there's a difference between a text box asking for a number (into which people might not think to put a 0) and a nice seemingly-safe "System setting" entry in a dropdown, which (once set) hoses your Mozilla until you restart. Does anyone else see this problem? Gerv

zero value problem should probably be a new bug. Adding a System setting option (0) sounds good to me. We should make sure to add it in a way that doesn't confuse mac/win32 people; what does a value of 0 do for those platforms? Does it really go ask the system?

> We do exactly the same on BeOS, Photon (whatever that is) and QT. Probably wrong, not implemented. File bugs. > <menuitem value="0" label="System setting"/> I'd do somthing like <menuitem id="systemResolution" value="0" label="System setting"/> var isX11 = ...; if (!isX11) { document.getElementById("systemResolution").hidden = true; } > It refuses any value < 96 that the X server gives it I don't read the source that way (the comment in the source is a bit misleading). Are you sure? I was about to file a bug...

McAfee: Win32 currently ignores this pref, and Mac would do something very strange with a value of 0, so we need to make this invisible on those platforms. > > We do exactly the same on BeOS, Photon (whatever that is) and QT. > > Probably wrong, not implemented. File bugs. I should read more carefully. OS2 uses 72dpi if the pref's not set, Photon doesn't set anything (that's probably a bug) but QT makes a seemingly-sane call: qPaintMetrics.logicalDpiX(); > <menuitem value="0" label="System setting"/> <snip> > var isX11 = ...; Very good - but how do you calculate the var isX11? > I don't read the source that way (the comment in the source is a bit > misleading). Are you sure? I was about to file a bug... Sorry, the comment confused me. I meant > 96 (in which case it uses 96.) Gerv

OK, here's a proper patch. The more I look at this code again, the more it seems I suck. This patch also fixes a bug which made the dropdown come up blank when you went back to the prefs after selecting a user resolution, and several nasty i18n issues which would have prevented it being properly localised. <sigh> Still, it's all a learning process. Looking for r= from one of the many people CCed on this bug. Gerv

The patch looks good so far and seems to work... ...but what happens if there is no initial DPI value... is "0"(=use DPI value of Xserver) really used ?

According to the code, if the pref is not set, it asks the OS (limiting to a max of 96dpi.) This is in the GTK file referenced above. Anyway, what it does when there's no pref is not the responsibility of the person working on the prefs panel ;-) Gerv

Er... what happens if the display has - for example - 120DPI ? (note: GTK+ has some ugly issues with handling DPI values which are out of the "usual" (75-100) DPI range... I won't rely on buggy code...)

Please read the code. LXR links are above :-) If you set the pref to 120dpi, it will do 120dpi, bugs or no bugs. It's only the OS-given resolution which is limited in this way. This is _nothing_ to do with this bug, of course :-) Gerv

Ohh... yes... I am blind. Xlib-module seems to be pretty correct... the GTK+-module solution of all-below-96dpi-is-wrong is a _BAD_ idea as this won't work with large, low-resolution displays... BAD... and that's worth to file another bug... ;-(

I still see a problem here. If there is no DPI value in ~/.mozilla/default/*/prefs.js and I change a value in the font prefs panel (but not touching the DPI widget) it sets the DPI value in prefs.js - which turns-off any further adaption if a later Mozilla "session" runns on another $DISPLAY with another DPI value (we have 90DPI(Sun/Ultra5), Xterminals (grayscale, 100DPI) and a Sun workstation which runns at 120DPI here...). But the default should be "adapt automagically to Xserver's DPI" - but how should that be possible when the first (fonts) prefs change sets a fixed DPI value even if I didn't touch that widget !?

> all-below-96dpi-is-wrong is a _BAD_ idea as this won't work with large, > low-resolution displays... BAD... and that's worth to file another bug... ;-( I think, you're misreading the source. The value is not ignored in either case. Note that there's an or ("||"), not an and ("&&").

http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsDeviceContextGTK.cpp#173 -- snip -- 173 // if we couldn't get the pref or it's negative, and the OS 174 // value is under 96ppi, then use 96. 175 mDpi = 96; -- snip -- If there is no prefs value and the OS is below 96DPI then 96DPI is forced... Solution would be to set the prefs value to "0" to get the expected result... ;-(

I take Roland's point about it losing the automagic ability. We shouldn't set the pref if it is unset. <sigh>. Did we get this right before? Back to the codeface. Gerv

Gerv... what about setting it always to "0" if this option is not present in prefs.ps ?

Because then if the behaviour in the C++ changes we may unintentionally change things we don't want to. If it's not set, it shouldn't be set. I will fix this. Gerv

> -- snip -- > 173 // if we couldn't get the pref or it's negative, and the OS > 174 // value is under 96ppi, then use 96. > 175 mDpi = 96; > -- snip -- > If there is no prefs value and the OS is below 96DPI then 96DPI is forced... > Solution would be to set the prefs value to "0" to get the expected result... > ;-( What you wuote is only the fallback. If you want the OS value, set the pref to 0. This is no workaround, it is the intention. 0 = os value. Yes, you shouldn't change the pref, if the user does nothing, if possible.

> Yes, you shouldn't change the pref, if the user does nothing, if possible. But then again, the pref should always be set )if not by the user, then the default pref). Otherwise, the behaviour is unspecified. So, I wouldn't care too much about that.

I'm talking rubbish. Roland is right. The best thing to do is set to 0 if we don't have a pref. This has the advantage that the dropdown will say "System setting" for platforms where this pref is actually ignored. This is good. However, this produces a problem if the user has an X server DPI value under 96. Before accessing the fonts prefs panel, 96 would have been used. Afterwards (when the pref has been set to 0), the OS value will be forced. But, as Roland says this special-casing should be removed, is this a problem? Having the pref "unset" really complicates things when trying to do UI for it, as you can imagine. I think we should go for the above solution, and remove the special casing in the C++ (if we think it's safe) later. After all, the more complex this patch gets, the less likely it'll get into 0.9.1. Gerv

Gerv, pref policy is: All prefs must be set, either as user pref or default pref. A pref having no value is illegal. The behaviour in this case is unspecified, but it should still do anything remotely sensible. (E.g. not crash or something.) That's why in the C++ code, there's always the case considered where no pref is sat. Usually, it's just sat to any specified value, e.g. 0. The fact that it does hav any logic in that case is unusal, but not a problem either. Personally, I'd set it to 96, if you can't get any value, since that's our current default, i.e. is known to work everywhere, which is not the fact for 0 (os value). As for 0.9.1, I don't know. I'd let drivers decide.

> pref policy is: All prefs must be set, either as user pref or default > pref. "default pref" being what the code falls back on when the pref isn't set, right? Because there are _hundreds_ of places where the code goes "Pref? No. Do this default thing then." > A pref having no value is illegal. But an unspecified pref is not. And we should, in an ideal world, avoid specifying an unspecified pref, especially when the "default" differs between platforms. However, in this case, we are going to do exactly that :-) > Personally, I'd set it to 96, if you can't get any value, since that's our > current default, i.e. is known to work everywhere, which is not the fact for 0 > (os value). We can't do that, for the problem Roland described. If someone accesses the fonts prefs panel, their OS autodetect (which is the default behaviour in the code) breaks and they get stuck with 96. Gerv

Any further improvements can wait until post 0.9.1. It's going to be tricky enough to get this in anyway; we've just branched. Gerv

gerv, no default prefs are what you find in defaults/pref/all.js etc. in the distro. What I call fallbacks are hardcoded values in the code, which are used when no pref at all can be gotten (libpref code will query both user and default prefs at the same time).

> - userResolution.setAttribute("label", resolution); > + userResolution.setAttribute("label", resolution + " " + dpi); Don't construct UI text that way. Localizers might need to reorg it or remove the space. Use a placeholder (e.g. dpi = "%v dpi") and replace it via regexp. Why don't you just put the unit (dpi) right outside the dropdown? > if (navigator.appVersion.indexOf("X11") != -1) Note that it is risky to depend on such browser features. We might remove that due to privacy reasons. Maybe only in specialized versions, but still. However, I have no better suggestion :-(. > - <menuitem value="72" label="72 dpi"/> > + <menuitem value="72" label="&resolution.72.label;"/> This cries "Hack!" to me. Compare bug 83060.

> Don't construct UI text that way. Localizers might need to reorg it or remove > the space. Use a placeholder (e.g. dpi = "%v dpi") and replace it via regexp. OK. Is there some standard for the placeholder? There should be... > Why don't you just put the unit (dpi) right outside the dropdown? Mpt's idea. But because several entries in the dropdown do not require the unit. Seems sensible to me. > > - <menuitem value="72" label="72 dpi"/> > > + <menuitem value="72" label="&resolution.72.label;"/> > > This cries "Hack!" to me. Compare bug 83060. How does it cry "hack" exactly? Because we make the most common values available directly? Gerv

> How does it cry "hack" exactly? Because we make the most common values available > directly? Because the values appear in both the XUL and the DTD file.

> Because the values appear in both the XUL and the DTD file. This is because the underlying code uses "72" for the "value" even if the "label" is in e.g. Arabic numerals. Or do you mean the fact that I've put the number as part of the entity name? What's wrong with that? Gerv

Ooh, this is horrible. We can't set to 0 on Mac because it would take it literally, and who knows what would happen. So we need an extra test for pref-not-set to set to 96 on Mac instead of 0. This next patch should cover all the bases: Pref present Pref 0 Pref not present ------------ ------ ---------------- Mac Uses pref Uses 96 Undefined (as before) Windows Ignored Ignored Ignored Unix Uses pref System setting System setting Can people please check the logic of this patch for sanity? The idea is that even if the pref gets set (when they access the fonts panel) the behaviour should not change if they didn't alter the pref. Gerv

We could easily fix the Mac C++ code to handle '0' as default (96).

Gerv, why not the following: (non.exclusive) - If pref not gettable, set it to 96. - If pref is 0, unhide dropdown item "System setting" and select it. - If we're on X11, unhide "System setting". - If pref matches a default setting (96, 72 etc.), select the corresponding item. Otherwise, unhide user setting item, set its value and select it.

> > Because the values appear in both the XUL and the DTD file. This is just a headache to maintain. Avoid it. > This is because the underlying code uses "72" for the "value" even if the > "label" is in e.g. Arabic numerals. I don't think, our UI works for Arabic at all.

arabic numerals are things like 1 2 3 ... =) and bidi landed so the other Arabic occasionally works, although i haven't seen mozilla localized for arabic. gerv: yes there's a standard for string substitutions. it's %s for strings and a method of stringbundle someone on irc should be able to point you in the right way.

*** Bug 36894 has been marked as a duplicate of this bug. ***

> Gerv, why not the following: (non.exclusive) > - If pref not gettable, set it to 96. No. We can't do this on X11, because of the problem Roland described - we have to make sure that X11 always has this pref, as I have done. Otherwise, what you propose is what Patch 5 does. > This is just a headache to maintain. Avoid it. Look, if you want the value of an element to be the number 72 and the label to be the text string "72 dpi" then, unless you really want to go round the houses, you put "72" as the value in the XUL (because it doesn't change on i18n) and "72 dpi" as a localisable string in the DTD, like I have done. This is not worth arguing about. > arabic numerals are things like 1 2 3 ... =) OK, Persian. You get the idea. > yes there's a standard for string substitutions. it's %s for strings and > a method of stringbundle someone on irc should be able to point you in the > right way. I'm not using stringbundles (it's overkill for one string) and I tried using %var but it didn't like it so I used $var. I added a l18n note. It should be obvious. Patch 5 has r=mcafee. Gerv

> Look, if you want the value of an element to be the number 72 and the label > to be the text string "72 dpi" then, [...] > This is not worth arguing about. Do you want to update all localizastions each time we change the a predefinition? (Note that the dialog will break completely, if one label is missing.) Why not construct the string exactly the same way you construct the userresolution string, from the value? > I'm not using stringbundles (it's overkill for one string) and I tried using > %var but it didn't like it so I used $var. %something is the convention, because that's what printf uses.

> Do you want to update all localizastions each time we change the a > predefinition? You continue to assume that the string "72" is the correct way to represent the number seventy-two in all locales. The underlying JS needs to use the string "72" (which it converts to a number) as the DPI value, but the label could use any representation of that number. Therefore, there needs to be two occurrences of the string "72" in chrome where "72" represents the number seventy-two. > (Note that the dialog will break completely, if one label is > missing.) This is true of any XUL - you get an undefined entity reference error. > %something is the convention, because that's what printf uses. It also seems to be a special character in some way (either to the regexp or in the XUL or in the DTD), because, as I said, it doesn't work. If you can explain why it doesn't work, that would be more constructive. There are no DTDs at all in the codebase containing the string %s, so I respecfully submit that using %s is not the way this is done in DTD (as opposed to property) files. Gerv

I've mailed Ben Goodger for sr=; however, he said in IRC that someone else familiar with this dpi stuff should look at it. Anyone have any suggestions? Gerv

> You continue to assume that the string "72" is the correct way to represent the > number seventy-two in all locales. Yes. That's exactly what you do for user-defined resolutions, too, not? > It also seems to be a special character in some way [...], > because, as I said, it doesn't work. I must have missed that, sorry.

You continue to assume that the string "72" is the correct way to represent the > Yes. That's exactly what you do for user-defined resolutions, too, not? OK, you're right. It's a fair cop. New patch attached. mcafee, if you aren't sick of the sight of this code, could you please review _again_? Incidentally, patch v.6 has a <separator class="thin/> between proportional and the rest of them to try and make German a little bit happier about the layout. Gerv

Ccing alecf for possible sr=. Gerv

ok, sr=alecf but you have to get localization approval of course

CCing Netscape i18n as a heads-up. There are two text strings added here: this is an i18n win because before the letters "dpi" were hard-coded into the panel, and therefore not localizable at all. The relevant bits of the patch are: +<!ENTITY resolution.dpi "dpi"> +<!ENTITY resolution.system.label "System setting"> -<!ENTITY resolution.default "default"> Of course, as far as I am aware, there is no mozilla.org i18n freeze. Gerv

r=mcafee for patch v6

a=blizzard (on behalf of drivers) by email. Gerv

Checked in on trunk and branch. Gerv

Fonts pref panel is currently broken. Backing out your changes gets it working again. Please prepare a fix ASAP or be subject to tree backout. XML Error in file 'chrome://communicator/content/pref/pref-fonts.xul', Line Number: 231, Col Number: 26, Description: undefined entity Source Line: <menuitem value="&resolution.72.value;"

Patch appears to fix the problem, but having numerical values in the localization dtd seems odd. What happened to having a text field where you can override the DPI setting?

Apologies, lads, I was asleep. Exam in 2 hours :-) The current patch is the wrong one, because I reworked this precisely to avoid having the numbers in the DTD. Correct patch coming up. (Having a direct text entry box is a separate, non-0.9.1 bug. I don't agree with the idea personally.) Correct patch coming up. Gerv

I suck. I swear I tested this before I checked it in, but obviously not well enough. <sigh>. New error-fixing patch attached. Anyone around to r= and sr=? Gerv

r=mcafee for 5/01 23:51 patch

sr/a = tor for branch and trunk

Checked in on branch and trunk. Apologies to all for the hassle caused. This works in my local tree (honest, really it does) so I hope I never have to see this bug again. Gerv

*** Bug 84275 has been marked as a duplicate of this bug. ***

vrfy fixed using commercial branch bits. linux 2001.06.06.04: i'm able to use the "System setting" option. winNT 2001.06.06.06 and mac 2001.06.06.14: "System setting" isn't present [as expected] if someone manages to vrfy this with trunk bits [either comm or moz, doesn't matter] before me, go ahead and remove the 'vtrunk' kw and mark this Verified. thx!

looks fine using comm trunk bits. mac, linux - 2001.06.07.08 winnt - 2001.06.07.09

If I select "System Setting" in my Mozilla 0.9.1 (Linux), browser.display.screen_resolution will not appear with a "0" value in prefs.js, but disappear completely. That's not correct, because the code at http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsDeviceContextGTK.cpp#140 will handle an undefined value different from "0" -- e.g. people with 75 dpi displays will get their dpi forced to 96. http://lxr.mozilla.org/seamonkey/source/gfx/src/xlib/nsDeviceContextXlib.cpp#130 also treats undefined as "use 96 dpi", NOT as "take dpi from X server". Reopen? Or just kill the silly 96 dpi logic in the nsDeviceContext* classes?

robbe, a user preference value that happens to match the default pref (in defaults/prefs/*.js) will be deleted. This is generic behaviour and not specific to this bug.

I think this caused regression bug 233082.