346605 - Make openHelp() calls call suiterunner help correctly

Status: RESOLVED FIXED

(SeaMonkey :: Help Documentation, defect)

Description

[Robert Kaiser]

Once both bug 344243 has been checked in, suiterunner needs to call the help window correctly on openHelp(), either by adding a parameter stating the help.rdf URI correctly or by setting a default URI via setHelpFileURI().

This would probably break xpfe-based suite, so the plan is to only do this once bug 328887 has been fixed, i.e. we have only suiterunner to deal with.

Comment 1

[[e-mail]]

Alternatively we could install help locale to chrome://communicator/content/help for both xpfe and toolkit suite, which would then allow us to change all the calls to openHelp without immediately breaking xpfe.

Comment 2

[[e-mail]]

Attachment: Part 1: move XPFE to use new help files

Note: these changes are deliberately slightly different to those in bug 344243.

Comment 3

[Robert Kaiser]

Comment on attachment 233236 [details] [diff] [review]
Part 1: move XPFE to use new help files

Nich patch for unforking the help files, though it doesn't solve the issue of this bug.

>Index: mozilla/suite/locales/jar.mn
>===================================================================
>+  locale/@AB_CD@/communicator/help/suite-toc.rdf                            (%chrome/common/help/suite-toc.rdf)

If we do it that way, we need to point suiterunner to this file to solve the bug here. No problem, just a note.

>Index: mozilla/suite/locales/en-US/chrome/common/help/suitehelp.rdf
>===================================================================

I'd really like to change this file to the same style as other toolkit-based apps have it, just like I did/do in my patch to the other bug.

Additionally, the parts from there that deal with the suiterunner-specific security help pointers still apply, also with your patch here.

Comment 4

[[e-mail]]

(In reply to comment #3)
>(From update of attachment 233236 [details] [diff] [review])
>Nice patch for unforking the help files, though it doesn't solve the issue of
>this bug.
Thus its name, "Part 1" ...

>>Index: mozilla/suite/locales/en-US/chrome/common/help/suitehelp.rdf
>I'd really like to change this file to the same style as other toolkit-based
>apps have it
Which makes it different style to a) our other RDF files b) toolkit help RDF.
Or do you intend to reformat them all too?

Comment 5

[Robert Kaiser]

(In reply to comment #4)
> >>Index: mozilla/suite/locales/en-US/chrome/common/help/suitehelp.rdf
> >I'd really like to change this file to the same style as other toolkit-based
> >apps have it
> Which makes it different style to a) our other RDF files b) toolkit help RDF.
> Or do you intend to reformat them all too?

I'd like to have the same style in suiterunner as in http://lxr.mozilla.org/mozilla/source/browser/locales/en-US/chrome/help/firebirdhelp.rdf
while it makes sense for xpfe to keep the old version - maybe it would be a good idea to leave that file forked in the source.

Additionally, your patch fails to remove the now unused files from help.jar in xpfe SeaMonkey.

Could we split this into two parts please, one that makes the relevant URLs local in suite/locales files and one that does the real changeover?
This way, we could get in the former, which we don't need to discuss and is actually orthogonal to the latter, which might still need some discussion.

Comment 6

[[e-mail]]

Attachment: Part 1a: fix relative paths in new help files

Comment 7

[[e-mail]]

Attachment: Part 1b: move XPFE to use new help files

Comment 8

[Robert Kaiser]

Comment on attachment 234407 [details] [diff] [review]
Part 1a: fix relative paths in new help files

This should help a lot, r=me

Comment 9

[Robert Kaiser]

Comment on attachment 234410 [details] [diff] [review]
Part 1b: move XPFE to use new help files

>Index: mozilla/extensions/help/resources/jar.mn
>===================================================================
>-en-US.jar:
>-*       locale/en-US/help/contents.rdf                  (locale/en-US/contents.rdf)
>-+       locale/en-US/help/help.dtd                      (locale/en-US/help.dtd)
>-        locale/en-US/help/helpMenuOverlay.dtd           (locale/en-US/helpMenuOverlay.dtd)

Have you tested this patch? I think you need to leave the lines I listed above in to keep the xpfe help viewer working. I don't believe it can work without locale files for the viewer itself (we don't have those in communicator/ as we have separated the viewer from the content for suiterunner).

>Index: mozilla/suite/locales/en-US/chrome/common/help/suitehelp.rdf
>===================================================================

