Closed
Bug 335154
Opened 19 years ago
Closed 18 years ago
Get SeaMonkey's themes registering and switching with Theme Manager (in suiterunner)
Categories
(SeaMonkey :: General, defect)
SeaMonkey
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(11 files, 9 obsolete files)
5.07 KB,
image/png
|
Details | |
1.64 KB,
image/png
|
Details | |
7.64 KB,
patch
|
standard8
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
422 bytes,
image/png
|
Details | |
2.20 KB,
image/png
|
Details | |
1.27 KB,
patch
|
kairo
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
10.46 KB,
patch
|
standard8
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
10.18 KB,
patch
|
kairo
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
645 bytes,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
2.16 KB,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
2.87 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
Now that we have theme manager, we need to:
- Set up the our existing themes for registration with the new theme manager
- Remove the old theme controls
Assignee | ||
Comment 1•19 years ago
|
||
The existing theme controls aren't any use in suiterunner. Therefore we should remove them in preference of the Theme Manager IMO. This patch adds in some temporary ifdefs that we can get rid of once we throw the switch.
Attachment #219520 -
Flags: superreview?(neil)
Attachment #219520 -
Flags: review?(neil)
Assignee | ||
Comment 2•19 years ago
|
||
This patch does the necessary to make the modern theme show up in extension manager. It actually needs some additional prefs to switch - I'll post those in a followup patch once I've looked at them in more detail.
It also needs two image files to go into suite/themes/modern which I'll attach in a moment.
Attachment #219534 -
Flags: superreview?(neil)
Attachment #219534 -
Flags: review?(neil)
Assignee | ||
Comment 3•19 years ago
|
||
The preview icon for the modern theme - to be added to suite/themes/modern. Based on the reload button and converted to a png.
Assignee | ||
Comment 4•19 years ago
|
||
Preview image for the modern theme - taken from the existing one, just converted to a png and will go into suite/themes/modern.
Assignee | ||
Comment 5•19 years ago
|
||
Comment on attachment 219535 [details] [diff] [review]
icon for modern theme
Try getting the right attachment type ;-)
Attachment #219535 -
Attachment is obsolete: true
Assignee | ||
Comment 6•19 years ago
|
||
Correct attachement type.
Assignee | ||
Comment 7•19 years ago
|
||
Comment on attachment 219534 [details] [diff] [review]
Part 2 - make the modern theme compatible with the new themes manager.
Thinking about it, it'd be useful if Kairo checks this patch as well.
Attachment #219534 -
Flags: review?(neil) → review?(kairo)
Assignee | ||
Comment 8•19 years ago
|
||
This attachment contains a provisional list of preferences for addition to browser-prefs.js. They are related to updates or extensions. We need the extension ones for certain so that the extension manager will work correctly. The update ones I'd thought I'd include as we are looking at prefs related areas - though will we be able to use the update mechanism? I've set updates off for app ones for the time being, and on for extensions, though obviously it won't really work just yet (unless we have some extensions that are already enabled for suiterunner.
I need to make this into a proper patch, but any comments before I do that would be very useful.
Comment 9•19 years ago
|
||
Comment on attachment 219588 [details]
Proposed addition to browser-prefs.js
Looks reasonable. I assume these will be #ifdef MOZ_XUL_APP (and the existing ones will be in the #else)?
>// Interval: When all registered timers should be checked (in milliseconds)
>// default=5 seconds
>pref("app.update.timer", 600000);
Looks like 10 minutes to me - someone forgot to update the comment ;-)
Comment 10•19 years ago
|
||
Comment on attachment 219588 [details]
Proposed addition to browser-prefs.js
>// A default value for the "More information about this update" link
>// supplied in the "An update is available" page of the update wizard.
>pref("app.update.url.details", "chrome://browser-region/locale/region.properties");
browser-region does not exist in SeaMonkey. Set this to communicator-region and add the correct line in that file along with a patch for that.
>// *** These are up & coming for firefox 2.0
I guess that comment can be removed ;-)
We need to check chrome://mozapps/locale/extensions/extensions.properties if the stuff it sets is OK for us, it's references multiple times in this section.
Comment 11•19 years ago
|
||
Comment on attachment 219534 [details] [diff] [review]
Part 2 - make the modern theme compatible with the new themes manager.
>+INSTALL_EXTENSION_ID = $(XPI_NAME)@mozilla.org
I think I'd prefer to make clear that this is a theme in the ID. Probably $(XPI_NAME)@themes.mozilla.org would be a good idea here.
>+++ suite/themes/modern/install.rdf 2006-04-23 17:37:10.000000000 +0100
Thanks for adding those files in the new location already! :)
>+ <em:id>modern@mozilla.org</em:id>
>+ <em:version>1.0</em:version>
Same with the name here, of course.
I know the way you do it is consistent with current code, but in theory, it would be version 3.x of modern already ;-)
>+ <em:targetApplication>
>+ <Description>
>+ <em:id>{92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}</em:id>
Actually, I wonder in how many places (outside our code) this GUID is already used and if it might make sense to move to seamonkey@applications.mozilla.org
With the extension ID change, r=me.
Attachment #219534 -
Flags: review?(kairo) → review+
Assignee | ||
Comment 12•19 years ago
|
||
(In reply to comment #11)
> (From update of attachment 219534 [details] [diff] [review] [edit])
> >+ <em:targetApplication>
> >+ <Description>
> >+ <em:id>{92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}</em:id>
> Actually, I wonder in how many places (outside our code) this GUID is already
> used and if it might make sense to move to seamonkey@applications.mozilla.org
I'm guessing all the extensions for SeaMonkey and things like update.mozilla.org and addons.mozilla.org as a minimum.
Comment 13•19 years ago
|
||
Comment on attachment 219534 [details] [diff] [review]
Part 2 - make the modern theme compatible with the new themes manager.
>+DIST_FILES := \
>+ ../../suite/themes/modern/install.rdf \
>+ ../../suite/themes/modern/preview.png \
>+ ../../suite/themes/modern/icon.png \
>+ $(NULL)
We can't preprocess binary files. You'll have to manually write a libs:: rule for them.
Attachment #219534 -
Flags: superreview?(neil) → superreview-
Assignee | ||
Comment 14•19 years ago
|
||
Updated to address KaiRo's and Neil's comments. Carrying forward KaiRo's r+.
Attachment #219610 -
Flags: superreview?(neil)
Attachment #219610 -
Flags: review+
Assignee | ||
Comment 15•19 years ago
|
||
This is the preview image for the classic theme taken from the existing one and made into a png file. It will go into suite/themes/classic see also patch I'm just about to attach.
Assignee | ||
Comment 16•19 years ago
|
||
This is the icon for the classic theme taken from the reload button on the preview image. It will go into suite/themes/classic see also patch I'm just about to attach.
Assignee | ||
Comment 17•19 years ago
|
||
Similar to the modern patch, however, to fit into the existing tree structure, we can't easily rename the extension to classic@themes.mozilla.org, therefore I've kept the uuid that Thunderbird & Firefox use, though note the only directories we create with it are on the output.
Also requesting approval of the two image files I've just posted.
Also note we can't use DIST_FILES for the install.rdf as that requires FINAL_TARGET which would affect where the classic.jar would end up.
Attachment #219660 -
Flags: superreview?(neil)
Attachment #219660 -
Flags: review?(kairo)
Comment 18•19 years ago
|
||
Comment on attachment 219610 [details] [diff] [review]
Part 2 v2 - make the modern theme compatible with the new themes manager. (checked in excluding Makefile.in)
Bah, this xpi_name stuff really sucks as a way of building chrome. But while you're using it, it looks as if you're supposed to distribute stuff to $(FINAL_TARGET) which rules.mk will then copy to extensions/
Comment 19•19 years ago
|
||
Comment on attachment 219660 [details] [diff] [review]
Part 3 - make the classic theme compatible with the new themes manager
>+ $(PERL) $(MOZILLA_DIR)/config/preprocessor.pl $(DEFINES) $(ACDEFINES) $(topsrcdir)/suite/themes/classic/install.rdf.in > install.rdf
I don't really like this, I'd prefer to preprocess suite/themes/classic/install.rdf directly to the target (a bit like EXTRA_PP_COMPONENTS). However I do like the way that the .jar file does not move. Is there any way you can do this for Modern too?
Comment 20•19 years ago
|
||
Comment on attachment 219660 [details] [diff] [review]
Part 3 - make the classic theme compatible with the new themes manager
>Index: themes/classic/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/themes/classic/Makefile.in,v
>retrieving revision 1.23
>diff -u -p -r1.23 Makefile.in
>--- themes/classic/Makefile.in 10 Aug 2004 22:59:42 -0000 1.23
>+FILES := \
>+ install.rdf \
>+ $(topsrcdir)/suite/themes/classic/preview.png \
>+ $(topsrcdir)/suite/themes/classic/icon.png \
>+ $(NULL)
>+
>+libs::
>+ $(PERL) $(MOZILLA_DIR)/config/preprocessor.pl $(DEFINES) $(ACDEFINES) $(topsrcdir)/suite/themes/classic/install.rdf.in > install.rdf
>+ $(INSTALL) $(FILES) $(DIST)/bin/extensions/$(CLASSIC_EXTENSION_DIR)
Like Neil said, please make the preprocessor write the file directly to the target location instead of using this intermediate location.
>+* skin/classic/global/contents.rdf (global/mac/contents.rdf)
>+* skin/classic/global/contents.rdf (global/win/contents.rdf)
This doesn't work like you want, it unconditionally overwrites the mac version with the win version - on all platforms.
>+ skin/classic/global/preview.gif (global/win/preview.gif)
>+ skin/classic/global/preview.gif (global/mac/preview.gif)
same here, but the other way round.
>+ <em:type>4</em:type>
Hmm, can you point me where I can find waht "4" is supposed to mean here? I'm just curious ;-)
>+ <em:description>This theme simulates the familiar appearance of previous Netscape versions</em:description>
We really want to change the icons of this theme at some point and that will break your description. Please change it to something that tells that we are trying to use colors and styling from the system to fit in as good as we can with other applications on the system.
Additionally:
Please attach an icon and preview that really fits Classic - you mistakenly attached the Modern ones again.
r- because of the skin/classic/global/ problems
Attachment #219660 -
Flags: review?(kairo) → review-
Assignee | ||
Comment 21•19 years ago
|
||
(In reply to comment #19)
> (From update of attachment 219660 [details] [diff] [review] [edit])
> However I do like the way that the .jar file does not move.
> Is there any way you can do this for Modern too?
Unfortunately no. The way extension manager does it only allows for one default theme - for all other themes you have to construct it as per a proper theme structure - there is no redirect. As far as I can tell, having classic stay where it is is a bit of a hack anyway: http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#1652
(In reply to comment #20)
> (From update of attachment 219660 [details] [diff] [review] [edit])
> >+ <em:type>4</em:type>
> Hmm, can you point me where I can find waht "4" is supposed to mean here? I'm
> just curious ;-)
http://developer.mozilla.org/en/docs/install.rdf#type
Assignee | ||
Comment 22•19 years ago
|
||
The correct one this time.
Attachment #219658 -
Attachment is obsolete: true
Assignee | ||
Comment 23•19 years ago
|
||
the correct one this time.
Attachment #219657 -
Attachment is obsolete: true
Assignee | ||
Comment 24•19 years ago
|
||
Comment on attachment 219610 [details] [diff] [review]
Part 2 v2 - make the modern theme compatible with the new themes manager. (checked in excluding Makefile.in)
The next patch I add will include both the theme patches in the same patch.
Attachment #219610 -
Flags: superreview?(neil)
Assignee | ||
Updated•19 years ago
|
Attachment #219660 -
Flags: superreview?(neil)
Comment 25•19 years ago
|
||
As we're not really building an extension.
Assignee | ||
Comment 26•19 years ago
|
||
This patch adds the prefs we need to use extension manager/update properly (note updates are disabled for now). It also incorporates the changes for the recent Extension Manager change from Firefox.
Attachment #219588 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #220040 -
Flags: review?(kairo)
Comment 27•19 years ago
|
||
Comment on attachment 220040 [details] [diff] [review]
Prefs update and EM/TM -> Addons update
>+// Whether or not we show a dialog box informing the user that the update was
>+// successfully applied. This is off in Firefox by default since we show a
>+// upgrade start page instead! Other apps may wish to show this UI, and supply
>+// a whatsNewURL field in their brand.properties that contains a link to a page
>+// which tells users what's new in this new update.
>+pref("app.update.showInstalledUI", false);
The comment tells about what Firefox does, but what do we do (once we have turned updates on at all)?
>+pref("extensions.update.interval", 86400); // Check for updates to Extensions and
>+ // Themes every week
Nit: 86400 isn't a week, right?
>+# This is the fallback URL for release notes. Do not change this
>+# unless you are providing localized release notes!
>+app.update.url.details=http://www.mozilla.org/products/seamonkey/releases/
"More information about this update" in the "An update is available" page of the update wizard is not necessarily release notes. Just tell in the comment what this really is (i.e. first part of my sentence before, shortenend from the blurb you're adding to prefs).
>- function toEM(aType)
>+ function toAM()
Nah, just leave toEM() as the function name, it's still called "extension manager" or "EM" everywhere internally.
r=me with those nits addressed.
Attachment #220040 -
Flags: review?(kairo) → review+
Assignee | ||
Comment 28•19 years ago
|
||
Addressed KaiRo's comments. Carrying forward r.
Attachment #220040 -
Attachment is obsolete: true
Attachment #220059 -
Flags: superreview?(neil)
Attachment #220059 -
Flags: review+
Assignee | ||
Comment 29•19 years ago
|
||
Revised patch for classic addressing previously received comments. Also included a comment removal for something I had previously thought was necessary, but no longer.
Note: the images attached for classic also apply as part of this patch.
Attachment #219534 -
Attachment is obsolete: true
Attachment #219660 -
Attachment is obsolete: true
Attachment #220060 -
Flags: review?(kairo)
Comment 30•19 years ago
|
||
Comment on attachment 220060 [details] [diff] [review]
Part 3 v2 - make the classic theme compatible with the new themes manager (checked in)
>+FILES = \
>+ $(topsrcdir)/suite/themes/classic/preview.png \
>+ $(topsrcdir)/suite/themes/classic/icon.png \
>+ $(NULL)
We have separate Mac previews, no? So, I guess this should be ifdef, although I don't know where the Mac files would go.
>+ $(SYSINSTALL) $(IFLAGS1) $(FILES) $(DESTDIR)$(mozappdir)/extensions/$(CLASSIC_EXTENSION_DIR)
>+ $(PERL) $(MOZILLA_DIR)/config/preprocessor.pl $(DEFINES) $(ACDEFINES) $(topsrcdir)/suite/themes/classic/install.rdf > $(DIST)/bin/extensions/$(CLASSIC_EXTENSION_DIR)/install.rdf
Surely this should be > $(DESTDIR)$(mozappdir)/extensions/$(CLASSIC_EXTENSION_DIR)/install.rdf
>+* skin/classic/editor/contents.rdf (editor/contents.rdf)
>+* skin/classic/messenger/contents.rdf (messenger/contents.rdf)
>+* skin/classic/navigator/contents.rdf (navigator/contents.rdf)
>+#ifdef XP_MACOSX
>+* skin/classic/global/contents.rdf (global/mac/contents.rdf)
>+ skin/classic/global/preview.gif (global/mac/preview.gif)
>+#else
>+* skin/classic/global/contents.rdf (global/win/contents.rdf)
>+ skin/classic/global/preview.gif (global/win/preview.gif)
>+#endif
>+#endif
It would be nice if all the (s were aligned.
>+ </Description>
I happened to spot some trailing whitespace here, please recheck your patch.
Updated•19 years ago
|
Attachment #219817 -
Flags: superreview?(jag)
Attachment #219817 -
Flags: review?(kairo)
Comment 31•19 years ago
|
||
Comment on attachment 220059 [details] [diff] [review]
Prefs update and EM/TM -> Addons update v2 (checked in)
> // defaultChromeURI is only needed until our default command line handler knows what to launch
> pref("toolkit.defaultChromeURI","chrome://navigator/content/navigator.xul");
Since this pref is hopefully going away real soon now (I guess I ought to ask for reviews!) I'd prefer you added your new xpinstall prefs near the old xpinstall prefs so that the old xpinstalled prefs only get ifdefed.
Comment 32•19 years ago
|
||
Comment on attachment 220060 [details] [diff] [review]
Part 3 v2 - make the classic theme compatible with the new themes manager (checked in)
Looks good to me now. r=me
Attachment #220060 -
Flags: review?(kairo) → review+
Comment 33•19 years ago
|
||
Comment on attachment 219817 [details] [diff] [review]
Bypass xpi-stage (for themes/modern/Makefile.in - checked in)
This version looks even better then Mark's original one ;-) r=me
Attachment #219817 -
Flags: review?(kairo) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #220060 -
Flags: superreview?(neil)
Assignee | ||
Comment 34•19 years ago
|
||
(In reply to comment #30)
> (From update of attachment 220060 [details] [diff] [review] [edit])
Oh didn't realise you'd already looked at this ;-)
> >+FILES = \
> >+ $(topsrcdir)/suite/themes/classic/preview.png \
> >+ $(topsrcdir)/suite/themes/classic/icon.png \
> >+ $(NULL)
> We have separate Mac previews, no? So, I guess this should be ifdef, although I
> don't know where the Mac files would go.
KaiRo and I have basically agreed there is no point keeping the Mac previews - they are not that different. Its the difference between http://lxr.mozilla.org/seamonkey/source/themes/classic/global/win/preview.gif and http://lxr.mozilla.org/seamonkey/source/themes/classic/global/mac/preview.gif
Assignee | ||
Comment 35•19 years ago
|
||
Comment on attachment 219610 [details] [diff] [review]
Part 2 v2 - make the modern theme compatible with the new themes manager. (checked in excluding Makefile.in)
We're going to need the non Makefile.in parts of this patch to go with the bypass xpi-stage patch. KaiRo already gave it r, so I'm requesting sr for the non Makefile.in parts.
Attachment #219610 -
Flags: superreview?(neil)
Comment 36•19 years ago
|
||
Comment on attachment 219817 [details] [diff] [review]
Bypass xpi-stage (for themes/modern/Makefile.in - checked in)
config.mk also pulls in autoconf.mk, so remove |include $(DEPTH)/config/autoconf.mk| and sr=jag
Attachment #219817 -
Flags: superreview?(jag) → superreview+
Comment 37•19 years ago
|
||
Comment on attachment 220060 [details] [diff] [review]
Part 3 v2 - make the classic theme compatible with the new themes manager (checked in)
sr=me with the whitespace nits fixed.
Attachment #220060 -
Flags: superreview?(neil) → superreview+
Comment 38•19 years ago
|
||
Comment on attachment 220059 [details] [diff] [review]
Prefs update and EM/TM -> Addons update v2 (checked in)
sr=me with the (ifdefed) prefs moved back to line 194.
Attachment #220059 -
Flags: superreview?(neil) → superreview+
Comment 39•19 years ago
|
||
Comment on attachment 219610 [details] [diff] [review]
Part 2 v2 - make the modern theme compatible with the new themes manager. (checked in excluding Makefile.in)
sr=me with install.rdf's trailing whitespace removed. I assume you'll check in my patch at the same time?
Attachment #219610 -
Flags: superreview?(neil) → superreview+
Comment 40•19 years ago
|
||
Comment on attachment 220059 [details] [diff] [review]
Prefs update and EM/TM -> Addons update v2 (checked in)
>+<!ENTITY addonsManagerCmd.label "Addons">
>+<!ENTITY addonsManagerCmd.accesskey "A">
Sorry for the late notice, but can we call this "Add-ons Manager"?
Comment 41•19 years ago
|
||
(In reply to comment #40)
> Sorry for the late notice, but can we call this "Add-ons Manager"?
"Manager" sounds good, but according to the patches in bug 329045 it's called "Addons" (no hyphen) everywhere else, introducing the hyphen would make us inconsistent.
Assignee | ||
Updated•19 years ago
|
Attachment #219536 -
Attachment description: preview image for modern theme → preview image for modern theme (checked in)
Assignee | ||
Updated•19 years ago
|
Attachment #219537 -
Attachment description: icon for modern theme → icon for modern theme (checked in)
Assignee | ||
Updated•19 years ago
|
Attachment #219763 -
Attachment description: icon for classic theme → icon for classic theme (checked in)
Assignee | ||
Updated•19 years ago
|
Attachment #219764 -
Attachment description: preview image for the classic theme → preview image for the classic theme (checked in)
Assignee | ||
Updated•19 years ago
|
Attachment #219817 -
Attachment description: Bypass xpi-stage → Bypass xpi-stage (for themes/modern/Makefile.in - checked in)
Assignee | ||
Updated•19 years ago
|
Attachment #219610 -
Attachment description: Part 2 v2 - make the modern theme compatible with the new themes manager. → Part 2 v2 - make the modern theme compatible with the new themes manager. (checked in excluding Makefile.in)
Assignee | ||
Updated•19 years ago
|
Attachment #220060 -
Attachment description: Part 3 v2 - make the classic theme compatible with the new themes manager → Part 3 v2 - make the classic theme compatible with the new themes manager (checked in)
Assignee | ||
Comment 42•19 years ago
|
||
(In reply to comment #36)
> (From update of attachment 219817 [details] [diff] [review] [edit])
> config.mk also pulls in autoconf.mk, so remove |include
> $(DEPTH)/config/autoconf.mk| and sr=jag
>
We can't remove autoconf.mk because it will actually break the build:
NeilAway: you must include autoconf.mk first to get MOZ_XUL_APP
...
NeilAway: we can't set USE_EXTENSION_MANIFEST in an XPFE build
NeilAway: so we need to set it after autoconf.mk to get MOZ_XUL_APP but before config.mk so that it actually does something
NeilAway: then after including config.mk we can reset FINAL_TARGET
Assignee | ||
Updated•19 years ago
|
Attachment #220059 -
Attachment description: Prefs update and EM/TM -> Addons update v2 → Prefs update and EM/TM -> Addons update v2 (checked in)
Assignee | ||
Comment 43•19 years ago
|
||
(In reply to comment #42)
> (In reply to comment #36)
> > (From update of attachment 219817 [details] [diff] [review] [edit] [edit])
> > config.mk also pulls in autoconf.mk, so remove |include
> > $(DEPTH)/config/autoconf.mk| and sr=jag
> >
>
> We can't remove autoconf.mk because it will actually break the build:
>
> NeilAway: you must include autoconf.mk first to get MOZ_XUL_APP
> ...
> NeilAway: we can't set USE_EXTENSION_MANIFEST in an XPFE build
> NeilAway: so we need to set it after autoconf.mk to get MOZ_XUL_APP but before
> config.mk so that it actually does something
> NeilAway: then after including config.mk we can reset FINAL_TARGET
>
I've checked in the original version of this patch as a fixup, but I've also added a comment to remove "include $(DEPTH)/config/autoconf.mk" when we throw the MOZ_XUL_APP switch as we won't need it then.
Assignee | ||
Updated•19 years ago
|
Whiteboard: Add-on Manager should work. Closure waiting on removal of existing controls patch.
Assignee | ||
Comment 44•19 years ago
|
||
Follow-up patch, installed-chrome.txt shouldn't be needed by suiterunner. I've been using this patch for quite a while without problems - just forgot to submit it earlier ;-)
Attachment #220313 -
Flags: superreview?(neil)
Attachment #220313 -
Flags: review?(neil)
Assignee | ||
Comment 45•19 years ago
|
||
Comment on attachment 220313 [details] [diff] [review]
installed-chrome.txt isn't needed by suiterunner (checked in).
KaiRo convinced me that we should just get rid of this after the switch to save adding more ifdefs. I'd missed adding the xxx comment anyway.
Attachment #220313 -
Flags: superreview?(neil)
Attachment #220313 -
Flags: review?(neil)
Comment 46•19 years ago
|
||
Comment on attachment 220313 [details] [diff] [review]
installed-chrome.txt isn't needed by suiterunner (checked in).
This sucked anyway as I now have a zillion select lines in my installed-chrome.txt!
Comment 47•19 years ago
|
||
Unfortunately this causs problem with some build configurations, it seems :(
My builds now stop with the following error:
/mnt/mozilla/build/seamonkey-dbg/config/nsinstall -R /mnt/mozilla/src/mozilla/suite/themes/modern/preview.png /mnt/mozilla/src/mozilla/suite/themes/modern/icon.png ../../dist/bin/extensions/modern@themes.mozilla.org
+++ making chrome /mnt/mozilla/build/seamonkey-dbg/themes/modern => ../../dist/bin/extensions/modern@themes.mozilla.org/chrome/modern.jar
copy(/mnt/mozilla/src/mozilla/themes/modern/communicator/brand.css, ../../dist/bin/extensions/modern@themes.mozilla.org/chrome/modern/skin/modern/communicator/brand.css) failed: No such file or directory at /mnt/mozilla/src/mozilla/config/make-jars.pl line 503, <STDIN> line 475.
make: *** [libs] Error 2
Obviously, this happens in the /mnt/mozilla/build/seamonkey-dbg/themes/modern dir on my machine (Linux), I think the problem is me building with --enable-chrome-format=both and the flat version of the chrome somehow can't be written correctly...
Comment 48•19 years ago
|
||
It's definately the chrome-format option. I tested with manually setting MOZ_CHROME_FILE_FORMAT in autoconf.mk to jar, flat and symlink (as both is just jar+flat), and out of those three only jar works, the two others fail. I guess the @ in the dir name might have something to do with it...
Assignee | ||
Comment 49•19 years ago
|
||
(In reply to comment #48)
> It's definately the chrome-format option. I tested with manually setting
> MOZ_CHROME_FILE_FORMAT in autoconf.mk to jar, flat and symlink (as both is just
> jar+flat), and out of those three only jar works, the two others fail. I guess
> the @ in the dir name might have something to do with it...
>
I'd be surprised if its the @ - take DOMI for example, I'm fairly sure that gets built before themes (with suiterunner) and that uses inspector@mozilla.org as the directory name.
Comment 50•18 years ago
|
||
Is theme manager set up in 2006101906 for Mac? 10.4.9. If I switch to the classic theme in Preferences and restart as instructed, I do not get any icon changes. Preferences shows the Classic theme as selected, but the Modern theme is displayed.
I've seen several bugs about building the Modern theme, but this looks closest to where I might share concern for theme switching not working, short of writing a new bug. (Which I'll do if that's a better solution.)
Assignee | ||
Comment 51•18 years ago
|
||
(In reply to comment #50)
> Is theme manager set up in 2006101906 for Mac? 10.4.9.
No, the theme manager (actually now called Add-on Manager) won't be enabled until bug 328887 depends - when we throw the switch on trunk, unless you're using one of the specific suiterunner builds.
There are several known theme bugs in the suiterunner builds other bugs with non-suiterunner builds should be filed in other bugs if there are no duplicates.
Summary: Get SeaMonkey's themes registering and switching with Theme Manager → Get SeaMonkey's themes registering and switching with Theme Manager (in suiterunner)
Comment 52•18 years ago
|
||
Comment on attachment 219520 [details] [diff] [review]
Part 1 - Remove existing theme controls from suiterunner.
>+#ifndef MOZ_XUL_APP
>+// XXX Remove this section when Suite becomes a full XUL app
> function applyTheme(themeName)
...
>+#ifndef MOZ_XUL_APP
>+<!-- XXX Remove this section when Suite becomes a full XUL app -->
> <menuseparator />
> <menu label="&applyTheme.label;" accesskey="&applyTheme.accesskey;">
> <menupopup id="theme" datasources="rdf:chrome"
> ref="urn:mozilla:skin:root"
> oncommand="applyTheme(event.target)"
> onpopupshowing="checkTheme()"
While keeping the pref panel makes no sense with themes being part of the add-on manager, I think we should try to keep the "View->Apply Theme" menu (if possible at all).
Assignee | ||
Comment 53•18 years ago
|
||
As discussed on the previous comment and on irc, this patch just removes the old theme preferences pane and adds an option to the debug pane for allowing dynamic switching of themes.
I think the View -> Apply Theme menu options should be covered by a difference bug.
Attachment #219520 -
Attachment is obsolete: true
Attachment #252158 -
Flags: superreview?(neil)
Attachment #252158 -
Flags: review?(neil)
Attachment #219520 -
Flags: superreview?(neil)
Attachment #219520 -
Flags: review?(neil)
Assignee | ||
Comment 54•18 years ago
|
||
Comment on attachment 220313 [details] [diff] [review]
installed-chrome.txt isn't needed by suiterunner (checked in).
Previously KaiRo & I said we'd leave this, however we have now decided it would be a good thing to do as it'll make it clearer that its not necessary (we now use the general.skins.selectedSkin pref), and help towards getting rid of installed-chrome.txt for suiterunner.
Attachment #220313 -
Flags: superreview?(neil)
Attachment #220313 -
Flags: review?(neil)
Comment 55•18 years ago
|
||
Comment on attachment 220313 [details] [diff] [review]
installed-chrome.txt isn't needed by suiterunner (checked in).
I never did like these lines, because they accumulate in depend builds. (I had prefixed grep -q skin,install,select <path to>installed-chrome.txt || to them in one of my builds to cut down on the wastage!)
> # select classic as the default skin
>+ifndef MOZ_XUL_APP
> libs::
> echo skin,install,select,classic/1.0 >> $(DIST)/bin/chrome/installed-chrome.txt
>
> install::
> echo skin,install,select,classic/1.0 >> $(DESTDIR)$(mozappdir)/chrome/installed-chrome.txt
>+endif
Nit: might as well ifdef out the comment too :-P
Attachment #220313 -
Flags: superreview?(neil)
Attachment #220313 -
Flags: superreview+
Attachment #220313 -
Flags: review?(neil)
Attachment #220313 -
Flags: review+
Assignee | ||
Updated•18 years ago
|
Attachment #220313 -
Attachment description: installed-chrome.txt isn't needed by suiterunner. → installed-chrome.txt isn't needed by suiterunner (checked in).
Assignee | ||
Comment 56•18 years ago
|
||
This patch does just the adding of the dynamic skin switching option to the debug page (as Neil mentioned to me a while ago on irc).
The removal of the theme preferences pane in suiterunner will be done at/after the switchover on trunk (no schedule for that yet), so I'll raise a separate bug for that which will allow this one to be closed down once this patch goes in.
Attachment #252158 -
Attachment is obsolete: true
Attachment #257163 -
Flags: superreview?(neil)
Attachment #257163 -
Flags: review?(neil)
Attachment #252158 -
Flags: superreview?(neil)
Attachment #252158 -
Flags: review?(neil)
Comment 57•18 years ago
|
||
Comment on attachment 257163 [details] [diff] [review]
Add dynamic skin switching option to debug page (checked in)
>+<!ENTITY dynamicSkinSwitching.label "Enable Dynamic Skin Switching (Add-on Manager enabled versions only.">
Nit: missing ). Or rewrite the text as "Allow Add-on Manager to dynamically switch skins". Or I could patch xpfe theme switcher to watch the pref ;-)
Attachment #257163 -
Flags: superreview?(neil)
Attachment #257163 -
Flags: superreview+
Attachment #257163 -
Flags: review?(neil)
Attachment #257163 -
Flags: review+
Comment 58•18 years ago
|
||
Assignee | ||
Updated•18 years ago
|
Attachment #257163 -
Attachment description: Add dynamic skin switching option to debug page → Add dynamic skin switching option to debug page (checked in)
Assignee | ||
Comment 59•18 years ago
|
||
Comment on attachment 257243 [details] [diff] [review]
Adds dss to xpfe (checked in)
Looks ok to me, r=me.
Attachment #257243 -
Flags: review+
Assignee | ||
Comment 60•18 years ago
|
||
Comment on attachment 257243 [details] [diff] [review]
Adds dss to xpfe (checked in)
Neil just checked this in.
Attachment #257243 -
Attachment description: Adds dss to xpfe → Adds dss to xpfe (checked in)
Assignee | ||
Comment 61•18 years ago
|
||
This is now fixed. The themes pane will be removed from preferences by bug 372856.
Blocks: 372856
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: Add-on Manager should work. Closure waiting on removal of existing controls patch.
You need to log in
before you can comment on or make changes to this bug.
Description
•