All of the app-specific #ifdefs need to die for help to be embeddable in XRE/xulrunner. They're only used to call Firefox Help on Firefox and Thunderbird Help on Thunderbird. The content pack url is currently hardcoded in contextHelp.js to chrome://help/locale/firebirdhelp.rdf or chrome://help/locale/thunderbirdhelp.rdf. I suggest to use a pref instead.
(In reply to comment #0) > The content pack url is currently hardcoded in contextHelp.js to > chrome://help/locale/firebirdhelp.rdf or > chrome://help/locale/thunderbirdhelp.rdf. I suggest to use a pref instead. Having a pref for the name really seems to me to be useless bloat. Just make the name something generic and stick with it for everything. I'm working primarily on some of the docs bugs now, but this is something I'd really like to do as a learning experience about the back end of Help (and probably about the build process as well). If you really want to do it, Steffen, do so, but if you can wait on this until I've tried or said I can't do it, I'd appreciate it.
(In reply to comment #1) > Just make the name something generic and stick with it for everything. Sounds even better. > I'm working primarily on some of the docs bugs now, but this is something I'd > really like to do as a learning experience about the back end of Help (and > probably about the build process as well). If you really want to do it, > Steffen, do so, but if you can wait on this until I've tried or said I can't > do it, I'd appreciate it. Sure, go ahead! I'm working on the context sensitive Help (bug 260054) right now. I'm on #documentation btw.
I don't think you understand how help content packs work. firebirdhelp.rdf IS the content pack. You can't have the content pack URL hard-coded into the content pack :). By default, you're supposed to use the contextHelp.js functions setURI(string) to set the content pack loaded into help. The default is set to firebirdhelp.rdf, but can be changed to another RDF file with appropriate settings. You should not need to modify the content pack unless you're making a API change.
In contextHelp.js, there's this code: #ifndef MOZ_THUNDERBIRD const MOZILLA_CONTENT_PACK = "chrome://help/locale/firebirdhelp.rdf"; #else const MOZILLA_CONTENT_PACK = "chrome://help/locale/thunderbirdhelp.rdf"; #endif In helpMenuOverlay.xul, there's this: #ifdef MOZ_THUNDERBIRD oncommand="openHelp('smtp-docs', 'chrome://help/locale/thunderbirdhelp.rdf');"/> #else oncommand="openHelp('firefox-help');"/> #endif We want to get rid of these because the toolkit, in which these files live, is supposed to become embeddable in xulrunner. Imagine a dozen more applications using the Help Viewer. We don't want to add a dozen ifdefs. So what do you suggest? - Get rid of helpMenuOverlay.xul and move it to browser-menubar.inc and browser-sets.inc (in Firefox). Use openHelp('firefox-help') there. Help would become a mandatory extension. - Use a app-specific pref which holds the content pack url (chrome://help/locale/firebirdhelp.rdf or chrome://help/locale/thunderbirdhelp.rdf). Query that pref in contextHelp.js. - Make the content pack url generic, being the same for every app. Hardcode that, but leave the option to change it via setHelpFileURI().
(In reply to comment #4) > In contextHelp.js, there's this code: > #ifndef MOZ_THUNDERBIRD > const MOZILLA_CONTENT_PACK = "chrome://help/locale/firebirdhelp.rdf"; > #else > const MOZILLA_CONTENT_PACK = "chrome://help/locale/thunderbirdhelp.rdf"; > #endif > > In helpMenuOverlay.xul, there's this: > #ifdef MOZ_THUNDERBIRD > oncommand="openHelp('smtp-docs', 'chrome://help/locale/thunderbirdhelp.rdf');"/> > #else > oncommand="openHelp('firefox-help');"/> > #endif > > We want to get rid of these because the toolkit, in which these files live, is > supposed to become embeddable in xulrunner. Imagine a dozen more applications > using the Help Viewer. We don't want to add a dozen ifdefs. I don't think you understand. MOZILLA_CONTENT_PACK is the default. You can easily change that value. Really Thunderbird Help shouldn't be using the #ifdef method, but instead should use setHelpFileURI(). > > So what do you suggest? > - Get rid of helpMenuOverlay.xul and move it to browser-menubar.inc and > browser-sets.inc (in Firefox). Use openHelp('firefox-help') there. Help would > become a mandatory extension. I never said that and wouldn't recommend it. You seem very confused about how the API works. Read the Help Content Spec -> http://www.mozilla.org/projects/help-viewer/content_packs.php Also here's the code to change the content pack to something else -> http://lxr.mozilla.org/mozilla/source/toolkit/components/help/content/contextHelp.js#76 Also note that you can take a content pack URI as a parameter to openHelp(), just a little extra feature I added awhile ago -> http://lxr.mozilla.org/mozilla/source/toolkit/components/help/content/contextHelp.js#50 The default content pack value should NEVER be modified. Why Thunderbird Help modifies it via #ifdef is an error.
WORKSFORME. This is not a issue with the API and is a default function. Whatever the default content pack is doesn't matter. People using the API shouldn't be toggling the default value, just changing that value to their app at runtime or using showHelp()'s URI parameter.
anything that's using #ifdefs in toolkit needs to die for xulrunner. If the API is specced that way, then the API spec needs to change. End of story. Using a pref is not bloat, we do the same for the browser chrome URL. Its the best solution for this, IMO.
(In reply to comment #7) > anything that's using #ifdefs in toolkit needs to die for xulrunner. If the API > is specced that way, then the API spec needs to change. End of story. > > Using a pref is not bloat, we do the same for the browser chrome URL. Its the > best solution for this, IMO. Did you read what I just said? Obviuosly not. having a pref would be USELESS. Let Jeff decide. You are not a help peer so therefore you cannot make this decision. If after READING my comments Jeff wants the bug reopened, then so be it. Now don't think I disagree with removing the ifdef. Thunderbird help shouldn't be modifying the default. Its sloppy coding and isn't what the API was designed for. That should be fixed, but that isn't really what this bug is about. I recommend filing a new one, but if a help peer disagrees, then reopen.
<Wolf> the code lives in toolkit, and appears to have a problem if you *need* to modify the source to embed it, IMO <rj_keller> Wolf: why would you need to modify it? <rj_keller> Wolf: you're supposed to use setHelpFileURI() to set the content pack, not change hte default. <rj_keller> Actually, just the fact that we have a default content pack is a bug.
I don't agree we need a new bug. This bug is about removing app-specific ifdefs from code in mozilla/toolkit/components/help, because toolkit should be somewhat independant from Firefox and Thunderbird. > <rj_keller> Actually, just the fact that we have a default content pack is a bug. So let's remove it. But where else does contextHelp.js get the default pack from? From a pref. Have a look at the pref browser.chromeURL. It stores the main entry point for Firefox. It says: chrome://browser/content/ Why not introduce a pref help.defaultContentPack, set to chrome://help/locale/firebirdhelp.rdf in firefox.js and to chrome://help/locale/thunderbirdhelp.rdf in thunderbird.js? That would be the default pack. Extensions wouldn't change it, but still use setHelpFileURI() to provide their own content.
(In reply to comment #10) > I don't agree we need a new bug. This bug is about removing app-specific ifdefs > from code in mozilla/toolkit/components/help, because toolkit should be somewhat > independant from Firefox and Thunderbird. > > > <rj_keller> Actually, just the fact that we have a default content pack is a bug. > So let's remove it. But where else does contextHelp.js get the default pack > from? From a pref. Have a look at the pref browser.chromeURL. It stores the main > entry point for Firefox. It says: chrome://browser/content/ > > Why not introduce a pref help.defaultContentPack, set to > chrome://help/locale/firebirdhelp.rdf in firefox.js and to > chrome://help/locale/thunderbirdhelp.rdf in thunderbird.js? That would be the > default pack. Extensions wouldn't change it, but still use setHelpFileURI() to > provide their own content. From the way I made the API, the default shouldn't really be used. In a perfect world it should be null. You can do whatever you want, its out of my hands now. don't really care enough to say anything other than what is supposed to happen in the way I designed the API. Just telling you that I didn't design the API to work that way. Function calls are much easier for apps using multiple content packs than prefs. If you want to rework the API, then at least update content_help on the help viewer site so others know. But really I do not care. I have bigger fish to fry than play around with Mozilla bugs.
I have a serious problem with loading a global persistent variable from delayedStartup(), there simply isn't a reason for this. if you need the path, set it when you invoke help, not before.
(In reply to comment #13) > I have a serious problem with loading a global persistent variable from > delayedStartup(), there simply isn't a reason for this. > > if you need the path, set it when you invoke help, not before. thanks for your opinion. Obviously the bug reporter would understand the bug better than anyone else and since hes a peer he gets the final say. IMO (not that it means much), there simply isn't a reason for a pref. Why would you want to constantly swap between prefs on a certain app as opposed to using a simple function call? If you have trouble understanding, ping me on IRC. Jeff, you need to get on IRC more often :). Most stuff like this gets discussed there anyways. Got a tree available for some patch testing? I can make a patch if you want but don't have a working tree (or working system for that matter :)). Might be slow though. I don't have a whole lot of time.
RJ, with all due respect for the work you guys do on Help, you don't have authority over browser.js. I think that having a persisted global var created at startup for each window is a lot more bloaty than having a default chrome URL as a pref. We don't hardcode the base URL for the entire app, why would we do it for Help content? Added bonus to using the pref, embeddors/corporates can create a sub/superset of contents and push that to their users in place of the defaults.
(In reply to comment #15) > RJ, with all due respect for the work you guys do on Help, you don't have > authority over browser.js. and neither do you.
It seems to me that the simplest solution might be to use the same name for the content pack RDF file for all apps, rather than put the app name into the filename. Also, it looks like you're packaging your locale files into help.jar, not en-US.jar, which makes it more difficult to localize, and difficult to update the language pack a browser is running with or the help engine independently of each other - ideally help.jar would contain just the UI/engine and thus the jar file would be the same for *all* languages.
I agree with Ben. That's the third suggestion I made in comment 4. Let's go for that. I filed bug 260417 about packaging Help content in en-us.jar.
If this isn't done, Help won't coexist well in Thunderbird and Firefox together. Thunderbird's coming from the branch, so this needs to be fixed in aviary. As such, I'm retargeting it for 1.0. As long as docs issues get fixed (they're coming together, and I'm pretty much doing what I can there already), there's plenty of time to fix this before 1.0. See end of comment for a few questions I need answered... (In reply to comment #17) > It seems to me that the simplest solution might be to use the same name for > the content pack RDF file for all apps, rather than put the app name into the > filename. i.e., chrome://help/locale/defaultContentPack.rdf? That's what I'm doing in the work-in-progress patch I'm making (via jar.mn for now, but perhaps renaming it on the CVS server side is a better idea). The patch will also make "openHelp();" a valid call by setting a default topic (marked by ID="default-topic" in the table of contents RDF file). The way I've implemented it, the code shouldn't interfere with the old API. Anyways, an overview of what's happening (partly as an FYI, partly so I can remember what I've done when I get back to working on the patch later): Issues to fix: -contents.rdf #ifdefs -ifdefs used to get overlays working -can't count on a specific name for files to overlay -SOLUTION: move contents.rdf into app-specific Help -helpMenuOverlay.xul -ifdef used to overlay F1/Help key access to Help -problematic: keysets that are overlaid have different @id values -SOLUTION: file a bug for Thunderbird, have mscott change the name (eurgh, probably not super-trivial)? create default keyset for all next-gen apps? ideas, anyone? -ifdef used to sort out different openHelp() arguments -this is also kinda bad -SOLUTION: set up openHelp() to accept 0 arguments and load a default topic (as outlined above) -default topic requires changing ID="default-topic" in help topic RDF -make help RDF default be what's used by app Help - not difficult to do in a backwards-compatible way, I think -openHelp used twice, might consider a <command/> - but orthogonal to this bug -ifdef used to display "For IE Users" in Fx -bad idea, w/o ifdef it'd show in Thunderbird -SOLUTION: add overlay into browser/components/help for the extra entry -would work well with the contents.rdf move mentioned above -toolkit/components/help/Makefile.in -ifdef used to eventually let apps w/o browser toolbar (Tb) coexist with those that do (Fx) -can't just remove because images needed by Help -SOLUTION: give Help a default skin, but use + in jar.mn to override it with an app-specific skin in browser/components/help (build process allows this) -don't quite know yet how that would work with the toolbar image - how to override *its existence* and replace with nothing? -actually a current problem - e.g., chrome://help/skin/Toolbar.png is in builds but is unused - *bloat/cruft* -contextHelp.js -default topics for Fx/Tb are hardcoded into file -SOLUTION: set default topic name, make Fx/Tb obey that name -also changes to jar.mn in browser/components/help, change in ToC RDF -extend Help API so openHelp() is callable w/o args to get default topic -a duplicate of some helpMenuOverlay.xul changes mentioned above -toolkit/themes/qute/help/help.css -(would be) used by TB Help (?) - could check but don't care to right now I think that's it, and I believe I've covered the app ifdefs in toolkit Help. Anyways, the questions that need answers: -Can I diff files in a new directory without creating the directory on cvs.m.o? -Should I file a bug for mscott to change the name of the main keyset in TB? Or would another way be better? Answers appreciated, preliminary patch sometime soon, likely after I take care of a few things that require my time this week...
> -helpMenuOverlay.xul > -ifdef used to overlay F1/Help key access to Help > -problematic: keysets that are overlaid have different @id values > -SOLUTION: file a bug for Thunderbird, have mscott change the name (eurgh, > probably not super-trivial)? create default keyset for all next-gen apps? Either that, or let each app handle its Help menu itself. That would solve the "for IE users" menuitem problem as well. Move helpMenuOverlay.xul into browser and mail. > -toolkit/components/help/Makefile.in There's something pretty wrong in that makefile: # Use Qute on non-Mac non-Phoenix apps Oops, there's no qute folder in toolkit/components/help/skins anymore. Skin now lives over there: http://lxr.mozilla.org/aviarybranch/source/toolkit/themes/pinstripe/help/ http://lxr.mozilla.org/aviarybranch/source/toolkit/themes/qute/help/ http://lxr.mozilla.org/aviarybranch/source/toolkit/themes/winstripe/help/ The ifdef between the themes is done here: http://lxr.mozilla.org/aviarybranch/source/toolkit/themes/Makefile.in That's somebody else's problem. But we need to remove the bogus ifdef in toolkit/components/help/Makefile.in and remove the skins subdirectory. > -ifdef used to eventually let apps w/o browser toolbar (Tb) coexist with > those that do (Fx) We're indeed referencing chrome://browser/skin/Toolbar.png in toolkit/themes/pinstripe/help/help.css toolkit/themes/qute/help/help.css (#ifndef MOZ_PHOENIX) toolkit/themes/winstripe/help/help.css > -can't just remove because images needed by Help Indeed, for the back/forwards/home/print buttons. > -SOLUTION: give Help a default skin, but use + in jar.mn to override it > with an app-specific skin in browser/components/help (build process > allows this) I think using browser buttons is a hack. Ask Kevin to update these files: http://lxr.mozilla.org/aviarybranch/source/toolkit/themes/winstripe/help/Toolbar.png http://lxr.mozilla.org/aviarybranch/source/toolkit/themes/pinstripe/help/Toolbar.png and back out bug 252974. The qute version has another problem: the zoom and toggle sidebar buttons are in a separate file zoomImg.png. > -actually a current problem - e.g., chrome://help/skin/Toolbar.png is > in builds but is unused - *bloat/cruft* Sorry, that's not true. We're using it for the help-sidebar-button, help-find-button, help-zoom-small-button, help-zoom-large-buttonm help-toolbar-customization. See help.xul: it has a class="toolbarbutton-1" > -toolkit/themes/qute/help/help.css > -(would be) used by TB Help (?) - could check but don't care to right now right. > I think that's it, and I believe I've covered the app ifdefs in toolkit Help. > Anyways, the questions that need answers: > > -Can I diff files in a new directory without creating the directory on cvs.m.o? No. But that's not a problem. Just cvs add the directory and the files in it. It's not visible to others until you cvs commit that. Use cvs diff -uN (N for new files). Thanks a lot for taking this. Writing this comment was time-consuming enough. :)
I think I'm missing something here. Last I heard, Thunderbird 1.0 wasn't going to ship with a Help Viewer, so based on that I see zero justification for taking/making these changes on the branch. This isn't fixing a visible bug that will affect Firefox or Thunderbird, so why are we going to make changes? If you want to fix it sooner, great, but it should stay on trunk unless there's a very compelling reason to make these changes on branch. Nothing I've read gives me that so far, but I'm open to reasoning as to why we need to take a calculated risk. (Keep in mind that EVERY patch going onto aviary has to meet an appropriate risk/reward factor.)
(In reply to comment #21) > I think I'm missing something here. Last I heard, Thunderbird 1.0 wasn't > going to ship with a Help Viewer, so based on that I see zero justification > for taking/making these changes on the branch. Actually, I think *I'm* the one missing something here. Some sort of built-in Help is an essential part of *any* software (even moreso in the case of something as complex as an email client -- does Gmail with its interface not have Help?). I *really* wish I'd gotten in the loop earlier, because this is the first mention I've heard of this decision (if indeed it's a set-in-stone, already-made decision that can't change). That said, I don't see a reason that I couldn't sync Help content on the branch with Help content on the trunk, create a patch for trunk, let the changes bake for a while, and then backport them to aviary (either before or after Firefox 1.0, probably wouldn't matter much unless you absolutely refuse changes that might somehow conceivably introduce instability). This way, Help in Thunderbird 1.0 is still a possibility. I guess for now I'll do the work on trunk, but not having Help in Thunderbird for a "1.0" release strikes me as a very, very bad decision.
Is there actually complete and ready documentation for Thunderbird Help? Its really really late in the game to add this, especially since we're at the l10n freeze, and there's currently nothing. If freezing now is needed for localizing Firefox itself, creating complete Help content for Thunderbird is probably out of the question for other localizations. If its ready _now_ we could possibly try to make a case to the localizers, but we'd probably have to let it slide. If its a week from now, there is basically zero chance. People are already complaining about localizing 900 strings for the Tbird UI, forcing Help content on them is just not going to happen. As for the "could possibly introduce stability issues" we're only taking patches on stuff that's user-visible in some way. Underlying architecture issues and dependencies aren't part of that category. Even if Tbird _was_ shipping Help, I don't think we'd have enough of a reason to land this on branch, since in theory there is no embedding requirement on the 1.0 branch. xulrunner is trunk-only at this point.
(In reply to comment #23) > Is there actually complete and ready documentation for Thunderbird Help? Its > really really late in the game to add this, especially since we're at the l10n > freeze, and there's currently nothing. Oh, so it's just a pragmatic decision? I suppose I can't argue with you there, although it's really sad how little attention Thunderbird gets compared to Firefox. I guess that's what happens when the Foundation can't put as much effort into it as it can in Firefox.
I haven't heard of a decision regarding Help in Thunderbird 1.0. But the current state of Thunderbird Help content is very poor. There's nothing in the tree, and http://www.mozilla.org/projects/help-viewer/installation.php only provides a "0.1 Beta" version as an extension, which contains almost no content and doesn't even work. I don't know if there's something newer in mozdev cvs. The Thunderbird 1.0 is scheduled together with Firefox 1.0, which means end of October right now. The l10n isn't necessarily an issue, since localizers don't need to include Help. They don't even have to include Firefox Help. Maybe we could copy some of Seamonkey Help, but it's still a terrible amount of work, and I doubt anyone is likely to contribute some 50-100 hours in the next few weeks. However, this bug is not necessary for Thunderbird 1.0 Help. The app-specific #ifdefs are in place to make Thunderbird Help possible. We don't need to remove those for Thunderbird Help (only the start page has to be changed). We want to remove them to make toolkit embeddable, i.e. not dependent on Firefox or Thunderbird.
I definetely agree with mconnor. If you want to scrap the ifdefs, then burn the Thunderbird ones. They're never used anyways. Just there for the future. MozDev isn't any more recent than whats on the Help Viewer page. I know because I've been looking at it. Not sure what path you're going, but if you're going against ifdefs, then pretend like Thunderbird Help doesn't exist, because it basically doesn't. Hopefully someone will get it for 2.0 :). The real hard part is the platform classes. Those are really useful for the help documentation and used often, making them difficult to remove. Those ifdefs will also likely break embeddable Help. I haven't been tracking this bug so it might've been mentioned but just something to bring up if it hasn't yet.
(In reply to comment #26) > The real hard part is the platform classes. Those are really useful for the help > documentation and used often, making them difficult to remove. Those ifdefs will > also likely break embeddable Help. Not at all, actually. The bug's only for "app-specific" ifdefs: #ifdef MOZ_PHOENIX, #ifndef MOZ_THUNDERBIRD, etc. #ifdef XP_WIN is perfectly okay (and would really, really break the toolkit if they couldn't be there).
Created attachment 168955 [details] [diff] [review] Move Help menu, F1 overlay into browser I have been doing some work on this in preparation for making the Help Viewer work in Thunderbird, but right now my laptop seems to be overheating during times of intense activity -- such as at the very end of the build process (usually during TestGtkNotebookEmbed or whatever the real name is). Consequently, this patch is untested in the full formal build. However, because the build completes 99.9% of the way, I have been able to successfully do partial builds to create Makefile's through the tree. Then, I can create an updated help.jar by using make in (browser|toolkit)/components/help and toolkit/themes/winstripe/help. I've tested the patched help.jar with the 20041216 nightly on FC3, and nothing seems amiss. This patch gets rid of all the app-specific ifdefs in Help code, but it still leaves some browser-dependent Help skin in toolkit. That code will be fixed in a later patch after this patch gets reviewed and applied.
The patch collides with the one in bug 267227, which proposes to get rid of helpMenuOverlay.xul altogether. We might do this as a first step however, since it's straight-forward, whereas bug 267227 is more complicated.
Comment on attachment 168955 [details] [diff] [review] Move Help menu, F1 overlay into browser Obsoleted by bug 267227, awaiting that fix to continue with fixes here...
Putting a useful URL here so I don't have to recreate it if I need it again... http://snurl.com/find_bar_help_xul
Created attachment 178540 [details] [diff] [review] Remove #ifdef for default content pack name This patch removes the platform-specific #ifdef that's used to set the default Help content pack RDF file. I don't really know why we even had the #ifdef in the first place as it serves no real purpose, but with this patch checked in we'll have one less of them. In the long term it probably makes sense to remove the default content pack altogether. If the content pack provided in the call to openHelp() or set earlier by setHelpFileURI() doesn't exist, we would then simply return an error. However, that change can wait until later.
Comment on attachment 178540 [details] [diff] [review] Remove #ifdef for default content pack name Restoring mconnor's review+
Comment on attachment 178540 [details] [diff] [review] Remove #ifdef for default content pack name Bitrotted.
?-ing since a patch exists. Bitrot?
Fixed by bug 268776. Everyone who was holding off from using the toolkit Help viewer because it wasn't app-independent should now be able to use it without unorthodox hacks (assuming, of course, that they ship a more or less proper representation of toolkit - those who don't really ship toolkit *coughThunderbird* need not apply).