For ChatZilla to work with the new SeaMonkey2 prefwindow (and SeaMonkey's appearance pref panel not breaking with rework to the new window), we need some changes to the current pref overlaying ChatZilla is doing. This won't break old Mozilla/SeaMonkey 1.x integration. Patch upcoming when I've tested it.

Here's a patch for this - with several comments: 1) We need to use two separate overlays for the tree and panel parts in SeaMonkey 2 as the pane itself is just an overlay on the prefwindow but loaded only when needed so at the time the main overlay is loaded, the pane elements are not there and if we'd overlay the pane with the same overlay, we'd have the same overlay loaded twice in the same window - using two overlays is the way to go. 2) The elements for 1.x don't match and get dropped at the time the main overlay is loaded, so this fits well. 3) The manifest lines at the top of the jar.mn get only used for toolkit-based apps, so those should load what SM2 needs, 1.x is overlayed/supported by contents.rdf overlays (we don't care about unsupporting older toolkit-based trunk nightlies). 4) The appearance pref overlay is for the new appearance panel from bug 411215 but currently this overlaying breaks because of bug 330458, I'll remove the comment with the checkin for bug 411215 once it can happen.

Comment on attachment 303711 [details] [diff] [review] patch: make ChatZilla pref overlays ready for SeaMonkey2 On code inspection. >Index: mozilla/extensions/irc/jar.mn >=================================================================== >-% overlay chrome://communicator/content/pref/preftree.xul chrome://chatzilla/content/prefsOverlay.xul application={92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a} >-% overlay chrome://communicator/content/pref/pref-appearance.xul chrome://chatzilla/content/prefsOverlay.xul application={92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a} >+% overlay chrome://communicator/content/pref/preferences.xul chrome://chatzilla/content/prefsOverlay.xul application={92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a} >+###% overlay chrome://communicator/content/pref/pref-appearance.xul chrome://chatzilla/content/prefsAppearanceOverlay.xul application={92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a} I prefer filename prefAppearanceOverlay.xul >+ content/chatzilla/pref-irc-main.xul (xul/content/pref-irc-main.xul) >+ content/chatzilla/prefsAppearanceOverlay.xul (xul/content/prefsAppearanceOverlay.xul) See comment above and I prefer pref-irc2.xul but not that bothered on that one. >Index: mozilla/extensions/irc/xul/content/prefsOverlay.xul >=================================================================== >- <!-- Startup option --> >+ <!-- Startup option (Mozilla/SeaMonkey 1.x) --> > <script type="application/x-javascript"> > <![CDATA[ > var panel; > if (panel != undefined && > (panel == "chrome://communicator/content/pref/pref-appearance.xul")) > _elementIDs.push("generalStartupChat"); > ]]> Chatzilla needs to work with every possible version of SM/Fx/etc so you will probably have to consider moving this into prefAppearanceOverlay.xul as that is what you are overlaying pref-appearance.xul with. >Index: mozilla/extensions/irc/xul/content/prefsAppearanceOverlay.xul >=================================================================== See comment about filename above. >+<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> See comment above about moving pre-SM2.x pref-apperance.xul entries into here. >+ <!-- Startup option (SeaMonkey 2) --> >+ <preferences id="appearance_preferences"> >+ <preference id="general.startup.chat" >+ name="general.startup.chat" >+ type="bool"/> >+ </preferences> >+ <groupbox id="generalStartupPreferences"> Change id to "generalStartupPrefs" as to not confuse with pre-SM2.x stuff >+ <checkbox id="generalStartupChat" You will need some insertafter entries "generalStartupBrowser,generalStartupEditor,generalSta rtupAddressBook" so that it appears at the end of the list. >+ label="&startup.chat.label;" >+ accesskey="&startup.chat.accesskey;" >+ preference="general.startup.chat"/>

(In reply to comment #2) > Chatzilla needs to work with every possible version of SM/Fx/etc so you will > probably have to consider moving this into prefAppearanceOverlay.xul as that is > what you are overlaying pref-appearance.xul with. Fx/etc don't use any of those overlays, and Moz/SM 1.x use the overlays via contents.rdf (as I referenced in my comments about the patch), which overlay both preftree and (the old) pref-appearance with the prefOveray, only SM2 will use the manifest entries that lead it to apply prefsOverlay to preferences.xul and prefAppearanceOverlay to pref-appearance. The <groupbox id="generalStartupPreferences"> would be no problem, as the overlay on prefereces.xul is loaded before pref-appearance is loaded and that part would be dropped at that stage ;-)

Comment on attachment 303711 [details] [diff] [review] patch: make ChatZilla pref overlays ready for SeaMonkey2 >Index: mozilla/extensions/irc/jar.mn >=================================================================== >RCS file: /cvsroot/mozilla/extensions/irc/jar.mn,v >retrieving revision 1.37 >diff -u -7 -p -r1.37 jar.mn >--- mozilla/extensions/irc/jar.mn 21 Dec 2007 15:28:11 -0000 1.37 >+++ mozilla/extensions/irc/jar.mn 16 Feb 2008 12:43:05 -0000 >@@ -1,14 +1,14 @@ > chatzilla.jar: > % content chatzilla %content/chatzilla/ > % skin chatzilla modern/1.0 %skin/modern/chatzilla/ > % overlay chrome://browser/content/browser.xul chrome://chatzilla/content/browserOverlay.xul application={ec8030f7-c20a-464f-9b0e-13a3a9e97384} > % overlay chrome://editor/content/editorTasksOverlay.xul chrome://chatzilla/content/chatzillaOverlay.xul application={92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a} >-% overlay chrome://communicator/content/pref/preftree.xul chrome://chatzilla/content/prefsOverlay.xul application={92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a} >-% overlay chrome://communicator/content/pref/pref-appearance.xul chrome://chatzilla/content/prefsOverlay.xul application={92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a} >+% overlay chrome://communicator/content/pref/preferences.xul chrome://chatzilla/content/prefsOverlay.xul application={92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a} >+###% overlay chrome://communicator/content/pref/pref-appearance.xul chrome://chatzilla/content/prefsAppearanceOverlay.xul application={92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a} If you haven't tested this, it's not going in yet. Pretty simple. r- for that. > <snip> >Index: mozilla/extensions/irc/xul/content/prefsOverlay.xul >=================================================================== > <snip> > >- <!-- Startup option --> >+ <!-- Startup option (Mozilla/SeaMonkey 1.x) --> > <script type="application/x-javascript"> > <![CDATA[ > var panel; > if (panel != undefined && > (panel == "chrome://communicator/content/pref/pref-appearance.xul")) > _elementIDs.push("generalStartupChat"); > ]]> This code will run on SM 2 too, no? Wouldn't that remove the need for all this silly overlaying magic? >Index: mozilla/extensions/irc/xul/content/pref-irc-main.xul Major code duplication... isn't there a smarter option? Why not? I'm afraid I don't understand the 5-line one-sentence-paragraph in point 1 of your comment, if that was meant to explain this. The file name is also pretty strange - can't you make it indicate that it's for toolkit support? Offhand I would have no idea what the difference between "pref-irc-main.xul" and "pref-irc.xul" would be. Seems rather arbitrary. >Index: mozilla/extensions/irc/xul/content/prefsAppearanceOverlay.xul Instead of adding this second overlay, why not just make the first one actually work? (also refer to the earlier comment about the script running

(In reply to comment #2) > Chatzilla needs to work with every possible version of SM/Fx/etc so you will > probably have to consider moving this into prefAppearanceOverlay.xul as that is > what you are overlaying pref-appearance.xul with. Nope, in old 1.x versions that need this script, we're overlying only prefsOverlay to both preftree and the pane - in SM2 we're using the manifest and prefAppearanceOverlay. I think it's actually better to keep the old code in the old place and don't change how it worked there. (In reply to comment #4) > If you haven't tested this, it's not going in yet. Pretty simple. r- for that. Bug 419452 fixed loading that overlay now, so it can be tested, and it works! New patch upcoming. > This code will run on SM 2 too, no? Wouldn't that remove the need for all this > silly overlaying magic? This much-cleaner-than-before overlay "magic" (which is less magic than what we're doing for 1.x) is something different than the script you're commenting on. That script is only for pushing pref elements, which is is an xpfe-ism which is not needed on toolkit-based prefwindows. > >Index: mozilla/extensions/irc/xul/content/pref-irc-main.xul > > Major code duplication... isn't there a smarter option? I don't know of any if we want to keep compat with both SeaMonkey/Mozilla 1.x and SeaMonkey 2. > Why not? Because the new toolkit-based prefwindow has prefpanels as overlays, while in the old xpfe-based one they were iframes - so in the new one the panel has a <overlay> (and a <prefpane>) as the top-level element, which the old one has a <page> - I have no idea how that would be made cleanly in the same file. > The file name is also pretty strange - can't > you make it indicate that it's for toolkit support? Offhand I would have no > idea what the difference between "pref-irc-main.xul" and "pref-irc.xul" would > be. Seems rather arbitrary. It is. I'm open to any suggestion that looks more clear for you. > >Index: mozilla/extensions/irc/xul/content/prefsAppearanceOverlay.xul > > Instead of adding this second overlay, why not just make the first one actually > work? (also refer to the earlier comment about the script running) The first one does work - but it can only work for the pref tree and not for the pane. The main problem there is that in the Moz/SM 1.x world, we apply the same overlay to the window with the preftree and as well to the pane, which is loaded in an iframe - this is a bit dirty, but it works fine. In the new world of the toolkit-based SeaMonkey 2 prefwindow, the preftree is still part of the main window, overlayed by prefsOverlay as usual (but with different markup). Once the user clicks on the entry in the preftree (long after overlays to the main window have been applied!), the panel is dynamically being loaded as an overlay into the prefwindow. So everything but the parts of prefsOverlay that apply to the preftree have already been discarded at this stage, but if we'd apply it again to the pane, we'd end up with having the same overlay loaded twice, which breaks the window. If we overlay the pane with a _different_ overlay, everything works out right, though. And actually, once ChatZilla drops support for Mozilla/SeaMonkey 1.x some time in the future, we'll end up with two clean overlays that only apply to their specific parts of the window.

OK, here's a second patch, addressing the previous comments. This has been fully tested now that the overlays work fine in current trunk - if you want to test yourself, it only fully works with the new patch I'll attach shortly in bug 411215 (that other patch cannot land without this one though, as the old overlay definitions would break the new pane there due to the double overlay mentioned in my previous comment here). If the patch here does land before bug 411215, the ChatZilla item will be missing from the old appearance pane on trunk (not on 1.x) for the moment, but if my patch on bug 411215 lands before the ChatZilla change, the whole appearance pane will be broken.

(In reply to comment #5) >In the new world of the toolkit-based SeaMonkey 2 prefwindow, the preftree is >still part of the main window, overlayed by prefsOverlay as usual (but with >different markup). Once the user clicks on the entry in the preftree (long >after overlays to the main window have been applied!), the panel is dynamically >being loaded as an overlay into the prefwindow. So everything but the parts of >prefsOverlay that apply to the preftree have already been discarded at this >stage, but if we'd apply it again to the pane, we'd end up with having the same >overlay loaded twice, which breaks the window. If we overlay the pane with a >_different_ overlay, everything works out right, though. Loading the same overlay twice works fine here.

Note that this version assumes that the groupbox id doesn't get changed. (I haven't read bug 411215 so I don't know why KaiRo changed it.)

Comment on attachment 306076 [details] [diff] [review] patch v2: update for comments, fully tested >+ insertafter="composer,mailnews,navigator" It's called editor, not composer, and mailnews makes no sense here.

Whoa, what that patch does is so messy. And yes, we _want_ to be listed after mailnews if it's installed.

Comment on attachment 306239 [details] [diff] [review] Leave the checkbox in the main overlay OK, let's go with that then. I tested it here and it works fine.

Comment on attachment 306239 [details] [diff] [review] Leave the checkbox in the main overlay >Index: xul/content/prefsOverlay.xul >=================================================================== >+ wsm_persist="true" preference="general.statup.chat"/> Make this "general.startup.chat" (typo corrected) and it even works with the new patch I'm about to attach to bug 411215 in a moment :)

(In reply to comment #10) >Whoa, what that patch does is so messy. Hey, it was only a suggestion. >And yes, we _want_ to be listed after mailnews if it's installed. Except that composer is already listed after mailnews, and you can't install mailnews without composer...

I just realized I've left this for a while now - I'll try to get to it late tonight. Robert, does Neil's last comment apply? It seems to make sense to me - ChatZilla does not require mailnews. On the other hand, we're not quite as important to suite users as mailnews, probably. Are there other solutions?

well, the current patch has |insertafter="composer,mailnews,navigator"| which means "insert after the item with id='composer', if it doesn't exist, after 'mailnews', if it doesn't exist after 'navigator'". As Neil has pointed out, the correct ID to use for Composer is 'editor' though, so I have changed that locally. This rule results in the correct order, but Neil pointed out that the fallbacks might not be needed - OTOH, the "editor" panel is not ported to the new prefwindow yet and missing there, so actually, we need that fallback to have the correct position right now - even if it might not matter once all panels are ported (should be done for SeaMonkey 2 alpha). So, basically, I'll go with whatever you prefer there.

Hum. So I don't understand why this doesn't break in pre-toolkit (assuming you've tested that...). AFAICT, XPFE doesn't have any implementation for <preference[s]>, it's not in any of the dtd's, so why doesn't XUL complain? Can someone enlighten me to what I'm missing?

XUL doesn't care about DTDs for determining which tags are legal or not - it wouldn't be easy to do XBL if we did. The good thing is that an overlay just ends up ignoring and dropping any tag+id that doesn't match a tag+id in the overlaid document, so we can put all those entries for old and new suites into one document. Additionally, the chrome.manifest line defined in jar.mn is only used in versions that use the new toolkit and xpfe versions use the contents.rdf entries, which are ignored by the new toolkit when a chrome.manifest is found. The only slight issue that comes up with that (and which we should file a followup for) is that we should care that make*xpi.sh create a correct chrome.manifest and package it, but we should do that independently of this bug here. (Note that packages created through the Mozilla build system automatically end up with that file in the right location already.)

Comment on attachment 306239 [details] [diff] [review] Leave the checkbox in the main overlay Alright, so r=me with the s/composer/editor/ and the followup bug filed, I guess.

OK, checked in and filed bug 431715 on the chrome.manifest packaging by make*xpi.sh