I'd prefer to use two different main RDF files for xpfe-based suite and suiterunner, so that we use the "old" file with your modifications here for the xpfe-based variant, and use the modifications I had in my patch for suiterunner, so that it fits the file layout I found in the other toolkit help package RDF files. Additionally, I believe the suiterunner file should merge the toolkit help viewer toc file and ours (like I did in my modifications), so that the "how to use help" section is imported from toolkit.

Comment 10

[[e-mail]]

Attachment: Part 1b: move XPFE to use new help content

Comment 11

[Robert Kaiser]

Comment on attachment 241932 [details] [diff] [review]
Part 1b: move XPFE to use new help content

Looks good to me and works well here on our good old xpfe-based SeaMonkey trunk :)

Comment 12

[[e-mail]]

Attachment: Part 2a: fix callers in suite/

Comment 13

[Robert Kaiser]

Comment on attachment 242065 [details] [diff] [review]
Part 2a: fix callers in suite/

r=me, this looks good, but I haven't tested it, as it probably will conflict with the changes I already have in my tree from the other bug and my builds are still running.

Comment 14

[Ian Neal]

Comment on attachment 242065 [details] [diff] [review]
Part 2a: fix callers in suite/

So why does chrome://communicator/locale/help/suitehelp.rdf have to be added? Should this not be the default value when contentPack is null? http://lxr.mozilla.org/seamonkey/source/extensions/help/resources/content/contextHelp.js#40

Comment 15

[Ian Neal]

Comment on attachment 242065 [details] [diff] [review]
Part 2a: fix callers in suite/

>Index: pref/pref-help.js
>===================================================================
>@@ -60,9 +60,9 @@
> function doHelpButton() {
>   var subsrc = document.getElementById("panelFrame").getAttribute("src");
>   if ( fm[subsrc] ) {
>-  	openHelp(fm[subsrc]);  
>+  	openHelp(fm[subsrc], 'chrome://communicator/locale/help/suitehelp.rdf');
>   } else { 
>-	openHelp('prefs'); 
>+	openHelp('prefs', 'chrome://communicator/locale/help/suitehelp.rdf');
>   }
> }
Just wondering if there is a tidier way of doing this...
> 
>Index: permissions/cookieViewer.js
>===================================================================
>@@ -713,10 +713,10 @@
> 
> function doHelpButton() {
>   if (dialogType == imageType) {
>-    openHelp("image_mgr");
>+    openHelp("image_mgr", "chrome://communicator/locale/help/suitehelp.rdf");
>   } else {
>     var uri = getSelectedTab();
>-    openHelp(uri);
>+    openHelp(uri, "chrome://communicator/locale/help/suitehelp.rdf");
>   }
>   // XXX missing popup help
> }
Again is there a tidier way?

Comment 16

[[e-mail]]

Attachment: Part 2a: fix callers in suite/

It transpires that the cookie viewer doesn't handle images any more, but the author of the permission manager patch (who shall remain nameless) forgot to remove the old code and neither db48x or jag noticed your mistake :-P

As an alternative I was considering adding help attributes to the tabs.

I also rewrote the prefs help code slightly.

Comment 17

[[e-mail]]

Attachment: Missed one

Comment 18

[[e-mail]]

I checked in the suite/ fixes.

Comment 19

[[e-mail]]

Attachment: Part 2b: fix new callers in suite

Comment 20

[Ian Neal]

Comment on attachment 248935 [details] [diff] [review]
Part 2b: fix new callers in suite

Even those that work display a help window all of them seem to generate the following messages in the error console:
Error: no element found
Source File: chrome://communicator/locale/help/search-db.rdf
Line: 1
and
[Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIRDFService.GetDataSourceBlocking]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: chrome://help/content/help.js :: loadHelpRDF :: line 198"  data: no]
Not sure if those are logged as different bug(s).

