643784 - Cannot open file from debugQATextEditorShell.xul

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect)

Description

[Ian Neal]

When you try an open a file from the file menu in debugQATextEditorShell.xul you just get the following messages in the error console:
Error: editPage is not defined
Source File: chrome://debugqa/content/debugQATextEditorShell.xul
Line: 1

Error: An error occurred executing the cmd_open command: [Exception... "'[JavaScript Error: "editPage is not defined" {file: "chrome://editor/content/ComposerCommands.js" line: 522}]' when calling method: [nsIControllerCommand::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 96"  data: yes]
Source File: chrome://global/content/globalOverlay.js
Line: 100

This is probably because it needs to include the script editorApplicationOverlay.js

Patch coming up.

Comment 1

[Ian Neal]

Attachment: Add editorApplicationOverlay.js script patch v1.0

Simply adds the required line to load the script.

Comment 2

[[e-mail]]

Hmm... the dialog defaults to opening HTML, and editPage opens an HTML Compose window anyway, is that what we want?

Comment 3

[Ian Neal]

Attachment: Get Text Editor working again patch v2.0

Changes since v1.0:
* Open now checks to see what type of editor and then presents a dialog with appropriate filters and passes editor type to SaveFilePickerDirectory function.
* Open now handles cancel from the dialog properly - try/catch no longer needed.
* Update editPage function to take an argument about what type of editor it is being used on or needs to look for and so it opens, if needed, the correct editor.
* Updated calls to editPage to remove obsoleted syntax from when editPage took window and delay arguments.
* Updated UpdateWindowTitle function so that it uses the filename instead of "untitled" for a text document.
* Store which editor (text or html) a page/file was opened in as well as title and url.
* Updated relevant functions to use this extra pref and store the type in recent pages menuitems so that it opens in same editor as it was opened in previously.

Comment 4

[[e-mail]]

Comment on attachment 522174 [details] [diff] [review]
Get Text Editor working again patch v2.0

>+    if (!IsHTMLEditor()) {
Nit: Might as well use if (IsHTMLEditor()) and swap the blocks around.

>+      title = GetString("OpenTextFile");
>+      fileType = "text";
>+    }
>+    else {
>+      title = GetString("OpenHTMLFile");
>+      fileType = "html";
Although I was wondering whether it was worth using
var fileType = IsHTMLEditor() ? "html" : "text";
var title = GetString(IsHTMLEditor() ? "OpenHTMLFile" : "OpenTextFile");

>-    fp.init(window, GetString("OpenHTMLFile"), nsIFilePicker.modeOpen);
>-
>-    SetFilePickerDirectory(fp, "html");
>-
>-    // When loading into Composer, direct user to prefer HTML files and text files,
>-    //   so we call separately to control the order of the filter list
>-    fp.appendFilters(nsIFilePicker.filterHTML);
>+    fp.init(window, title, nsIFilePicker.modeOpen);
>+
>+    SetFilePickerDirectory(fp, fileType);
>+
>+    // Direct user to prefer HTML files and/or text files depending on whether
>+    // loading into Composer or Text editor, so we call separately to control
>+    // the order of the filter list.
>+    if (fileType == "html")
>+      fp.appendFilters(nsIFilePicker.filterHTML);
[Huh, your diff program made a meal of this, mine does -+ -+ ---+++++]

>-        editPage(element.href, window, false);
>+        editPage(element.href);
Oh dear, that was my fault in bug 332668 for not checking all callers :-(

>+      var fileType = !IsHTMLEditor() ? "text" : "html";
Nit: IsHTMLEditor() ? "html" : "text" again please.

>       var title = GetUnicharPref("editor.history_title_"+i);
>       titleArray.push(title);
>       urlArray.push(url);
>+      var fileType = GetUnicharPref("editor.history_type_" + i);
>+      typeArray.push(fileType);
Nit: please group the GetUnicharPref statements together.

>+  // aFileType is optional and, if not present, need to set it to be html.
"optional and needs to default to html"?

>+    var enumerator = windowManagerInterface.getEnumerator("composer:" + aFileType);
This is my favourite line of the whole patch :-)

>+    var chromeURL = aFileType == "html" ?
>+        "chrome://editor/content" :
>+        "chrome://debugqa/content/debugQATextEditorShell.xul";
This is my least favourite line of the whole patch, since it means that disabling or uninstalling debug/QA breaks editor. The best idea I could come up with is to have a function in editorApplicationOverlay.js that always returns chrome://editor/content/ and gets redefined by one in debugQAEditorOverlay.js(?) to return the appropriate chrome URL.

>-<window id="main-window"
>+<window id="editorWindow"
This is so we get the right icon?

Comment 5

[Ian Neal]

Attachment: Get Text Editor working again patch v2.1

Changes since v2.1:
* Switched from !IsHTMLEditor to IsHTMLEditor as suggested.
* Grouped the GetUnicharPref statements together as suggested.
* Revised comment as suggested.
* Moved JS from editorTasksOverlay.xul into editorApplicationOverlay.js so it can be used by editPage.
* Tweaked EditorNewPlaintext and NewEditorWindow to take aUrl and aCharsetArg arguments.
* Made editPage test for presence of "EditorNewPlaintext" and use it for text if present otherwise use NewEditorWindow function.
* Pass title to SaveRecentFilesPrefs function so that text files can have a title saved for them too.

Comment 6

[[e-mail]]

(In reply to comment #5)
> * Moved JS from editorTasksOverlay.xul into editorApplicationOverlay.js so it
> can be used by editPage.
But now you're reading editorApplicationOverlay.js into all windows that use editorTasksOverlay.xul, which is basically all windows, in which case you now need to remove the individual references to editorApplicationOverlay.js

> * Made editPage test for presence of "EditorNewPlaintext" and use it for text
> if present otherwise use NewEditorWindow function.
Nice :-)

> * Pass title to SaveRecentFilesPrefs function so that text files can have a
> title saved for them too.
I haven't tried this out yet, but I'm not sure this is a good idea.

Comment 7

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #5)
> > * Pass title to SaveRecentFilesPrefs function so that text files can have a
> > title saved for them too.
> I haven't tried this out yet, but I'm not sure this is a good idea.
I'm still not convinced that this is useful. What might work is if the URL is a tooltip/statustip rather than being part of the label (as per Bookmarks).

Comment 8

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #5)
> > * Pass title to SaveRecentFilesPrefs function so that text files can have a
> > title saved for them too.
> I haven't tried this out yet, but I'm not sure this is a good idea.
I need to try this with a non-ASCII file name. I didn't think of that before.

Comment 9

[[e-mail]]

Comment on attachment 522241 [details] [diff] [review]
Get Text Editor working again patch v2.1

Sorry, I just don't see the point of that duplicate file name.

(You might want to land that string to beat the l10n freeze though.)

Comment 10

[Ian Neal]

(In reply to comment #9)
> Comment on attachment 522241 [details] [diff] [review]
> Get Text Editor working again patch v2.1
> 
> Sorry, I just don't see the point of that duplicate file name.
> 
> (You might want to land that string to beat the l10n freeze though.)

Landed string as http://hg.mozilla.org/comm-central/rev/5c719af5c0ea

Comment 11

[Ian Neal]

Attachment: Get Text Editor working again patch v2.2 [Checked in: Comment 12]

Changes since v2.1
* Reverted use of filename for when there was no document title.
* As suggested, added tooltip of url for menuitems (as status field is used for tags in composer, cannot use statustip).

Comment 12

[Ian Neal]

Comment on attachment 523902 [details] [diff] [review]
Get Text Editor working again patch v2.2 [Checked in: Comment 12]

http://hg.mozilla.org/comm-central/rev/a52cfb552cfd