Now that bug 342087 is fixed, we finally can use toolkit's new preferences architecture without losing our distinctive preferences UI. Callek started work on this in attachment 229635 [details] [diff] [review], which we need to continue here. This will mean we need to (a) rewrite pref.xul to use <prefwindow> and <prefpane> (and kill preftree.xul) (b) rewrite *all* our pref panels to use <preferences> etc. (c) kill nsPrefWindow.js and nsWidgetStateManager.js

This is a first WIP patch for the new style preferences framework in SM: - The new preferences main window will be named preferences.xul (as in TB or FF). This will allow us to keep the old preferences window around until all panels are migrated. The patch includes some then-to-be-deleted code for that. - To ease the pain of having to specify both a prefpane element and a treeitem in the main preferences window, this patch makes prefwindow.xml create such prefpanes automatically. This WIP patch does not include panel migration, but any such attempt at least needs to make the pref file's main element be an <overlay> including the actual <prefpane> element, whose id must match those specified in the prefpane/url attribute in preferences.xul. I'll provide a demo panel migration soon.

Comment on attachment 283452 [details] [diff] [review] WIP: main pref window, global interim changes Requesting review to get some thoughts if the general direction is acceptable.

Comment on attachment 283452 [details] [diff] [review] WIP: main pref window, global interim changes >+ openWindow(null, "chrome://communicator/content/pref/preferences.xul", >+ "chrome,titlebar,dialog=no,resizable", ""); This will reselect the last selected pane, right? Presumably we can preset lastSelected so that it will default to navigator on a new profile? >- var resizable; The new pref window is always resizable? Hmm... >+ <resources> >+ <stylesheet src="chrome://communicator/skin/preferences.css"/> >+ </resources> Out of interest, what's the intention here? >Index: suite/themes/classic/jar.mn Modern? ;-) Think of this as an sr+ rather than an r+ :-)

Comment on attachment 283452 [details] [diff] [review] WIP: main pref window, global interim changes All in all I like the direction this took, even better than my initial concept if you can believe it... anyway brief review of this WiP >Index: suite/common/bindings/prefwindow.xml >+ // add the class "prefnavtree", so that themes can set defaults >+ aTreeElement.className += ' prefnavtree'; Nit: when |initNavigationTree| is called with another tree should we remove this className somehow? (p.s. I may be simply missing it, but do we hide extra <tree>'s that get pulled in to there? [if we do, ignore the nit]) >+ if (!pane && autopanes) >+ { >+ // if we have a url, create a <prepane> for it >+ var paneURL = node.getAttribute('url'); Nit: <prefpane> not <prepane> >- this.showPane(pane); >+ try >+ { >+ this.showPane(pane); >+ } >+ catch (e) >+ { >+ dump('***** broken prefpane: ' + pane + '\n' + e + '\n'); >+ pane = null; >+ } Without knowing top-of-my-head why showPane would throw (xml parsing error for overlay loading?) would a better error message here make sense? [maybe paneURL instead for one] >Index: suite/themes/classic/jar.mn >+ skin/classic/communicator/preferences.css (communicator/preferences.css) This file missing from patch, and from modern (as neil noted)

