220561 - Add better support for content packs

Status: RESOLVED FIXED

(SeaMonkey :: Help Documentation, enhancement)

Description

[R.J. Keller]

We should have the ability to use content pack the way that Firebird Help uses them.

Comment 1

[R.J. Keller]

I'll get this in once the bug 219120 - help.js cleanup is in.

Comment 2

[Daniel Wang]

question: is this about language pack (bug 87225?) or content pack? also is this
about switching language in Help?

Comment 3

[R.J. Keller]

This has nothing to do with localization. The purpose of this bug is to port
Brant Gurganus's patch for adding support for content pack. A content pack is a
package of files that define the TOC, Index, and help files.

Firebird Help has 2 content packs, one for Firebird Help content, another for
Help  on how to use the Help menu, and, soon to come, a Thunderbird Help content
pack.

Currently in Firebird Help, the Firebird and Help on Help content packs are
merged in the help window. This ability should be in Seamonkey because it makes
it a lot easier for people outside of help to use the help viewer.

Comment 4

[R.J. Keller]

This is one of many bugs I've filed to port Firebird Help bug fixes to Seamonkey
Help.

I'm also writing documentation about how to use Help content packs (which I'll
put on the Help Viewer Project home page).

Comment 5

[R.J. Keller]

-> Mozilla 1.7 alpha.

This is a much bigger change than I'd originally thought. Probably not going to
make 1.6.

Comment 6

[R.J. Keller]

Attachment: Patch implements Content Packs specification

Comment 7

[[e-mail]]

Sorry, I don't understand the point of this bug - SeaMonkey can already open
help windows for different packages, for instance my XULMine game comes with its
own help index.

Comment 8

[R.J. Keller]

True, but it's not that easy. This bug will change the functions in
contextHelp.js so that instead of having to make their own version of the
openHelp() function to specify the content pack, they can use the one already there.

This function is much easier for embedders and more user-friendly. See the
Contents Pack specification (in the URL field) to see what I'm hoping to
accomplish in this bug in greater detail.

Comment 9

[[e-mail]]

I just put
setHelpFileURI("chrome://xulmine/locale/help.rdf");
in my onLoad function - then I can just call openHelp(); for help...

Comment 10

[R.J. Keller]

The problem I have with doing that is what if you have multiple content packs
for a specific app? Something i'd like to do with Mozilla in the future is to
split the current Mozilla content pack into many content packs (which would be
convient if Mail, Composer, etc. wasn't built). It's quite inconvienient to call
setHelpFileURI();OpenHelp() every time you want to open help with a different
content pack.

Comment 11

[R.J. Keller]

Comment on attachment 137817 [details] [diff] [review]
Patch implements Content Packs specification

after talking with Neil privately, we've decided on a slightly modified system
that I will post momentarily.

Comment 12

[R.J. Keller]

Attachment: Patch implements Content Packs 1.2

Patch implements Content Packs Specification v1.2, which contains feedback by
Neil. This patch is a LOT simpler than the other patch!

Comment 13

[[e-mail]]

Comment on attachment 138046 [details] [diff] [review]
Patch implements Content Packs 1.2

>+// openHelp - Opens up the Mozilla Help Viewer with the specified
>+//    content pack and topic.
Should say "topic and content pack", because that's the parameter order.
I wonder whether a JavaDoc-style comment would be more appropriate.

>+// see http://www.mozilla.org/projects/help-viewer/content_packs.html
>+function openHelp(topic, contentPack)
>+{
>+  //cp is the content pack to use in this function. If the contentPack
>+  //parameter was set, we will use that content pack. If not, we will
>+  //use the default content pack set by setHelpFileURI().
>+  var cp = (contentPack == null) ? helpFileURI : contentPack;
Another nit: contentPack || helpFileURI; works better here.

>+  var aWindow;
> 
>   // Loop through the help windows looking for one with the
>-  // current Help URI loaded.
>+  // current Help Content Pack loaded.
>   while (iterator.hasMoreElements()) {
>-    var aWindow = iterator.getNext();
>+    aWindow = iterator.getNext();
This declaration appears to have been moved unnecessarily.

r=me if you fix those nits.

Comment 14

[R.J. Keller]

Comment on attachment 138046 [details] [diff] [review]
Patch implements Content Packs 1.2

> Should say "topic and content pack", because that's the parameter order.
> I wonder whether a JavaDoc-style comment would be more appropriate.

OK, I'll change that before I checkin. What exactly is the point of Java-Doc
style comments if you can't
use JavaDoc? I probably should've used multiline comments.

> Another nit: contentPack || helpFileURI; works better here.

I don't think that that would work, since it'd always return true. Maybe
!contentPack would be better? I'm not sure if that would work for null, so
that's why I didn't do that.

> This declaration appears to have been moved unnecessarily.

I moved it there because it seems stupid to me to keep declaring a variable for
every interation of the loop. This way is much more efficient. It has nothing
to do with the patch.

Comment 15

[[e-mail]]

I know .idl files have JavaDoc comments but I wasn't sure about .js files.

You must be used to C-style ||; Perl and JavaScript have a more useful || which
means "return the first non-null, non-zero, non-empty value" and can be quite
useful chained, for instance the browser status handler has the line:
var text = this.overLink || this.status || this.jsStatus || this.jsDefaultStatus
|| this.defaultStatus;

Also, the javaScript compiler automatically moves all declarations to the
beginning of the function, in theory you can put them anywhere but we generally
put them in the place where it looks most convenient.

Comment 16

[R.J. Keller]

> I know .idl files have JavaDoc comments but I wasn't sure about .js files.

really? I wonder why.

> You must be used to C-style ||; Perl and JavaScript have a more useful || which
> means "return the first non-null, non-zero, non-empty value" and can be quite
> useful chained, for instance the browser status handler has the line:
>var text = this.overLink || this.status || this.jsStatus || this.jsDefaultStatus
|| this.defaultStatus;

really? that's cool! Java style || actually. I don't know C or C++. I'll make
that change.

> Also, the javaScript compiler automatically moves all declarations to the
> beginning of the function, in theory you can put them anywhere but we generally
> put them in the place where it looks most convenient.

really? so scope isn't done by the brackets? I'll have to do some research on
the way JavaScript is interpreted. It's a lot different than I thought.

Comment 17

[R.J. Keller]

Attachment: Patch with neils comments

> This declaration appears to have been moved unnecessarily.

well, I'll keep it in there for program readability.

Comment 18

[Alec Flett]

Comment on attachment 138084 [details] [diff] [review]
Patch with neils comments

+  var aWindow;


"a" means argument - you don't declare local variables with an "a"

this should be var currentWindow or something.

(I'm also not fond of "cp" for content pack, but whatever :) )

with the aWindow stuff fixed, sr=alecf

Comment 19

[R.J. Keller]

Fix checked in.