>Index: helpMessengerOverlay.xul
>===================================================================
>   <dialog id="editDirectories"
>           buttons="accept,cancel,help"
>-          ondialoghelp="return doHelpButton();"/>
>+          ondialoghelp="return openHelp('mail-ldap-properties');"/>
Works but see first comment
> 
>   <dialog id="addDirectory"
>           buttons="accept,cancel,help"
>-          ondialoghelp="return doHelpButton();"/>
>+          ondialoghelp="return openHelp('mail-ldap-properties');"/>
Works but see first comment
> 
>   <dialog id="FilterEditor"
>           buttons="accept,cancel,help"
>-          ondialoghelp="return doHelpButton();"/>
>+          ondialoghelp="return openHelp('mail-filters');"/>
Does not work get blank "untitled window" and following errors in error console:
Help file:  was not found.
TypeError: datasource has no properties
> 
>   <dialog id="junkMailInfo"
>           buttons="accept,help"
>-          ondialoghelp="return openHelp('mail-junk', 'chrome://communicator/locale/help/suitehelp.rdf');"/>
>+          ondialoghelp="return openHelp('mail-junk');"/>
Works but see first comment
> 
>   <dialog id="select-offline"
>           buttons="accept,cancel,help"
>-          ondialoghelp="return doHelpButton();"/>
>+          ondialoghelp="return openHelp('mail-offline-items');"/>
Works but see first comment
> 
>   <dialog id="subscribeWindow"
>           buttons="accept,cancel,help"
>-          ondialoghelp="return doHelpButton();"/>
>+          ondialoghelp="return openHelp('mail-subscribe');"/>
Works but see first comment
> 
>   <dialog id="mailViewListDialog"
>           buttons="accept,help"
>-          ondialoghelp="return openHelp('message-views-using', 'chrome://communicator/locale/help/suitehelp.rdf');"/>
>+          ondialoghelp="return openHelp('message-views-using');"/>
Works but see first comment
> 
>   <dialog id="mailViewSetupDialog"
>           buttons="accept,cancel,help"
>-          ondialoghelp="return doHelpButton();"/>
>+          ondialoghelp="return openHelp('message-views-create-new');"/>
Cannot see help button on that screen.
> 
>   <dialog id="msgCompSecurityInfo"
>           buttons="accept,help"
>-          ondialoghelp="return doHelpButton();"/>
>+          ondialoghelp="return openHelp('compose_security');"/>
Works but see first comment
> 
>   <dialog id="msgReadSecurityInfo"
>           buttons="accept,help"
>-          ondialoghelp="return doHelpButton();"/>
>+          ondialoghelp="return openHelp('received_security');"/>
Works but see first comment

Comment 21

[[e-mail]]

