Closed Bug 254992 Opened 20 years ago Closed 19 years ago

Need a way to make ToC/Index entries platform-specific

Categories

(SeaMonkey :: Help Viewer, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: jwalden+fxhelp, Assigned: jwalden+fxhelp)

References

Details

Attachments

(6 files, 5 obsolete files)

I'm working on bringing the menu reference up to snuff.  For items like
Tools>Options (Edit>Preferences on Linux, and therein lies the rub), it appears
that it's impossible to make only one show up in the table of contents unless
the file is preprocessed, which isn't doable because it'd be a localization
hurdle.  This would also apply to Windows-only entries, like the email items
under Tools in Windows, Default Browser on the Mac, etc.

I don't know how this is doable, but if it's possible to add a custom attribute
to some entries in the ToC to mark them in the same way that CSS can be used to
mark elements in the docs themselves as win/noWin/mac/noMac/unix/noUnix, that
would work.  Unfortunately, it's impossible for 1.0 (I'm not sure exactly what
I'll do for now, because CSS hiding would be confusing if the entry doesn't
exist).  I do, however, think it's an extremely important feature for After
Firefox 1.0 to ensure platform conformity.  (It should have been done before
1.0, but of course we're 20/20 now.)

I won't take this because I don't know enough yet, but I may give it a stab as I
learn more about how Help works (which has zero chance of happening to the
degree needed anytime before 1.0).
*** Bug 254991 has been marked as a duplicate of this bug. ***
Accepting. I got an idea for this.
Status: NEW → ASSIGNED
(In reply to comment #2)
> Accepting. I got an idea for this.

Can we fix this for Seamonkey as well?
While I'm thinking about it, Index would also need a fix eventually, because
it'll have links to platform-specific stuff like Default Browser.
Summary: Need a way to make ToC entries platform-specific → Need a way to make ToC/Index entries platform-specific
My idea didn't work out so well :). Neil, do you have any idea on a possible
solution? We need this for Seamonkey as well as Firefox (give me one and I'll
make the other). I was thinking of making a attribute hack but it didn't work
too well.
Moving to new Firefox Help owner.
Assignee: rlk → jwalden+fxhelp
Status: ASSIGNED → NEW
I don't know enough about RDF and friends to even attempt fixing this for 1.0. 
I'd bet it'd be a pretty big change, anyways, and I'm not sure I'd want to
introduce it into what's supposed to be the first mission-critical Firefox
release.  It looks bad, sure, but it's not something that really, really matters
in the grand scheme of things.  Patches still accepted...
Target Milestone: --- → After Firefox 1.0
I think it could be done along these lines:
1. Each platform has a platform-specific RDF file in the tree. Using jar.mn
preprocessing this file is then built into a known location in content.
2. Each platform has a platform-specific RDF file in locale. These are separate
files although due to 1. above only one will be used on each platform.
3. Common entries remain in the existing RDF files.
*** Bug 262744 has been marked as a duplicate of this bug. ***
This is a very noticeable issue, and it needs to be fixed.  (Docs are also
important, but right now I seem to be most productive at working with code
instead of docs, so I'm sticking with what's working.  Reviews will still
happen, but I'm afraid I have a copy-writing block at the moment.  Hopefully
it'll be gone by the end of the week.)

I'll try some hacking at this via the method Neil discussed to see if I can get
anything to work.  If I can't get anything to work without being forced to make
anything more than trivial edits toolkit/components/help/content/help.js, then
it probably won't get done without outside help from someone who knows RDF,
Mozilla's RDF interfaces, and the workings of this code.
Status: NEW → ASSIGNED
Target Milestone: After Firefox 1.0 → Firefox1.0
The attached help.js does this at run time by copying the datasources into
memory and filtering out the entries that are not for the current platform.

It filters the ToC, Search, glossary and index.

In the RDF files, add nc:platform="xxx" to any platform-specific entry, where
xxx is a space-separated list of case-insensitive tokens.  The meaningful
tokens are:

  unix mac os/2 win nounix nomac noos/2 nowin

The first token in the list implies the converse for other platforms.  For
example, unix implies nomac noos/2 nowin, or nounix implies mac os/2 win.

Normally specify just one or two tokens to get the effect you need.

If you omit nc:platform, then the entry is displayed on all platforms.

I produced this by hacking my installed help.js in Firefox, because I don't
yet know how to work CVS.  Maybe I'll figure it out soon...  Meanwhile if
someone sends me the source file I'll be happy to make the changes and return it.
Attached patch Proposed help.js patch (obsolete) — Splinter Review
I got CVS to work.  Here's the same thing as a patch.
Attachment #161661 - Attachment is obsolete: true
(In reply to comment #13)

Wow.  I've been struggling all weekend to understand 1) RDF and 2) Mozilla's RDF
impl and help.js to make some sort of a hack to fix this (xulplanet's pretty
decent, it seems, but it's still rather opaque for someone without any formal
programming experience), and I was almost at the point of giving up and just
pushing through some sort of workaround (at least for Options/Preferences -- not
much to do about completely missing entries).

Because I don't really know this code I can't actually review it, but I'll
probably give it a quick once-over and then forward it on to someone who can. 
If the patch is deemed of sufficiently high quality, this needs to get in ASAP
so that we can meet the Help documentation freeze (which was targeted for the
15th last I heard).
The previous patch, for whatever reason, had no context to it, making it
difficult to evaluate without adding it to a source tree to read it.  This
patch contains context, making it much more understandable by itself.
Attachment #161668 - Attachment is obsolete: true
Comment on attachment 161677 [details] [diff] [review]
The previous patch, with 8 lines' context

Okay, I've looked at it, and I've mostly got nits.  I can't comment much on the
code itself, however, because I don't quite understand it.  (Oh, in the future
when making a diff, use `cvs diff -pu8N
mozilla/toolkit/components/help/content/help.js` from the top of the
mozilla.org source tree.  You'll give context this way, and the patch will be
less fragile in general.)

>+var platformCode;
...
>+    setPlatformCode();

You leave platformCode as an undefined global variable here and define it later
in setPlatformCode(), called within init().  While it's possible to get the
platform using Javascript, in chrome JS it's not the way I've ever seen used. 
For what are most likely performance reasons, we use the tree's chrome
preprocessor, which lets us use code specific to a certain platform and then
only include that code in the built source.  Here's how you probably want to
define platformCode (insert in place of the line above):

#ifdef XP_WIN
var platformCode = "win";
#else
#ifdef XP_MACOSX
var platformCode = "mac";
#else
#ifdef XP_OS2
var platformCode = "win";
#else
var platformCode = "unix";
#endif
#endif
#endif

The chrome preprocessor works similarly to how the preprocessor works in a
C/C++ compiler.  In the case above, all of XP_WIN, XP_MACOSX, and XP_OS2 are
automatically defined/not defined for the proper target platform for the build.
 It's also worth noting that the preprocessor strips out the lines that aren't
evaluated, so the above will only take one line in compiled source instead of
many.


>-function loadDatabasesBlocking(datasources) {
>+# filter datasources for platform-specific entries, then build tree
>+function setFilteredDatabase(datasources, tree) {
> 	var ds = datasources.split(/\s+/);
>     for (var i=0; i < ds.length; ++i) {

You've used (or rather left) a tab in the |var ds| line above.	Tabs are
technically not allowed in mozilla.org code (its presence here is an anomaly),
so it would be nice if you could remove it.  You should also correct formatting
as you go, too.

>+        var filteredDS = Components
>+            .classes["@mozilla.org/rdf/datasource;1?name=in-memory-datasource"]
>+            .createInstance(Components.interfaces.nsIRDFDataSource);
>+
>+        filterNode(datasource, RDF.GetResource("urn:root"), filteredDS);

Speaking almost 100% from a gut feeling here, I don't know how much your use of
recursion in filterNode() will be liked (it's entirely possible I could be
wrong).

>+        else {
>+            targetDS.Assert(node, arc, tgt, true);
>+            // add tocid attribute for getLink() to look up
>+            var hashpos = node.Value.indexOf("#");
>+            var tocid = node.Value.substr((hashpos == -1)? 0 : hashpos + 1);
>+            targetDS.Assert(
>+                node,
>+                NC_TOCID,
>+                RDF.GetLiteral(tocid),
>+                true);
>+            // recurse down tree
>+            filterNode(sourceDS, tgt, targetDS);
>+        }

When you set tocid, you use node.Value.substr() with only one argument.  This
may or may not work properly (whatever properly is, as I couldn't really
understand the code -- but don't forget I didn't understand the code to start),
but it is definitely wrong.  You need to use substr() with both arguments.

>+# set global plaformCode used by filterNode()
>+function setPlatformCode() {
>+    var plat = window.navigator.platform.toLowerCase();
>+    if (plat.indexOf("mac") == 0)
>+        platformCode = "mac";
>+    else if (plat.indexOf("os/2") == 0)
>+        platformCode = "os/2";
>+    else if (plat.indexOf("win") == 0)
>+        platformCode = "win";
>+    else platformCode = "unix";
> }

I notice that you define a platformCode="os/2" here.  Because OS/2 isn't
officially supported, there's pretty much no code that is OS/2-specific within
toolkit or browser.  In fact, the one place in Help that's used to create some
platform-specific code specifically diverts OS/2 into the same subset as
Windows.  (That's probably an error as standalone #ifdef XP_WINs appear pretty
frequently through source, but OS/2 behavior should be expected to be erratic
as long as no one working on Help tests it.)  Anyways, we basically shouldn't
do any OS/2 filtering, and the preprocessor code I give above will properly
filter OS/2 in with Windows.

Aside from those comments, I don't have anything else to say on the rest of the
patch.	It /looks/ like it works as desired, even if in a small test it didn't
work (my syntax for defining the platform-ness of a node in firebirdhelp.rdf
could easily have been wrong).	Neil is much more familiar with how the RDF
code in Help works, so I'll forward this on to him for a real review.
Attachment #161677 - Flags: review?(neil.parkwaycc.co.uk)
Thanks for all your comments.  It will be obvious how unfamiliar I am with
Mozilla coding!  A couple of remarks make me think it would be worthwhile
explaining the approach I took.

When the existing code loads the ToC or any other panel, loadHelpRDF() gets a 
list of RDF files from (say) firefoxhelp.rdf, calls loadDatabasesBlocking() to 
preload them, and sets the tree's datasources.  This automatically makes the 
tree rebuild.  The aggregated datasources are available in the tree's database 
property.

To open a particular help topic, getLink() looks up its ID in the aggregated
ToC.  Unfortunately the ID has file scope--it is tied to the file that it 
originally came from.  This is a fault in the original design.  To work around 
it,  getLink() finds the names of the original files and tries each of them in
turn.

My approach was to replace loadDatabasesBlocking() with setFilteredDatabase().
This preloads the RDFs as before, but calls filterNode() to filter out any 
unwanted platform-specific parts, storing the result in a temporary datasource
in memory.  Then it adds the temporary datasource to the tree's database.  By
doing it this way, the tree has to be rebuilt explicitly.

filterNode() is recursive because it walks the tree structure of the ToC.
An expert might be able to copy and filter the datasource without walking
the tree, just by copying the arcs individually, but I could not make it
work that way.

Now getLink() cannot work around the ID problem in the same way as before.
It cannot get the original RDF file names from the tree, because the tree
got its data from my filtered in-memory copies and knows nothing about the 
files.  So I worked around it in a different way, making filterNode copy each
ID (without the file name) into a tocid attribute.  This simplifies getLink().

substr with one argument goes to the end of the string, as in ECMA-262.  It
is not hard to find other examples of this usage in Mozilla code.

I have a personal preference for platform-independent code that determines the
platform at run time.  This is because I want to be able to put a help viewer 
in a XPI that will work on any platform.  But I understand that there may be
good reasons to use the preprocessor instead.  I can easily work around it.
No way now this'll get in for 1.0.  Pushing off the list...
Target Milestone: Firefox1.0 → After Firefox 1.0
Is this bug branch-only? Should it be moved to Trunk?
Target Milestone: Future → Firefox1.1
Version: 1.0 Branch → Trunk
Flags: review?(neil.parkwaycc.co.uk)
Product: Firefox → Toolkit
Target Milestone: Firefox1.1 → ---
Version: Trunk → unspecified
Comment on attachment 161677 [details] [diff] [review]
The previous patch, with 8 lines' context

Rerequesting review, er first-review, after the component move, for Jeff.
Attachment #161677 - Flags: first-review?(neil.parkwaycc.co.uk)
Targetting bugs which were targetted to Firefox1.1 before the move to
mozilla1.8beta2.
Target Milestone: --- → mozilla1.8beta2
Attached patch First DraftSplinter Review
Naturally I'm working on Suite help but I expect the port to be trivial.
Comment on attachment 178742 [details] [diff] [review]
First Draft

Actually, I like this quite much... Especially, how mozillahelp.rdf goes to
content and leaves only a dtd behind... now if we could do that for help-toc
and help-win as well, I'd be even happier... but I guess that's a different bug
:)
Comment on attachment 178742 [details] [diff] [review]
First Draft

If we fix the problem this way, we fix it for ourselves, but we don't fix it in
a way that will let extensions use it.	Contrast this with the original patch,
which would have been usable by extensions.  Is this loss something we should
be concerned about?  I'm inclined to think it is, but I'm not familiar enough
with external uses of the Help Viewer to be able to argue this point.

One advantage to the first method, however, is that it wouldn't require
creating a bunch of new files in source code to make things work properly.  The
second method requires simultaneous checkin of all the platform-specific code,
which will be slower (for Firefox Help, at least, if not Suite Help) than
committing a fix and making trivial attribute changes in the appropriate RDF
files at a later time.
Attachment #178742 - Flags: first-review?(doronr)
Comment on attachment 178742 [details] [diff] [review]
First Draft

>Index: content/mozillahelp.rdf
>===================================================================
>RCS file: content/mozillahelp.rdf
>diff -N content/mozillahelp.rdf
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ content/mozillahelp.rdf	27 Mar 2005 22:03:46 -0000
>@@ -0,0 +1,38 @@
>+<?xml version="1.0"?>
>+
>+<!DOCTYPE rdf:RDF [
>+<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
>+%brandDTD;
>+<!ENTITY % helpDTD SYSTEM "chrome://help/locale/mozillahelp.dtd" >
>+%helpDTD;
>+]>
>+
>+<rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>+         xmlns:nc="http://home.netscape.com/NC-rdf#">
>+
>+<!-- MASTER HELP DOCUMENT -->
>+  <rdf:Description about="urn:root"
>+          nc:title="&title;"
>+          nc:defaulttopic="welcome"
>+          nc:base="chrome://help/locale/">
>+    <nc:panellist>
>+      <rdf:Seq>
>+        <rdf:li> <rdf:Description nc:panelid="glossary" nc:datasources="help-glossary.rdf"/> </rdf:li>
>+#ifdef XP_WIN
>+        <rdf:li> <rdf:Description nc:panelid="index" nc:datasources="help-indexAZ.rdf help-index1.rdf"/> </rdf:li>
>+        <rdf:li> <rdf:Description nc:panelid="toc" nc:datasources="help-toc.rdf"/> </rdf:li>
>+#else
>+        <rdf:li> <rdf:Description nc:panelid="index" nc:datasources="help-indexAZ.rdf help-index1.rdf help-win.rdf"/> </rdf:li>
>+        <rdf:li> <rdf:Description nc:panelid="toc" nc:datasources="help-toc.rdf help-win.rdf"/> </rdf:li>
>+#endif

Shouldn't help-win.rdf be in the #ifdef XP_WIN part?
(In reply to comment #24)
The patch does not require a simultaneous checkin of the platform-specific
files. The mozillahelp.rdf can be moved to content as the first step. Only when
someone actually moves a contents or index entry might they need to create a new
file.

Note that extensions can adapt my idea by using script rather than preprocessing
to choose between master help files.

Although, you've got me thinking... maybe the master help file should contain
RDF lists of toc and index files rather than space-separated values...
(In reply to comment #26)
> Note that extensions can adapt my idea by using script rather than
> preprocessing to choose between master help files.

Scripting is still to a degree hacking around it, and IMHO Help should simply
dynamically adapt without the programmer having to think about it.

Here's another idea along the lines of what you're proposing that wouldn't
require quite as much work for extension writers: add an optional attribute to
each panel definition within help.rdf that would contain one (or more) OS
string(s).  Then, add datasources if no such attribute exists (i.e.,
cross-platform datasource) or the attribute equals (or contains) the appropriate
OS string.  Even for someone like me who's unfamiliar with RDF, this wouldn't be
at all difficult.  (Right now I'm feeling like I might even hack this into a
patch tomorrow if the difficulty isn't too much for me.)

> Although, you've got me thinking... maybe the master help file should contain
> RDF lists of toc and index files rather than space-separated values...

There are a few existing users who might not like this if the old way didn't
still work, but I personally couldn't care.  What we have may not be as RDF-like
as it could be, but it's a nitpicky point (and probably another bug).
Attached patch Something like this? (obsolete) — Splinter Review
So, using the same help-win.rdf as before, but now (as yet unwritten) help.js
code tries urn:<platform> before url:<root> ?
I mean urn:root of course.
No, more like this.  Instead of having a whole bunch of definition duplication,
just add the attribute to the datasource definition for each help panel.  If
the panel's platform attribute is empty, non-existent, or contains the
appropriate platform string, add it to the datasources -- otherwise, move on to
the next panel definition.  The logic's simple to implement, and the results
are about as expected in my Linux build.  (I haven't yet tested whether panels
are properly hidden when they're specified for another platform, but I don't
expect surprises.)

With the patch above applied, some of the expected changes occur as expected. 
The totally new top-level RDF node "foo-toplevel" is displayed in the table of
contents after all the previous datasources' nodes.  The node "foobar" is added
at the end in the correct place, too.  The small problem comes with the
redefinition of "prefs" and "prefs-general".  I'd hoped these would overwrite
the originals, but apparently they don't -- new items were added instead of old
items being overwritten.  This is probably just an area of RDF that I haven't
read about yet (I hope).  As a worst-case scenario, we either define the
relevant elements twice or add in an sharedRDF.dtd containing duplications,
which would get around the problem.

This also ignores the problem of ordering.  It appears the composite is created
such that nodes added later are automatically pushed to the end of the list,
which isn't optimal, as order often *does* matter (for example, preference
panel items, which would be in the order of the panels themselves).  This can
be worked around by expanding the datasource into more and more RDF files as
necessary, but it's still not optimal.

Anyway, there's my solution, more or less as it would be added.  I don't know
RDF or the RDF API well enough to get very far with the above questions, so
I'll leave this to you now.
Attachment #179482 - Attachment is obsolete: true
Comment on attachment 179521 [details] [diff] [review]
More like this then? (untested)

Yeah, pretty much, but at least in the toolkit patch we're probably going to
use #ifdefs because it's how we usually handle such things.
Attached patch Completed patchSplinter Review
Attachment #179521 - Attachment is obsolete: true
Attachment #179588 - Flags: first-review?(doronr)
Comment on attachment 179588 [details] [diff] [review]
Completed patch

>+var emptySearchText = "No search items found.";

Shouldn't this be localized?

>+var emptySearchLink = "about:blank";
> var helpTocPanel;
> var helpIndexPanel;
> var helpGlossaryPanel;
>@@ -59,6 +59,7 @@
> const RDF_ROOT = RDF.GetResource("urn:root");
> const NC_PANELLIST = RDF.GetResource(NC + "panellist");
> const NC_PANELID = RDF.GetResource(NC + "panelid");
>+const NC_PLATFORM = RDF.GetResource(NC + "platform");
> const NC_EMPTY_SEARCH_TEXT = RDF.GetResource(NC + "emptysearchtext");
> const NC_EMPTY_SEARCH_LINK = RDF.GetResource(NC + "emptysearchlink");
> const NC_DATASOURCES = RDF.GetResource(NC + "datasources");
>@@ -87,6 +88,9 @@
> var defaultTopic = "welcome"; 
> var searchDatasources = "rdf:null";
> var searchDS = null;
>+var platform = /mac/i.test(navigator.platform) ? "mac" :
>+               /win/i.test(navigator.platform) ? "win" :
>+               /os\/2/i.test(navigator.platform) ? "os/2" : "unix";
> 
> const NSRESULT_RDF_SYNTAX_ERROR = 0x804e03f7; 
> 
>@@ -212,24 +239,27 @@ function loadHelpRDF() {
>       var panelDefs = helpFileDS.GetTarget(RDF_ROOT, NC_PANELLIST, true);      
>       RDFContainer.Init(helpFileDS, panelDefs);
>       var iterator = RDFContainer.GetElements();
>-        while (iterator.hasMoreElements()) {
>+      while (iterator.hasMoreElements()) {
>         var panelDef = iterator.getNext();
>-        var panelID = getAttribute(helpFileDS, panelDef, NC_PANELID, null);        
>+        if (getAttribute(helpFileDS, panelDef, NC_PLATFORM, platform) != platform)
>+          continue; // ignore datasources for other platforms.
> 
>-        var datasources = getAttribute(helpFileDS, panelDef, NC_DATASOURCES, "rdf:none");
>+        var panelID = getAttribute(helpFileDS, panelDef, NC_PANELID, null);
>+
>+        var datasources = getAttribute(helpFileDS, panelDef, NC_DATASOURCES, "rdf:null");
>         datasources = normalizeLinks(helpBaseURI, datasources);
>         // cache additional datsources to augment search datasources.
>         if (panelID == "search") {
>-	       emptySearchText = getAttribute(helpFileDS, panelDef, NC_EMPTY_SEARCH_TEXT, "No search items found.");
>-	       emptySearchLink = getAttribute(helpFileDS, panelDef, NC_EMPTY_SEARCH_LINK, "about:blank");
>-          searchDatasources = datasources;
>-          datasources = "rdf:null"; // but don't try to display them yet!
>+          emptySearchText = getAttribute(helpFileDS, panelDef, NC_EMPTY_SEARCH_TEXT, emptySearchText);
>+          emptySearchLink = getAttribute(helpFileDS, panelDef, NC_EMPTY_SEARCH_LINK, emptySearchLink);
>+          searchDatasources += " " + datasources;
>+          continue; // but don't try to display them yet!
>         }  
> 
>         // cache toc datasources for use by ID lookup.
>         var tree = document.getElementById("help-" + panelID + "-panel");
>         loadDatabasesBlocking(datasources);
>-        tree.setAttribute("datasources", datasources);
>+        tree.setAttribute("datasources", tree.getAttribute("datasources") + " " + datasources);
>       }  
>     }
>     catch (e) {
>Index: content/help.xul
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/help/resources/content/help.xul,v
>retrieving revision 1.57
>diff -u -d -r1.57 help.xul
>--- content/help.xul	19 Feb 2005 00:50:40 -0000	1.57
>+++ content/help.xul	27 Mar 2005 22:03:46 -0000
>@@ -155,8 +155,7 @@
>         </template>
>             <treecols>
>                <treecol id="GlossaryNameColumn" flex="1"
>-                  hideheader="true"
>-                  primary="true"/>
>+                  hideheader="true"/>
>             </treecols>
>          </tree>
> 
>@@ -248,7 +247,7 @@
> 
>                  <treecols>
>                     <treecol id="ResultsColumn" flex="1"
>-                             hideheader="true" primary="true"
>+                             hideheader="true"
>                              sortActive="true"
>                              sortDirection="ascending"
>                              sort="?name"/>
>Index: locale/en-US/help-index1.rdf
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/help/resources/locale/en-US/help-index1.rdf,v
>retrieving revision 1.42
>diff -u -d -r1.42 help-index1.rdf
>--- locale/en-US/help-index1.rdf	6 Feb 2005 21:37:16 -0000	1.42
>+++ locale/en-US/help-index1.rdf	27 Mar 2005 22:03:49 -0000
>@@ -1828,16 +1828,6 @@
>    </nc:subheadings>
> </rdf:Description>
> 
>-<rdf:Description about="help-indexAZ.rdf#q">
>-   <nc:subheadings>
>-     <rdf:Seq><rdf:li>
>-       <rdf:Description ID="Quick_Launch"
>-         nc:name="Quick Launch"
>-	 nc:link="nav_help.xhtml#using_quick_launch"/>
>-     </rdf:li></rdf:Seq>
>-   </nc:subheadings>
>-</rdf:Description>
>-
> <rdf:Description about="help-indexAZ.rdf#r">
>    <nc:subheadings>
>      <rdf:Seq><rdf:li>
>@@ -1934,11 +1924,6 @@
>        <rdf:Description ID="SSL"
>          nc:name="SSL"
> 	 nc:link="ssl_help.xhtml#edit_ciphers"/>
>-     </rdf:li>
>-     <rdf:li>
>-       <rdf:Description ID="System"
>-         nc:name="System Preferences"
>-	 nc:link="cs_nav_prefs_advanced.xhtml#system"/>
>      </rdf:li></rdf:Seq>
>    </nc:subheadings>
> </rdf:Description>
>Index: locale/en-US/help-toc.rdf
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/help/resources/locale/en-US/help-toc.rdf,v
>retrieving revision 1.73
>diff -u -d -r1.73 help-toc.rdf
>--- locale/en-US/help-toc.rdf	25 Mar 2005 23:09:59 -0000	1.73
>+++ locale/en-US/help-toc.rdf	27 Mar 2005 22:03:52 -0000
>@@ -131,7 +131,6 @@
>         <rdf:li> <rdf:Description ID="nav-doc-cache" nc:name="Changing Cache Settings" nc:link="nav_help.xhtml#changing_cache_settings"/> </rdf:li>
>         <rdf:li> <rdf:Description ID="nav-doc-smartup" nc:name="Getting the Latest Software Automatically" nc:link="nav_help.xhtml#getting_the_latest_software_automatically"/> </rdf:li>
>         <rdf:li> <rdf:Description ID="nav-doc-mousewheel" nc:name="Using a Mouse Wheel" nc:link="nav_help.xhtml#using_a_mouse_wheel"/> </rdf:li>
>-        <rdf:li> <rdf:Description ID="nav-doc-quicklaunch" nc:name="Using Quick Launch" nc:link="nav_help.xhtml#using_quick_launch"/> </rdf:li>
>       </rdf:Seq>
>     </nc:subheadings>
>   </rdf:Description>
>@@ -804,7 +803,6 @@
>         <rdf:li><rdf:Description ID="advanced_pref_installation" nc:name="Software Installation" nc:link="cs_nav_prefs_advanced.xhtml#software_installation"/> </rdf:li>
>         <rdf:li><rdf:Description ID="advanced_pref_mouse_wheel" nc:name="Mouse Wheel" nc:link="cs_nav_prefs_advanced.xhtml#mouse_wheel"/> </rdf:li>
>         <rdf:li><rdf:Description ID="advanced_pref_dom_inspector" nc:name="DOM Inspector" nc:link="cs_nav_prefs_advanced.xhtml#dom_inspector"/> </rdf:li>
>-        <rdf:li><rdf:Description ID="advanced_pref_system" nc:name="System" nc:link="cs_nav_prefs_advanced.xhtml#system"/> </rdf:li>
>       </rdf:Seq>
>     </nc:subheadings>
>   </rdf:Description>
>Index: locale/en-US/help-win.rdf
>===================================================================
>RCS file: locale/en-US/help-win.rdf
>diff -N locale/en-US/help-win.rdf
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ locale/en-US/help-win.rdf	27 Mar 2005 22:03:52 -0000
>@@ -0,0 +1,41 @@
>+<?xml version="1.0"?>
>+<rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>+         xmlns:nc="http://home.netscape.com/NC-rdf#">
>+ 
>+  <rdf:Description about="help-toc.rdf#nav-doc-ses">
>+    <nc:subheadings>
>+      <rdf:Seq>
>+        <rdf:li> <rdf:Description ID="nav-doc-quicklaunch" nc:name="Using Quick Launch" nc:link="nav_help.xhtml#using_quick_launch"/> </rdf:li>
>+      </rdf:Seq>
>+    </nc:subheadings>
>+  </rdf:Description>
>+
>+  <rdf:Description about="help-toc.rdf#advanced_pref_advanced">
>+    <nc:subheadings>
>+      <rdf:Seq>
>+        <rdf:li><rdf:Description ID="advanced_pref_system" nc:name="System" nc:link="cs_nav_prefs_advanced.xhtml#system"/> </rdf:li>
>+      </rdf:Seq>
>+    </nc:subheadings>
>+  </rdf:Description>
>+
>+<rdf:Description about="help-indexAZ.rdf#q">
>+   <nc:subheadings>
>+     <rdf:Seq><rdf:li>
>+       <rdf:Description ID="Quick_Launch"
>+         nc:name="Quick Launch"
>+	 nc:link="nav_help.xhtml#using_quick_launch"/>
>+     </rdf:li></rdf:Seq>
>+   </nc:subheadings>
>+</rdf:Description>
>+
>+<rdf:Description about="help-indexAZ.rdf#s">
>+   <nc:subheadings>
>+     <rdf:Seq><rdf:li>
>+       <rdf:Description ID="System"
>+         nc:name="System Preferences"
>+	 nc:link="cs_nav_prefs_advanced.xhtml#system"/>
>+     </rdf:li></rdf:Seq>
>+   </nc:subheadings>
>+</rdf:Description>
>+
>+</rdf:RDF>
Attachment #179588 - Flags: first-review?(doronr) → first-review+
Attached patch toolkit version of patch (obsolete) — Splinter Review
It's pretty much the same as the above patch with localization of the two
search strings added in.
Comment on attachment 179639 [details] [diff] [review]
toolkit version of patch

This may end up waiting until after the tree closes, but it's simple enough I
guess it can live with a little extra hassle if necessary.
Attachment #179639 - Flags: first-review?(neil.parkwaycc.co.uk)
(In reply to comment #34)
>(From update of attachment 179588 [details] [diff] [review])
>>+var emptySearchText = "No search items found.";
>Shouldn't this be localized?
It is, in mozillahelp.rdf
Comment on attachment 179639 [details] [diff] [review]
toolkit version of patch

>+# Strings
>+var bundle = document.getElementById("bundle_help");
>+var emptySearchText = bundle.getString("emptySearchText");
>+var emptySearchLink = bundle.getString("emptySearchLink");
IIRC you have to load strings from your onload function as the XBL won't work
until then, assuming that you're actually able to retrieve the DOM element at
this point, which I doubt, thus the r-, unless someone like bryner says that
this is safe.

>                     emptySearchText = getAttribute(helpFileDS, panelDef,
>+                        NC_EMPTY_SEARCH_TEXT, null) || emptySearchText;
>                     emptySearchLink = getAttribute(helpFileDS, panelDef,
>+                        NC_EMPTY_SEARCH_LINK, null) || emptySearchLink;
Nit: nice though the || operator is, the fourth parameter to getAttribute
should be the default value anyway.

>+emptySearchLink=about:blank
I'm not quite sure why you felt it necessary to localize about:blank :-P
Attachment #179639 - Flags: first-review?(neil.parkwaycc.co.uk) → first-review-
(In reply to comment #38)
> IIRC you have to load strings from your onload function as the XBL won't work

> until then

Fixed and tested.

> Nit: nice though the || operator is, the fourth parameter to getAttribute
> should be the default value anyway.

Yes, true, and also fixed.

> I'm not quite sure why you felt it necessary to localize about:blank :-P

Me neither.  I was just in a rush and wasn't thinking properly.

I also moved Help's string bundle out to be a global variable instead of having
it be manually queried any time a string from it was needed.
Attachment #179639 - Attachment is obsolete: true
Attachment #179804 - Flags: first-review?(neil.parkwaycc.co.uk)
Attachment #179804 - Flags: first-review?(neil.parkwaycc.co.uk) → first-review+
Comment on attachment 179804 [details] [diff] [review]
Updated to address review

Having parity with SeaMonkey's Help would be a Good Thing(TM).	This will also
be useful for Firefox Help and the calendarhelp project on mozdev, as I assume
they'll port the fix over into their mirror of the code when they have time.
Attachment #179804 - Flags: approval-aviary1.1a?
Attachment #161677 - Flags: first-review?(neil.parkwaycc.co.uk)
Attachment #178742 - Flags: first-review?(doronr)
Comment on attachment 179804 [details] [diff] [review]
Updated to address review

a=asa
Attachment #179804 - Flags: approval-aviary1.1a? → approval-aviary1.1a+
Blocks: 289776
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Blocks: 295904
Attached patch Missing linkSplinter Review
Attachment #191513 - Flags: first-review?(doronr)
Attachment #191513 - Flags: first-review?(doronr) → first-review+
Comment on attachment 191513 [details] [diff] [review]
Missing link

Trivial suite help fix accidentally omitted from attachment 179588 [details] [diff] [review].
Attachment #191513 - Flags: approval1.8b4?
Attachment #191513 - Flags: approval1.8b4? → approval1.8b4+
Comment on attachment 191513 [details] [diff] [review]
Missing link

Neil, can you check this in please?
Flags: in-testsuite-
Product: Toolkit → Seamonkey
You need to log in before you can comment on or make changes to this bug.