Architecture patch, introducing the new main pref dialog preferences.xul and temporary changes to allow having both pref dialogs at once for a transition period, plus some functionality to ease the panel migration. (In reply to comment #3) > This will reselect the last selected pane, right? Presumably we can preset > lastSelected so that it will default to navigator on a new profile? The last selected panel is only relevant, if the dialog does not get a panel argument on startup, but that is usually the case anyway. > >- var resizable; > The new pref window is always resizable? Hmm... Yes. I see no point in prohibiting (temporary) adjustments by the user here. > >+ <resources> > >+ <stylesheet src="chrome://communicator/skin/preferences.css"/> > >+ </resources> > Out of interest, what's the intention here? Otherwise, the dialog elements like tree or header would stick to the borders. Doesn't look very nice... > >Index: suite/themes/classic/jar.mn > Modern? ;-) Added. (In reply to comment #4) > >Index: suite/common/bindings/prefwindow.xml > >+ // add the class "prefnavtree", so that themes can set defaults > >+ aTreeElement.className += ' prefnavtree'; > > Nit: when |initNavigationTree| is called with another tree should we remove > this className somehow? I don't think this is worth the effort to solve, calling this with another tree should be rare enough to pack that burden upon those who do so. > Nit: <prefpane> not <prepane> Heh! Done. > Without knowing top-of-my-head why showPane would throw (xml parsing error for > overlay loading?) Or trying to load a pane twice, etc. pp. > >+ skin/classic/communicator/preferences.css (communicator/preferences.css) > > This file missing from patch, and from modern (as neil noted) preferences.xul was missing also, added them.

First two panel migrations, as a demo.

Hmpf, the changes to mozilla/suite/common/pref/preftree.xul belong into the migration patch, not into the architecture one. Just in case anyone wonders. But both patches should be applied together anyway...

(In reply to comment #5) >(In reply to comment #3) >>This will reselect the last selected pane, right? Presumably we can preset >>lastSelected so that it will default to navigator on a new profile? >The last selected panel is only relevant, if the dialog does not get a panel >argument on startup, but that is usually the case anyway. Usually != always. Anyway, I couldn't get -preferences to default to a pane. The old dialog does at least load browser preferences even though it doesn't highlight the right tree item.

Comment on attachment 286063 [details] [diff] [review] main pref window, global interim changes, v1 >- var resizable; Oh, and I'm still not convinced about the resizability.

Comment on attachment 286063 [details] [diff] [review] main pref window, global interim changes, v1 >+ // list of scripts to load >+ var scripts = aPane.getAttribute('script') >+ .replace(/\s+/g, ' ').split(' '); >+ var count = scripts.length; >+ if (!count) >+ return; I would have pointed this out before but I thought I was superreviewing the other patch :-\ split always returns an array of at least one item, that may be empty. You probably want something like this: var scripts = aPane.getAttribute('script').match(/\S+/g); if (!scripts) return; var count = scripts.length;

Comment on attachment 286064 [details] [diff] [review] migration of main browser pref panel and main security panel, v1 >Index: suite/common/pref/pref-navigator.xul >+ <prefpane id="navigator_pane" onpaneload="this.parentNode.initPane(this);"> Is there someway around needing to explicitly init the pane's? can't we overload "onepaneload" or something, or *somehow* get our event handler for it to fire before the default one? I won't press us to block on that alone, but it just seems like overkill w/ this.

Comment on attachment 286063 [details] [diff] [review] main pref window, global interim changes, v1 Looks good. Nit: regarding overall design placed in c#12

(In reply to comment #9) > Anyway, I couldn't get -preferences to default to a pane. > The old dialog does at least load browser preferences even though it doesn't > highlight the right tree item. Ah, -preferences. I'll look into this, but why should it default to *browser* preferences? Or the other way round: why aren't browser preferences the first section then? (The dialog as in this patch should default to the first panel.) (In reply to comment #10) > Oh, and I'm still not convinced about the resizability. Why? I never understood why it wasn't... The resizability mustn't be an excuse for overstuffed panels, of course... (In reply to comment #12) > onpaneload="this.parentNode.initPane(this);"> > > Is there someway around needing to explicitly init the pane's? can't we > overload "onepaneload" or something, or *somehow* get our event handler for it > to fire before the default one? I'm not particularly happy with that either. I tried to derive the <prefpane> binding and specify a handler there, but it didn't quite work. I'm not quite sure if this was before or after I solved some loadOverlay issues, though, so I'll have a look at that again.

- Introduced a news <prefpane> derivation with just a paneload event handler which does our subscript loading incl. the eventual call to Startup(), so prefpanes don't need to care for that. Any given onpaneload attribute will still work, of course. - When starting the preferences with -preferences, the first panel is selected (but does not quite work because it's not migrated yet). I see no pane in selecting the browser panel here. Opening the preferences from the browser menu will open the right panel, of course.

Updated to match architecture patch v2: - dropped onpaneload attribute - added missing diff for preftree.xul Taking over reviews, since there are no fundamental changes here.

Errata to comment #15: > - Introduced a news <prefpane> derivation ... a new <prefpane> ... > I see no pane in I see no point in ...

Comment on attachment 286198 [details] [diff] [review] migration of main browser pref panel and main security panel, v2 [checked in] >Index: suite/common/pref/pref-navigator.xul >+ <prefpane id="navigator_pane" >+ script=" zzzz >+ chrome://communicator/content/pref/pref-navigator.js"> zzzz? :-P Also, after re-looking at this, why is the logic in "OnDialogAccept" needed?

Comment on attachment 286197 [details] [diff] [review] main pref window, global interim changes, v2 [checked in] does our prefpane handler fire before the handler on <prefpane onpaneload=""> ? If not, I think we _need_ to document that (or change some stuff around). Second, as a followup, we might want to trim the (very) lengthy comment at the top of preferences.xml to be a MDC doc or something. :-)

(In reply to comment #18) > >+ <prefpane id="navigator_pane" > >+ script=" zzzz > >+ chrome://communicator/content/pref/pref-navigator.js"> > > zzzz? :-P It's been rather late then. :-[ We need some kind "do a visual interdiff between attached patch x and new-patch-to-attach". ;-| > Also, after re-looking at this, why is the logic in "OnDialogAccept" needed? We can have an arbitrary number of homepages in a homepage group. OnDialogAccept is needed to get rid of any unused preferences - they wouldn't actually do harm, but cost disk space. (Imagine having your homepage set to a group of 100 tabs, then changing it to just one - .count would say 1, only 1 homepage will be seen by the dialogue, but 99 will still be dangling in prefs.js...) (In reply to comment #19) > does our prefpane handler fire before the handler on <prefpane onpaneload=""> ? paneload events descend from the top, hit our prefwindow.paneload handler, descend further down, hit our prefpane.paneload, hit toolkit's prefpane.paneload, finally evaluate any given onpaneload attribute and then bubble up again. (See general Mozilla event handling and XBL event handling in MDC, plus toolkit's prefwindow._fireEvent method.) > Second, as a followup, we might want to trim the (very) lengthy comment at the > top of preferences.xml to be a MDC doc or something. :-) Supposing you mean prefwindow.xml: I don't think so. Websites can get lost. ;-) But it would be more visible in DevMo or the wiki, true.

(In reply to comment #20) >(In reply to comment #19) >>does our prefpane handler fire before the handler on <prefpane onpaneload=""> ? >paneload events descend from the top, hit our prefwindow.paneload handler, >descend further down, hit our prefpane.paneload, hit toolkit's >prefpane.paneload, finally evaluate any given onpaneload attribute and then >bubble up again. That's not quite true. All of the event listeners we use are bubbling event listeners. I'm not sure whether our or toolkit's paneload event handler fires first, but our prefwindow's handler fires third and onpaneload fires last.

(In reply to comment #21) > >paneload events descend from the top, hit our prefwindow.paneload handler, > >descend further down, hit our prefpane.paneload, hit toolkit's > >prefpane.paneload, finally evaluate any given onpaneload attribute and then > >bubble up again. > That's not quite true. All of the event listeners we use are bubbling event > listeners. Yeah, true. Actually, I even did make a testrun last night and still wrote the nonsense above... :-[ > I'm not sure whether our or toolkit's paneload event handler fires > first, but our prefwindow's handler fires third and onpaneload fires last. If an XBL derivation and its ancestor both handle an event, the derived handler gets called first. See <http://developer.mozilla.org/en/docs/XBL:XBL_1.0_Reference:Event_Handlers>. _fireEvent then calls the onpaneload after the event processing.

>>If an XBL derivation and its ancestor both handle an event, the derived handler gets called first.<< ok, good; thats (part of) the bit I wasn't sure about. [and the onpaneload attrib, but I suspected that part was right]

Debug dump for opening preferences from browser (some warnings cut out): ########## sm.prefpane.paneload navigator_pane ########## ... WARNING: empty langgroup: file /home/kd/projekte/mozilla/mozilla.org/src/trunk/mozilla/gfx/thebes/src/gfxFont.cpp, line 871 ########## tk.prefpane.paneload navigator_pane ########## ########## sm.prefwindow.paneload navigator_pane ########## ########## onpaneload navigator_pane ##########

I checked in the architecture patch and the first two "demonstration" panel migrations on trunk. Any further panel migration can work independently of each other now, but we need every helping hand - it's like 50 panels we need to migrate! So, if you have the time to help out, please visit the wiki page <http://wiki.mozilla.org/SeaMonkey:Toolkit_Transition:PrefwindowPanes> where we coordinate the efforts and where I will put together some notes on How To Migrate A Pref Panel. ;-) Patches should be attached here, thus leaving the bug open for now.

Bah, I checked in the zzzz from comment #18 - and thus had to remove it; got r=Kairo over IRC for that.

Note: when migrating mouse wheel preferences, don't forget to add full zoom.

Came looking for a "Mac Preferences are broken" bug, and found this one. The comments suggest this is a work in progress. I'll just note that, in 2007110701, Pref panes aren't painting properly. Or, at all, in come cases. I haven't tested extensively, but I'll be happy to take thorough notes, if someone would find them valuable. Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O 10.4; en-US; rv:1.9a9pre) Gecko/2007110701 SeaMonkey/2.0a1pre

David: The problems are known, only 2 preferences panes are already migrated and working properly, the rest still needs to be done and therefore doesn't work/render correctly.

(In reply to comment #28) > Came looking for a "Mac Preferences are broken" bug, and found this one. The > comments suggest this is a work in progress. I bet you haven't seen the new menuitem at the bottom of the "Edit" menu :-)

(In reply to comment #30) > (In reply to comment #28) > > Came looking for a "Mac Preferences are broken" bug, and found this one. The > > comments suggest this is a work in progress. > > I bet you haven't seen the new menuitem at the bottom of the "Edit" menu :-) > Okay, Stefan, I'll play the straight man: "No, I didn't! Which new item on the bottom of the "Edit" menu?" ;-)

(In reply to comment #31) [...] > Okay, Stefan, I'll play the straight man: "No, I didn't! Which new item on the > bottom of the "Edit" menu?" > > ;-) > Edit -> (Legacy Prefwindow) That's where you find all the pref panes which haven't yet been migrated to use the new toolkit. The two (Migrated) items should be interpreted as meaning "Navigator" (or "Browser") and "Privacy & Security" respectively. They still have non-migrated subpanes.

We've had quite some confusion among nightly users about what happened to the pref dialog, because the new dialog is only reachable from the browser and contains some not yet working panels. This patch adds some temporary explanation panel to the new panel, hides the unmigrated ones and reincludes the original labels to the "(migrated)" entries.

Comment on attachment 288235 [details] [diff] [review] deconfuse pref dialogs Some spelling nits: >+ The SeaMonkey preferences are rewritten to use the same backend code as are being rewritten >+ other applications using the Mozilla XUL toolkit, eg. Firefox or e.g. >+ Thunderbird. We have like 50 panels to migrate, which we'll do in small We have over 40 panels to migrate, >+ chunks to guarentee we don't break anything. During this transition, to guarantee >+ this dialog here will only contain those panels which have already been >+ ported to new format. You can reach the old dialog via the menuitem migrated. You >+ Edit→(Legacy Prefwindow...) in a browser window or via the button below. or by clicking the button below. >+ <description> >+ This panel will go away once all panels have been migrated. >+ See <a xmlns="http://www.w3.org/1999/xhtml" >+ href="https://bugzilla.mozilla.org/show_bug.cgi?id=394522" >+ style="text-decoration: underline;" target="_blank">Bugzilla bug 394522</a> >+ for details and progress. >+ </description> Ideally you would use a <label class="text-link"> here. Maybe if you add the "This panel will go away" text to the top description, and just have a "More information..." link here?

Check-in version of attachment 288235 [details] [diff] [review], incorporating Neil's comments. Landed on trunk.

The introduction of the static info panel dragged a flaw in the initial panel selection out of the dark: on startup, toolkit only sees the static panels and selects one of them instead of a requested dynamic one... (Under Windows, the OS integration overlay is broken, which results in an incorrectly selected treeitem. I'll fix that in a separate patch later.)

Comment on attachment 288900 [details] [diff] [review] allow mixing static and dynamic panels >+ this._initialized = false; Why are we setting this false. I can't see a need for us to.

Setting this._initialized to false is the crucial fix of this patch! Without it, when passing a specific pane id (eg. navigator_pane), toolkit's _selectPane code will resize the prefwindow to height 0 here: <http://mxr.mozilla.org/seamonkey/source/toolkit/content/widgets/preferences.xml#765>. _initialized is only used in _selectPane and will (for us) effectively just call window.sizeToContent(); if needed.

To be "more" precise: If we have both static panels and autogenerated panels, toolkit will initialize and select a static panel before our prefwindow constructor gets called. We then autogenerate panels like navigator_pane and try to show them, but their height is still 0, so we end up with a very tiny dialog...

Comment on attachment 288900 [details] [diff] [review] allow mixing static and dynamic panels >- if (!this.currentPane) >+ if (selectPane) I don't like this. I think it would be better if you calculated the pane you wanted (based on arguments etc.) and see if it's already current or not.

Like this? (Most changes are whitespace changes due to the removed nesting level.)

Comment on attachment 289083 [details] [diff] [review] allow mixing static and dynamic panels, v2 [checked in] Landed on trunk.

This patch fixes various issues with the browser panel: - repair default browser settings under Windows by moving the relevant parts from platformPrefOverlay into #ifdef sections in pref-navigator.xul/js and preferences.xul. (Left the remaining treeitem in there, so that the legacy dialog won't break; it can be deleted as soon as we migrate pref-winhooks.) - make browser panel aware of instant-apply

Comment on attachment 289186 [details] [diff] [review] fix browser panel issues, v1 Just looking at the shell service bits: >+// Windows integration As the "SetAsDefault" behaviour is due to be implemented by the shell service, which could eventually exist on several platforms, I'd like the code to use detection rather than #ifdef, although obviously for the moment it will detect the windows hooks. Also I'd like the code to be platform neutral e.g. >+const kWinIntDeckNotDefault = 0; >+const kWinIntDeckDefault = 1; >+const kWinIntDeckPending = 2; kShellDeckXXX > // register our OK handler for the capturing(!) phase > window.addEventListener("dialogaccept", this.OnDialogAccept, true); Only do this when the set default button is clicked, and we're not using instant apply. Then you can use ApplySetAsDefaultBrowser as the listener. >+<!DOCTYPE overlay [ >+ <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> %brandDTD; >+ <!ENTITY % dtd1 SYSTEM "chrome://communicator/locale/pref/pref-navigator.dtd"> %dtd1; >+ <!ENTITY % dtd2 SYSTEM "chrome://communicator-platform/locale/pref/platformPrefOverlay.dtd"> %dtd2; >+]> You'll need to copy the entities from win/platformPrefOverlay.dtd (and remove all the platform overlay files, although you don't have to show that). >+ <vbox flex="1"> Nit: don't need flex on deck children >+ <hbox> >+ <spacer flex="1"/> >+ <button label="&defaultBrowserButton.label;" >+ oncommand="SetAsDefaultBrowser()"/> >+ <spacer flex="1"/> >+ </hbox> Nit: could use <hbox pack="center">. Or maybe forget the hbox and use <vbox align="center">. Or maybe put the button outside the deck. Note: whichever way, it occurs to me that you're switching a focused button with a disabled button. Ideally you should therefore focus something else. >+ <description pack="start">&defaultPendingText;</description> pack="start" is the default. (Does pack actually work on a description?) >+#ifdef XP_WIN >+ <!- - Windows integration is (obviously) only applicable on Windows - -> Don't bother with the winhooks panel.

Comment on attachment 289186 [details] [diff] [review] fix browser panel issues, v1 It seems to me that if you had a group, then change the input field, then click cancel, that you'll still lose the other pages?

Also adds radiobuttons for the full zoom feature.

Also supports the new full zoom prefrerence.

Comment on attachment 290008 [details] [diff] [review] Mouse wheel preferences (-w for review) Looks good r=me (without the password manager change in the full patch).

Comment on attachment 290010 [details] [diff] [review] Mouse wheel preferences >Index: suite/common/pref/pref-mousewheel.js >=================================================================== >+ for (var i = 0; i < checkboxes.length; i++) { >+ var cb = document.getElementById(checkboxes[i]); >+ var pref = document.getElementById(cb.getAttribute("preference")); >+ if (pref.value) >+ document.getElementById(fields[i]).disabled = true; You shouldn't need to ask the <preference> element, just make sure its ctor always sees the <checkbox> element: put the <preferences> block at the end of the <prefpane>!

JFTR: Toolkit <preference>s try to initialize observing elements, but toolkit <prefpane>s' paneload listener also does this. But our prefpane.paneload handler is executed before the toolkit one, so Startup() sees no initialized elements if they are defined in XUL after the observed <preference>.

This moves our paneload handler from the prefpane element to the prefwindow element so that it can fire Startup after the prefpane has done its stuff. The other change is that we can now read the checkbox directly of course.

This makes the enabling work in instant apply mode.

Given that bug 143065 is fixed, we can be consistent with our access keys.

Comment on attachment 289186 [details] [diff] [review] fix browser panel issues, v1 I've taken a brief look at this, but my main comment (like Neil said) is that we should auto detect what we've got for our platforms.

Comment on attachment 290595 [details] [diff] [review] Use fewer accesskeys r=me. Just one comment though: + <textbox id="mousewheelWithNoKeyNumlines" size="3" + checkbox="mousewheel.withnokey.sysnumlines" + preference="mousewheel.withnokey.numlines"/> I realise this just updating what is there, but could we do a follow-up patch to change this (and the other boxes) to be a number textbox?

Mouse wheel preferences checked in.

Comment on attachment 290595 [details] [diff] [review] Use fewer accesskeys Please change the labels in the locales files when the content change. So the localizers will notice the change when there tinderbox turn orange

> Please change the labels in the locales files when the content change. So the > localizers will notice the change when there tinderbox turn orange Which ones do you mean in particular here? (It makes no sense doing extensive renaming for stuff which may just live for only a limited time.)

(In reply to comment #59) >(From update of attachment 290595 [details] [diff] [review]) >Please change the labels in the locales files when the content change. So the >localizers will notice the change when there tinderbox turn orange In that patch, there was one major content change, and it was a new entity, and if that doesn't turn your tinderbox orange then I don't know what will.

(In reply to comment #61) > (In reply to comment #59) > >(From update of attachment 290595 [details] [diff] [review]) > >Please change the labels in the locales files when the content change. So the > >localizers will notice the change when there tinderbox turn orange > In that patch, there was one major content change, and it was a new entity, and > if that doesn't turn your tinderbox orange then I don't know what will. true :-) but there was also a (really) small change: -<!ENTITY scrollLines.label " lines"> -<!ENTITY scrollChars.label " characters"> +<!ENTITY scrollLines.label "lines"> +<!ENTITY scrollChars.label "characters"> it's nothing (i now) but i want to make sure that every developer is aware of the fact that they need to change the label if the change the content.

(In reply to comment #62) > but there was also a (really) small change: ...which was not semantic, just the removal of a space. > it's nothing (i now) but i want to make sure that every developer is aware of > the fact that they need to change the label if the change the content. Only if it's a semantic change, i.e. changes the meaning or use of a string in a way that localizers need to do an according change. That was probably not the case here.

Neil said in bug 404263 comment 8 that we're happy to change the id for the main treechildren to aid with updating overlays - i.e. we can do two sets of treechildren in the same file - one for the old style window, one for the new (extensions can also use this to work with both versions as well). I thought it best to separate it out to this bug, so here is the change. I decided adding "prefs" would be more descriptive than what it is now.

This update has quite some changes: - fixed Mark's comment #56 and Neil's comment #45, especially: - (preparations for) autodetecting default browser settings, - rewrote default browser XUL - better instantApply handling - killed platformPrefOverlay.xul & Co. (that's the main noise here) - left out WinHooks panel - fixed Neil's comment #46: - worked around newly found toolkit bug 410562 *sigh* - rewrote homepage group handling - killed the accesskey workaround now that accesskeys-in-decks issues are fixed Please test especially the Windows default browser settings, since I don't have a Windows dev environment (I squeezed it into a normal end user build to test, though, which seemed to work). I left out the actual removal of the platformPrefOverlay.xul and .css files.

Comment on attachment 295170 [details] [diff] [review] fix browser panel issues, v2 >Index: suite/locales/en-US/chrome/common/pref/pref.dtd >=================================================================== >+<!ENTITY prefWindowWin.size "width: 52em; height: 41em;"> >+<!ENTITY prefWindowMac.size "width: 58em; height: 41em;"> Please, 1) leave this spread to three variants: Win/Mac/unix(GNOME), in the same way as it's done in http://mxr.mozilla.org/mozilla/source/browser/components/preferences/preferences.xul#85 2) As you're already touching this, please convert it to use ch units for width, see bug 380378

(In reply to comment #66) >Please, >1) leave this spread to three variants: Win/Mac/unix(GNOME), in the same way as >it's done in >http://mxr.mozilla.org/mozilla/source/browser/components/preferences/preferences.xul#85 >2) As you're already touching this, please convert it to use ch units for >width, see bug 380378 Actually once you're using ch units you shouldn't need separate variants.

(In reply to comment #67) > Actually once you're using ch units you shouldn't need separate variants. Theoretically, that's true, but I'm not sure how it works out in practice, as prefs is a very special case of dialog, and I'd vote for keeping it as flexible as possible, esp. as long as we don't know if the same ch width actually does work fine in all languages on all platforms equally.

(In reply to comment #65) > - killed platformPrefOverlay.xul & Co. (that's the main noise here) Sure, kill platformPerfOverlay.xul, but no need to kill the .dtd is there?

(In reply to comment #66) > 1) leave this spread to three variants: Win/Mac/unix(GNOME) I didn't do this since in other places we also just differ between the different versions actually _needed_, not those theoretically possible (eg. <http://mxr.mozilla.org/seamonkey/source/toolkit/content/widgets/preferences.xml#512 > and following lines, which we copied (sanitized)). I don't mind which version we use, but it should be consistent. > 2) As you're already touching this, please convert it to use ch units for > width, see bug 380378 Okay, will do. (In reply to comment #68) > esp. as long as we don't know if the same ch width actually does > work fine in all languages on all platforms equally. I'm having trouble with the em dialog width under Linux/KDE/Raleigh anyway, but I can test at least with Linux vs. Mac. (In reply to comment #69) > > - killed platformPrefOverlay.xul & Co. (that's the main noise here) > Sure, kill platformPerfOverlay.xul, but no need to kill the .dtd is there? What's the point of having these .dtd around if noone would use them? After moving out the default browser stuff to pref-navigator.xul and the prefWindow sizes to pref.dtd, the content is only used by pref-tabs.xul, so I don't see any need for keeping the platformPrefOverlay.dtd files... Given that the ch issue is rather minor, I'd like to wait for other review comments before updating the patch (yeah, I know I forgot to remove an already commented out style hack (used in Raleigh since it doesn't draw groupbox outlines) ;-) ).

(In reply to comment #70) > (In reply to comment #66) > > 1) leave this spread to three variants: Win/Mac/unix(GNOME) > > I didn't do this since in other places we also just differ between the > different versions actually _needed_, not those theoretically possible Sure, but we usually (from my experience) run into locales that need the pref window differently sized on Win/Linux (even Firefox came to need this) - I didn't debate the rest, though I don't particularly like calling things "Win" in key names when they get used for anything but Mac.

(In reply to comment #70) >(In reply to comment #69) > > > - killed platformPrefOverlay.xul & Co. (that's the main noise here) > > Sure, kill platformPerfOverlay.xul, but no need to kill the .dtd is there? > What's the point of having these .dtd around if noone would use them? After > moving out the default browser stuff to pref-navigator.xul and the prefWindow > sizes to pref.dtd, the content is only used by pref-tabs.xul, so I don't see > any need for keeping the platformPrefOverlay.dtd files... Ah, I didn't realise that mousewheel prefs uses platformCommunicatorOverlay.dtd - would you mind moving tab browsing pref strings there too?

Having two pref windows is counted as "broken functionality" per http://home.kairo.at/blog/2008-01/seamonkey_2_alpha_criteria as discussed in the Council - therefore this bug blocks Alpha.

(In reply to comment #72) > Ah, I didn't realise that mousewheel prefs uses platformCommunicatorOverlay.dtd ? But they don't?!

(In reply to comment #74) >(In reply to comment #72) >>Ah, I didn't realise that mousewheel prefs uses platformCommunicatorOverlay.dtd >? >But they don't?! suite/common/pref/pref-mousewheel.xul, line 139: <tab label="&usingWheelAndAlt.label;"/> suite/locales/en-US/chrome/common/pref/pref-mousewheel.dtd, line 7: <!ENTITY usingWheelAndAlt.label "&altKey.label;"> suite/locales/en-US/chrome/common/mac/platformCommunicatorOverlay.dtd, line 59: <!ENTITY altKey.label "Option">

Sorry, I got confused by platformCommunicatorOverlay.dtd vs. platformPrefOverlay.dtd. So you want me to put the urlbar.* and middleclick.* entities into platformCommunicatorOverlay.dtd, thus avoiding the #ifdef in pref-tabs.dtd?

(In reply to comment #76) > So you want me to put the urlbar.* and middleclick.* entities into > platformCommunicatorOverlay.dtd, thus avoiding the #ifdef in pref-tabs.dtd? I forgot to mention that I don't think that this would be a good idea, because these strings clearly don't have any SM-wide use...

Comment on attachment 295170 [details] [diff] [review] fix browser panel issues, v2 By code inspection, there seem to be some issues with this patch: * currentGroupButton isn't disabled when the tabbrowser only has one tab * SetHomePageToCurrentGroup doesn't work because it tries to stuff the whole home page group into a single-line textbox * RestoreHomePageGroup restores to the previous home page group, but the old button restored to the default home page I'd also prefer it if you used browserWindow.getBrowser() (In reply to comment #76) > So you want me to put the urlbar.* and middleclick.* entities into > platformCommunicatorOverlay.dtd, thus avoiding the #ifdef in pref-tabs.dtd? I'd prefer them in a platform overlay, yes.

(In reply to comment #78) > * currentGroupButton isn't disabled when the tabbrowser only has one tab It's a group - just with one member. But, okay... > * SetHomePageToCurrentGroup doesn't work because it tries to stuff the whole > home page group into a single-line textbox It's a multiline textbox now, so users can actually see what their homepage group is. > * RestoreHomePageGroup restores to the previous home page group, Yes. > but the old button restored to the default home page No, it didn't. It just restored the first homepage found on dialog startup (which already made me wonder when I first migrated that panel). > I'd also prefer it if you used browserWindow.getBrowser() Instead of what? > > So you want me to put the urlbar.* and middleclick.* entities into > > platformCommunicatorOverlay.dtd, thus avoiding the #ifdef in pref-tabs.dtd? > I'd prefer them in a platform overlay, yes. Then we probably shouldn't kill platformPrefOverlay.dtd in the first place (but platformPrefOverlay.xul still can go, since we don't have any different XUL stuff)?

(In reply to comment #79) > It's a multiline textbox now Ah! Sorry, I'd overlooked that. > > but the old button restored to the default home page > No, it didn't. Yes it did, because it used the default pref. > > I'd also prefer it if you used browserWindow.getBrowser() > Instead of what? document.getElementById("content") > > I'd prefer them in a platform overlay, yes. > Then we probably shouldn't kill platformPrefOverlay.dtd in the first place New alternative: add an entity for accelKey.label = Cmd/Control in platformCommunicatorOverlay.dtd and reference that in pref-tabs.dtd

Comment on attachment 290088 [details] [diff] [review] Move prefpane event handler >-prefpane { >- -moz-binding: url("chrome://communicator/content/bindings/prefwindow.xml#prefpane"); >-} It turns out I can't actually remove this because Modern needs it.

Comment on attachment 290088 [details] [diff] [review] Move prefpane event handler The most severe issue with this is something else: currently, a 'paneload' event is dispatched to the <prefpane> element, first caught by SM's <prefpane> 'paneload' handler (where Startup is called, allowing dynamic creation of <preference> elements before they are used to init the UI), then by toolkit's <prefpane> 'paneload' handler (where UI elements get fed with their <preference> data), then by SM's <prefwindow> 'paneload' handler (syncing <tree> and <pane>), and finally UI element's 'onpaneload' attribute is evaluated. Dropping the <prefpane> 'paneload' handler would mean some event phase tinkering, which I think isn't exactly easier to comprehend. If you need to read a <preference> value via the UI element, put your function into the <prefpane>'s onpaneload handler (but it's probably better to just ask the <preference> directly in Startup()).

After some IRC discussion I decided to keep the platformPrefOverlay.dtd files while still dropping the platformPrefOverlay.xul ones - it's much cleaner this way. (The actual CVS removals are not shown in this patch.) Fixed also Neil's nits about ch usage, button disabling, default homepage etc.

Correct patch file this time; see comment #83 for details.

Comment on attachment 300479 [details] [diff] [review] [wrong patch] <!-- LOCALIZATION NOTE : this is part of an inline-style attribute on the preference dialog's <window> node, which specifies the width and height in em units of the dialog. Localizers ONLY can increase these widths if they are having difficulty getting panel content to fit. 1em = the width of the letter 'm' in the selected font. XUL/FE DEVELOPERS: DO NOT MODIFY THIS VALUE. It represents the correct size of this window for en-US. --> -<!ENTITY prefWindow.size "width: 58em; height: 41em;"> +<!ENTITY prefWindow.title "Preferences"> +<!ENTITY prefWindow.size "width: 80ch; height: 41em;"> + 68ch looks more reasonable on mac. Note that the localization comment is wrong now (no dialog, units are in ch and em).

sigh, wrong attachment - I ment to comment on attachment #300481 [details] [diff] [review], of course

Comment on attachment 300481 [details] [diff] [review] fix browser panel issues, v3 Btw, I must say that I question the usage of the multiline textbox for the homepage prefs. I find it hard to see the benefits of a scrollable textfield with all your URI's (if you have a groupmark). It looks more to me like a power-user pref. Normal user will wonder why the textbox is so big. And if you did remembered a bunch of pages (or keywords) that you wanted to bookmark, wouldn't you rather just type/paste them in the url bar instead?

some notes for now... This does indeed fix the Debug Panel (appearing blank), will probably correct MReimers overlay issue (so he should be able to remove his dirty hack; see newsgroup). YAY! I'm not fond of the new layout for the homepage pref, I do feel that is an (highly?) advanced usage of it, that we probably don't need to expose here. [Besides it makes the prefwindow too short for it on windows]. for a spinoff bug (probably): Error: _elementIDs is not defined Source File: chrome://messenger-mapi/content/pref-mailnewsOverlay.xul Line: 50 That is why the mailnews pane does not want to show for us now... I'll give a proper review later...

> Btw, I must say that I question the usage of the multiline textbox for the > homepage prefs. I'll try to explain my reasoning: - If you want to set a homepage, it's usually (AFAICT) the current page or group you're viewing. It seems rather unlikely that someone went to Preferences and started typing, musing "let's see what urlbar history thinks I can have as a homepage". => autocomplete is nice, but not essential - If you have set a tab group as your homepage, there's no way currently to review that list, except by opening it (and thus causing a lot of poor electrons to wiggle around without real need). => we need a way to display the group of homepages in the prefs - (Enhancement: It'd be nice if you could set multiple keyword searches as your homepages.) My current implementation tries to balance these points. I want my homeIt seems extremely unlikely to say - If you want to change you homepage, you have to go to Preferences. There's no way to see your current homepage(s) before I find it hard to see the benefits of a scrollable textfield > with all your URI's (if you have a groupmark). It looks more to me like a > power-user pref. Normal user will wonder why the textbox is so big. And if you > did remembered a bunch of pages (or keywords) that you wanted to bookmark, > wouldn't you rather just type/paste them in the url bar instead? >

Uh, how I hate this mini textbox! Ignore the last comment, read this one: Comment #87: > Btw, I must say that I question the usage of the multiline textbox for the > homepage prefs. I'll try to explain my reasoning: - If you want to set a homepage, it's usually (AFAICT) the current page or group you're viewing. It seems rather unlikely that someone went to Preferences and started typing, musing "let's see what urlbar history thinks I can have as a homepage". => autocomplete is nice, but not essential - If you have set a tab group as your homepage, there's no way currently to review that list, except by opening it (and thus causing a lot of poor electrons to wiggle around without real need). => we need a way to display the group of homepages in the prefs - (Enhancement: It'd be nice if you could set multiple keyword searches as your homepages.) My current implementation tries to balance these points. Comment #88: > [Besides it makes the prefwindow too short for it on windows]. Hm, shouldn't be, of course. OTOH, when Ratty finishes his toolbar customization, we can get rid of the button prefs anyway...

(In reply to comment #90) > Uh, how I hate this mini textbox! > Ignore the last comment, read this one: > > Comment #87: > > Btw, I must say that I question the usage of the multiline textbox for the > > homepage prefs. > > I'll try to explain my reasoning: > - If you want to set a homepage, it's usually (AFAICT) the current page or > group you're viewing. It seems rather unlikely that someone went to Preferences > and started typing, musing "let's see what urlbar history thinks I can have as > a homepage". > => autocomplete is nice, but not essential Yeah, well - I guess I put more weight on "nice" than essential here ;-) > - If you have set a tab group as your homepage, there's no way currently to > review that list, except by opening it (and thus causing a lot of poor > electrons to wiggle around without real need). > => we need a way to display the group of homepages in the prefs Yeah, but wouldn't the most useful way to review your homepage tabgroup be to to just hit the home button? Would it be better to look at a list of 10-15 URI's? Besides, most people that are interested in the homepage prefs would already have a browser window open, no? > - (Enhancement: It'd be nice if you could set multiple keyword searches as your > homepages.) This is the most valid point of your reasoning, I think. Imo, the only argument against this would be that it's really a power-user feature.

Comment on attachment 300481 [details] [diff] [review] fix browser panel issues, v3 >+<!DOCTYPE overlay [ >+ <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> %brandDTD; >+ <!ENTITY % dtd1 SYSTEM "chrome://communicator/locale/pref/pref-navigator.dtd"> %dtd1; >+]> Nit: I'd call this something like navigatorDTD rather than dtd1. >+ <groupbox id="defaultBrowserGroup" flex="1" align="center" hidden="true"> Having seen what happens when your preferences window is too wide ;-) I see that this groupbox flexes depending on the current description... to reduce that effect I suggest increasing the flex here, say to 1000. >+ <description id="defaultBrowserDesc" >+ desc0="&makeDefaultText;" >+ desc1="&alreadyDefaultText;" >+ desc2="&defaultPendingText;"/> Hmm... the button also moves depending on the amount of text. Maybe flex here? >+<!ENTITY prefWindow.title "Options"> What's this for? Fortunately the correct pref title is in pref.dtd; please remove this entity from the platform pref files. >-<!ENTITY prefWindow.size "width: 52em; height: 41em;"> >+<!ENTITY prefWindow.size "width: 72ch; height: 41em;"> This is far too wide. 70ch would have been the absolute limit; by my calculations (which I might have written incorrectly or you may have misinterpreted) it should be 63 or 64, but obviously bump it up a little if the Mac needs it - I'd prefer all three sizes to be the same.

(In reply to comment #91) [...] > Yeah, but wouldn't the most useful way to review your homepage tabgroup be to > to just hit the home button? Would it be better to look at a list of 10-15 > URI's? Besides, most people that are interested in the homepage prefs would > already have a browser window open, no? [...] In Suiterunner (Sm2) my homepage consists of over 30 tabs. AFAIK, the most useful way to review it (and without needless bandwidth consumption) currently consists of loading the about:config page, then typing "homepage" (without the quotes of course) in the Filter box. Yes, looking at a list of 30-some URIs, even with a need for scrolling, is better than waiting a sizable time for them to load, then walking the tabs all the way across the screen. Not to mention that I might have a non-default (non-"home") set of tabs open at the time, which I wouldn't want to lose. IMHO it would be nice to be able to see the list (instead of "-- Home Page Group is Set --") in the "Browser" prefpane. Or maybe in an additional popup by clicking a button there. Currently using "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008013119 SeaMonkey/2.0a1pre".

(In reply to comment #91) > [...] Imo, the only argument > against this would be that it's really a power-user feature. > So? Isn't SeaMonkey meant to be useful for everyone, even power users? ;-) Up to now, I thought they were more "at ease" with the Suite than with Fx&Tb (where the Preferences in particular, have a kind of "kindergarten" look & feel IYSWIM).

Comment on attachment 300481 [details] [diff] [review] fix browser panel issues, v3 I'm still not ecstatic over the homepage changes, though a few people seem to think an alpha tryout is a good idea, so I'm not going to block on that. Given Neil's nits, I'll add two more.. Please increase the height of the dialog to fit the toolbar customize stuff. [we can re-adjust whenever Ratty's customization patch lands] >+function InitPlatformIntegration() >+ // In future, we will ask the Shell Service about platform integration here, >+ // for now, just check WinHooks. >+ var showDefaultBrowserGroup = /Win/.test(navigator.platform); >+ if (showDefaultBrowserGroup) > { ... can we loose an indent level here? possibly by: var showDefaultBrowserGroup = /Win/.test(navigator.platform); document.getElementById("defaultBrowserGroup").hidden = !showDefaultBrowserGroup; if (!showDefaultBrowserGroup) return;

Checked-in fix (plus CVS removal of the platformPrefOverlay.xul files), incorporating changes for the latest nits by Neil and Callek. Since the dialog width values could not be computed to the same ch value on all platforms (and even differed between my and Neil's Windows), I left the width untouched; work on that will happen in bug 380378 (I think).

Bug 416274 is relevant to anyone porting proxy preferences.

The code removal is/was blocked by this, not the other way around. Marking this FIXED! Yay! \o/ Thanks to all who helped!