(In reply to comment #20)
>Even those that work display a help window all of them seem to generate the
>following messages in the error console:
>Error: no element found
>Source File: chrome://communicator/locale/help/search-db.rdf
>Line: 1
>and
>[Exception... "Component returned failure code: 0x80520012
>(NS_ERROR_FILE_NOT_FOUND) [nsIRDFService.GetDataSourceBlocking]"  nsresult:
>"0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame ::
>chrome://help/content/help.js :: loadHelpRDF :: line 198"  data: no]
>Not sure if those are logged as different bug(s).
Hmm, that's KaiRo's error that I failed to spot :-/

Comment 22

[[e-mail]]

(In reply to comment #20)
>(From update of attachment 248935 [details] [diff] [review])
>>   <dialog id="mailViewSetupDialog"
>>           buttons="accept,cancel,help"
>>-          ondialoghelp="return doHelpButton();"/>
>>+          ondialoghelp="return openHelp('message-views-create-new');"/>
>Cannot see help button on that screen.
That's not the fault of this patch; you can work around the problem by deleting line 128 of searchTermOverlay.js (the stringBundle: one).

Comment 23

[[e-mail]]

(In reply to comment #20)
>(From update of attachment 248935 [details] [diff] [review])
>>   <dialog id="FilterEditor"
>>           buttons="accept,cancel,help"
>>-          ondialoghelp="return doHelpButton();"/>
>>+          ondialoghelp="return openHelp('mail-filters');"/>
>Does not work get blank "untitled window" and following errors in error console
You actually tried the filter *list* rather than the editor... although you would have had trouble trying the filter editor for the same reason that the mail view setup dialog doesn't work. Oh, and that should have been line 125.

Comment 24

[[e-mail]]

Comment on attachment 248935 [details] [diff] [review]
Part 2b: fix new callers in suite

Rerequesting review as per the last three comments.

Comment 25

[Ian Neal]

Comment on attachment 248935 [details] [diff] [review]
Part 2b: fix new callers in suite

r=me
Added bug 365181 for the help window error messages issue.

Comment 26

[Stefan [:stefanh]]

Talked to Neil - what's left here is editor stuff. We miss a lot of help stuff for Composer.

Comment 27

[Stefan [:stefanh]]

FYI, I'm working on a patch that will fix the calls in Composer. It will probably be ready in 5-7 days.

Comment 28

[Stefan [:stefanh]]

Attachment: Fix Composer dialogs in suiterunner and xpfe trunk

While I was there I removed the MOZ_XUL_APP stuff in extension/help/resources/jar.mn. Note also that we don't have any image map editor (and no help for it). The "Troubleshooting" button in the publish progress dialog is now gone and replaced by a standard Help button. Now users who keeps the dialog open and don't have any uploading failures can click on a help button, fwiw.

Comment 29

[Stefan [:stefanh]]

Comment on attachment 264021 [details] [diff] [review]
Fix Composer dialogs in suiterunner and xpfe trunk

Whoops, forgot to remove some stuff...

Comment 30

[Stefan [:stefanh]]

Attachment: Correct patch for Composer dialogs

Here's a correct patch (I forgot to remove all the references to contextHelp.js). For some info, see comment #28.

Comment 31

[[e-mail]]

Comment on attachment 264024 [details] [diff] [review]
Correct patch for Composer dialogs

>+    function doPublishDialogHelpButton()
>+    {
>+     var selectedTab = document.getElementById('publishDlgTabs').selectedItem;
>+     var selectedTabID = selectedTab.getAttribute('id');
>+     
>+     if(selectedTabID == "PublishTab")
>+       openHelp('comp-doc-publish-publishtab');
>+     else
>+       openHelp('comp-doc-publish-settingstab');
>+    }
You could use .id instead of getAttribute('id') which would save you a line, or alternatively just test to see whether the PublishTab is selected.

>   <tabbox id="TabBox">
>-    <tabs flex="1">
>+    <tabs id="publishDlgTabs" flex="1">
You don't need to add this id. Just use the selectedTab of the tabbox instead. (See doCertManagerHelpButton for instance, although it doesn't use .id, sigh.)

Comment 32

[Stefan [:stefanh]]

Attachment: New updated version

New version that uses .id instead of getAttribute('id') and SelectedTab in helpEditorOverlay.xul. I also choosed to keep the original copyright date in the license header, instead of adding another year (seems that this is the common way). Found one empty line (that I added accidentally) in extensions/help/resources/content/contents.rdf that I removed as well ;-). Transferring Neil's sr.

Comment 33

[Ian Neal]

Comment on attachment 264389 [details] [diff] [review]
New updated version

>--- /dev/null	Thu May 10 21:15:18 2007
>+++ suite/common/helpEditorOverlay.xul	Thu May 10 20:59:52 2007

>+     var selTab = document.getElementById('tabBox').selectedTab;
>+     
>+     if(selTab.id == "PublishTab")
Nit: space between if and ( please.

Comment 34

[Stefan [:stefanh]]

Landed with IanN's nit addressed:

Checking in suite/common/helpEditorOverlay.xul;
/cvsroot/mozilla/suite/common/helpEditorOverlay.xul,v  <--  helpEditorOverlay.xul
initial revision: 1.1
done
Checking in suite/common/jar.mn;
/cvsroot/mozilla/suite/common/jar.mn,v  <--  jar.mn
new revision: 1.20; previous revision: 1.19
done
Checking in editor/ui/locales/en-US/chrome/dialogs/EditorPublishProgress.dtd;
/cvsroot/mozilla/editor/ui/locales/en-US/chrome/dialogs/EditorPublishProgress.dtd,v  <--  EditorPublishProgress.dtd
new revision: 1.2; previous revision: 1.1
done
Checking in extensions/help/resources/jar.mn;
/cvsroot/mozilla/extensions/help/resources/jar.mn,v  <--  jar.mn
new revision: 1.74; previous revision: 1.73
done
Checking in extensions/help/resources/content/contents.rdf;
/cvsroot/mozilla/extensions/help/resources/content/contents.rdf,v  <--  contents.rdf
new revision: 1.26; previous revision: 1.25
done
Checking in editor/ui/dialogs/content/EdAdvancedEdit.js;
/cvsroot/mozilla/editor/ui/dialogs/content/EdAdvancedEdit.js,v  <--  EdAdvancedEdit.js
new revision: 1.38; previous revision: 1.37
done
Checking in editor/ui/dialogs/content/EdAdvancedEdit.xul;
/cvsroot/mozilla/editor/ui/dialogs/content/EdAdvancedEdit.xul,v  <--  EdAdvancedEdit.xul
new revision: 1.59; previous revision: 1.58
done

Comment 35

[Stefan [:stefanh]]

If anyone wonders I did checked in the rest (copy/paste error):

Checking in editor/ui/dialogs/content/EdImageMap.js;
/cvsroot/mozilla/editor/ui/dialogs/content/EdImageMap.js,v  <--  EdImageMap.js
new revision: 1.26; previous revision: 1.25
done
Checking in editor/ui/dialogs/content/EdImageMap.xul;
/cvsroot/mozilla/editor/ui/dialogs/content/EdImageMap.xul,v  <--  EdImageMap.xul
new revision: 1.37; previous revision: 1.36
done
Checking in editor/ui/dialogs/content/EdImageProps.js;
/cvsroot/mozilla/editor/ui/dialogs/content/EdImageProps.js,v  <--  EdImageProps.js
new revision: 1.105; previous revision: 1.104
done
Checking in editor/ui/dialogs/content/EdImageProps.xul;
/cvsroot/mozilla/editor/ui/dialogs/content/EdImageProps.xul,v  <--  EdImageProps.xul
new revision: 1.104; previous revision: 1.103
done
Checking in editor/ui/dialogs/content/EdLinkProps.js;
/cvsroot/mozilla/editor/ui/dialogs/content/EdLinkProps.js,v  <--  EdLinkProps.js
new revision: 1.81; previous revision: 1.80
done
Checking in editor/ui/dialogs/content/EdLinkProps.xul;
/cvsroot/mozilla/editor/ui/dialogs/content/EdLinkProps.xul,v  <--  EdLinkProps.xul
new revision: 1.83; previous revision: 1.82
done
Checking in editor/ui/dialogs/content/EdTableProps.js;
/cvsroot/mozilla/editor/ui/dialogs/content/EdTableProps.js,v  <--  EdTableProps.js
new revision: 1.83; previous revision: 1.82
done
Checking in editor/ui/dialogs/content/EdTableProps.xul;
/cvsroot/mozilla/editor/ui/dialogs/content/EdTableProps.xul,v  <--  EdTableProps.xul
new revision: 1.82; previous revision: 1.81
done
Checking in editor/ui/dialogs/content/EditorPublish.js;
/cvsroot/mozilla/editor/ui/dialogs/content/EditorPublish.js,v  <--  EditorPublish.js
new revision: 1.25; previous revision: 1.24
done
Checking in editor/ui/dialogs/content/EditorPublish.xul;
/cvsroot/mozilla/editor/ui/dialogs/content/EditorPublish.xul,v  <--  EditorPublish.xul
new revision: 1.24; previous revision: 1.23
done
Checking in editor/ui/dialogs/content/EditorPublishProgress.js;
/cvsroot/mozilla/editor/ui/dialogs/content/EditorPublishProgress.js,v  <--  EditorPublishProgress.js
new revision: 1.15; previous revision: 1.14
done
Checking in editor/ui/dialogs/content/EditorPublishProgress.xul;
/cvsroot/mozilla/editor/ui/dialogs/content/EditorPublishProgress.xul,v  <--  EditorPublishProgress.xul
new revision: 1.12; previous revision: 1.11
done
Checking in editor/ui/dialogs/content/EditorPublishSettings.js;
/cvsroot/mozilla/editor/ui/dialogs/content/EditorPublishSettings.js,v  <--  EditorPublishSettings.js
new revision: 1.17; previous revision: 1.16
done
Checking in editor/ui/dialogs/content/EditorPublishSettings.xul;
/cvsroot/mozilla/editor/ui/dialogs/content/EditorPublishSettings.xul,v  <--  EditorPublishSettings.xul
new revision: 1.12; previous revision: 1.11
done

Comment 36

[Stefan [:stefanh]]

Attachment: Fix for composer_help.xhtml

I forgot to include the changes in composer_help.xhtml in the latest patch.

Comment 37

[Stefan [:stefanh]]

Comment on attachment 264690 [details] [diff] [review]
Fix for composer_help.xhtml

From #seamonkey:

<IanN> stefanh: yes, r= for that

Comment 38

[Stefan [:stefanh]]

Checking in suite/locales/en-US/chrome/common/help/composer_help.xhtml;
/cvsroot/mozilla/suite/locales/en-US/chrome/common/help/composer_help.xhtml,v  <--  composer_help.xhtml
new revision: 1.39; previous revision: 1.38