122992 - Remove editorshell dependencies from editor.js

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect)

Description

[Kathleen :Brade]

We need to get rid of editorshell's editor type string.
Instead, people should be using the editorshell's editor's flags.

Right now, there are bugs in various places in the code where we assume "html"
is only Composer but that isn't true.  Hopefully we can get this all
straightened out by removing the editortype.

Comment 1

[Akkana Peck]

Not going to make 0.9.9.

Comment 2

[Akkana Peck]

Attachment: Partial patch

Here's a start at the editor part of this.  I've gone through editor.js and
excised editorShell references where I could, except in cases where I knew
Charley was more familiar with the code.  Searching for "editorshell" (case
insensitive) in editor.js will get all the cases where I'm hoping to get help.

This version does work, but is still dependant on the editor shell.

I've also cleaned up the API for nsIEditor to make it more JS-friendly, and
fixed some warnings.

Comment 3

[Kathleen :Brade]

Comment on attachment 71579 [details] [diff] [review]
Partial patch

Thanks for cleaning up nsIEditor.idl; I've been wanting to do that for so long!

* you have an extra space here:  readonly attribute  nsIDOMElement rootElement;

* I'd say get rid of this line and the associated comment; we can always add it
back later if we want it:
+  //readonly attribute nsIPresShell presShell;

* I would probably put the selection line closer to selectionController; at a
minimum probably after the Init/Shutdown.

* Do we really need both selection and selectioncontroller? (probably need a
bug for just this issue?)

* not sure if you intended the extra space after nsIPlaintextEditor =

* I'm worried about some of the QI's failing and throwing errors; I wish our JS
were more solid and had try/catches in key places :-/

* in editor.js, in this block, you added a blank line with spaces:
@@ -324,22 +342,23 @@

* CheckOpenWindowForURLMatch
is pretty important to get working since it could cause users to open the same
file in two different windows and cause dataloss.

* for consistency (?), I'd add a space after the if on these lines (since you
are touching them):
+  if(editor)
+  if(editor.documentModified && !editor.documentIsEmpty)

* I wonder if we should cache the html editor?

* in JS, doing a QI will throw an error rather than return null so this piece
should be fixed:
+  var htmlEditor = editor.QueryInterface(Components.interfaces.nsIHTMLEditor);
+  if (!htmlEditor)
+    return;

and

+  var htmlEditor = editor.QueryInterface(Components.interfaces.nsIHTMLEditor);
+  if (htmlEditor)

and

+  var htmlEditor = editor.QueryInterface(Components.interfaces.nsIHTMLEditor);
+  if (htmlEditor)


and other places I'm sure you can find without me pasting them into this tiny
edit field

* typo on these lines:
+  if (!popup || !editor || !editor.ocument)
+  var curTitle = editor.ocument.title;

Comment 4

[Akkana Peck]

> Do we really need both selection and selectioncontroller?

nsEditorCommands uses the selection controller (gotten through the nsIEditor
interface) for motion commands (e.g. delete line).  nsISelection doesn't have
any way to get a selection controller (and probably shouldn't).  So we'd need to
come up with a way around that.  We could make editor users get the selection
via the selection controller, but that complicates getting the selection, which
should be a straightforward thing.  I'm not sure of the best way around it.

Charley also wondered about cacheing the html editor.  I was inclined against it
but don't feel strongly about it -- I'll go with whatever Kathy and Charley
think best.

Thanks for the other suggestions, Kathy -- I'll make a new patch incorporating
all of them.

Comment 5

[Akkana Peck]

Charley and I both doubt that we can entirely excise the editorshell in time for
1.0, and suspect that the risk of trying might be unacceptable.  Since the
important parts (make nsIEditor available from the xul object, clean up the
editor idl files, and put comments in a few places making it clear that
nsIEditorShell is deprecated) I'm un-plussing this bug and proposing punting it
to post-1.0.

Comment 6

[Akkana Peck]

Bumping milestone.

Comment 7

[Akkana Peck]

Changing summary to make it clearer: this bug is about removing all editorshell
dependencies from the editor, so that layout's nsEditorBoxObject can go through
the editor embedding API and never have to know about the editorshell.

Comment 8

[Akkana Peck]

Changing milestone to reflect realities of bug 122922.

Comment 9

[Akkana Peck]

Hwaara offered to help with reviving this.  Updated patch coming soon.

Comment 10

[Akkana Peck]

Attachment: Updated patch

This is the editor.js changes, updated to take into account all the new
editorshell code that has been added since the patch was made (sigh), plus the
removal of the ifdefed-out from nsEditorRegistration (unchanged).

The remaining editor shell instances in this file are:
- Register/Unregister document state listener.
- Initialization issues related to editorType (personally, I'd like to see
editorType go away altogether; but mail and perhaps other users use it, so I'm
leaving it for this round.
- LoadURL (we need the docshell, or the window so we can get the docshell, for
that.  Maybe there's a way to do it through the new embedding API?)
- RebuildDocumentFromSource (probably related to loadURL?)
- SetDocumentTitle (editorshell's version of this calls
UpdateWindowTitleAndRecentMenu(); we need to find some other way of doing
that).
- SetDisplayMode
- DisplayParagraphMarks (this looks like it'll be easy to translate into JS, I
just haven't done it yet).
- The tricky stuff in SwitchInsertCharToThisWindow (that's probably a matter of
making the dialog code work with an editor rather than an editorshell).

The doc state listeners and loadurl are probably the biggies.

Comment 11

[Håkan Waara]

Comment on attachment 88060 [details] [diff] [review]
Updated patch

>@@ -438,11 +447,11 @@
> function EditorResetFontAndColorAttributes()
> {
>   document.getElementById("cmd_fontFace").setAttribute("state", "");
>-  window.editorShell.RemoveTextProperty("font", "color");
>-  window.editorShell.RemoveTextProperty("font", "bgcolor");
>-  window.editorShell.RemoveTextProperty("font", "size");
>-  window.editorShell.RemoveTextProperty("small", "");
>-  window.editorShell.RemoveTextProperty("big", "");
>+  gEditor.RemoveInlineProperty("font", "color");
>+  gEditor.RemoveInlineProperty("font", "bgcolor");
>+  gEditor.RemoveInlineProperty("font", "size");
>+  gEditor.RemoveInlineProperty("small", "");
>+  gEditor.RemoveInlineProperty("big", "");

Are RemoveInlineProperty() and RemoveTextProperty() completely interchangable?

>+function GetAtomService()
>+{
>+    gAtomService = Components.classes["@mozilla.org/atom-service;1"].getService().QueryInterface(Components.interfaces.nsIAtomService);

This is the
Components.classes["@mozilla.org/atom-service;1"].getService(Components.interfa
ces.nsIAtomService);

This patch looks great. It removes a lot of editorshell cruft and hardcoding of
document `types' in favor of checking for availability of the functions by
using COMish approaches like failing on QI.

Seeing as much of the editor.js code `depend' on HTML and plaintext (and many
other places that appear they should be `generic' and not depend on any one
format), I would personally like to see the format-specific code factored out
at some point in the future. If factorization of type and file specific formats
can be made, and the other base code handling these components can be truly
generic, it will be much easier to add new formats (or even plugins?) to
editor. Is this also the goal?

I will apply the patch and run with it later today or tonight.

Comment 12

[Akkana Peck]

Attachment: Update

It's probably better to call EditorRemoveTextProperty instead of
gEditor.RemoveInlineProperty for those calls anyway.

In this new patch, I've made that change, fix some gEditor.Set lines that
should be .set, and made the getService change you suggested.

I heartily agree with you about factoring html from plaintext code; that would
be much better and we should try to move in that direction.  Charley, is there
any argument against separating the html-specific code into a new file, say,
htmlEditor.js?	(I don't think that should be part of this bug, but I could
file a new bug to track that issue.)

Comment 13

[Charles Manske]

Sure. Factoring into separate htmlEditor.js sounds good. We should do this and 
the complete overhaul of plaintext editing in next release cycle.

Comment 14

[Håkan Waara]

That sounds great. And it's a job, probably so big that you need to do it in
steps...as a work in progress. I filed bug 152649 for this work, and made it a
tracking bug for the whole modularization process.

Comment 15

[Akkana Peck]

Cc Simon.

Comment 16

[Kathleen :Brade]

Comment on attachment 88148 [details] [diff] [review]
Update

Please reverse these lines:
+  var flags = gEditor.flags;
+  if (!gEditor) return false;


We need to ensure (via try/catch) that BeginTransaction calls are paired with
EndTransaction calls.


We should try to clean up places where we check for null for QI failure:
 - add a try/catch or global:
@@ -105,7 +106,7 @@ 
@@ -127,7 +128,7 @@ ***
@@ -181,13 +182,13 @@
@@ -585,7 +597,12 @@
EditorGetTextProperty
EditorSetTextProperty
EditorRemoveTextProperty
@@ -1022,7 +1067,10 @@ ***
@@ -1047,13 +1095,14 @@
@@ -1085,7 +1134,9 @@
@@ -1223,16 +1274,17 @@ (at the end)
@@ -1340,7 +1392,7 @@
@@ -1360,20 +1412,25 @@
@@ -1405,9 +1462,10 @@
@@ -1562,7 +1621,8 @@
@@ -1847,7 +1905,8 @@
@@ -1925,7 +1984,8 @@ **
@@ -1949,12 +2009,13 @@ **
@@ -1978,10 +2039,11 @@ **
@@ -2351,8 +2413,17 @@
@@ -2409,8 +2480,9 @@ ***
@@ -2441,7 +2513,10 @@
@@ -2452,11 +2527,21 @@ **
IsInTable
IsInTableCell
@@ -2522,8 +2616,11 @@ **
@@ -2559,7 +2656,10 @@ **
GetNumberOfContiguousSelectedRows **
GetNumberOfContiguousSelectedColumns **
@@ -2716,16 +2822,16 @@

** may not need to do anything?
*** may need more than a simple wrapping of try/catch or use of global

Comment 17

[Kathleen :Brade]

Comment on attachment 88148 [details] [diff] [review]
Update

Akkana--can you update this patch since I touched a lot of code in this area
last week?

Comment 18

[Akkana Peck]

Attachment: Update (after Kathy's checkin)

Kathy checked in a lot of the proposed changes in editor.js already, so the
remaining changes are pretty small.

This gets rid of editorshell usage in editor.js with two exceptions:
1. Any code surrounding creation of the window (that's going to get rewritten
anyway when we have the true embedding API).

2. SetDocumentTitle: the editorshell version is a long routine that does things
like set the window titlebar.  It's probably not appropriate for the editor to
be doing this sort of thing.  What to do?

3. SetDisplayMode -- we should probably rethink how this works.

4. DisplayParagraphMarks: there's no editor equivalent yet.  The logic is easy,
but it requires access to nsEditorShell's protected member
mParagraphMarksStyleSheet, which isn't exposed with a getter.

5. SwitchInsertCharToAnotherEditorOrClose: this has code where it loops over
all windows and tries to find one with an editor -- do we have another way to
do that?  Should we hang the editor rather than the editorshell off the window?
 It's probably not safe to do that until the dialogs are cleaned up, and
there's an enormous amount of editorshell usage in our dialog code.

Comment 19

[Charles Manske]

2. SetDocumentTitle is a tricky issue! Composer definitely benefits by the 
extra information we supply in the title and I don't think all the required
methods are scripted.
3. I've thought about SetDisplayMode(). The issue is that the current Editor
style sheet interface uses a non-scripted interface (nsICSSStyleSheet). I think
the best solution is to rewrite nsIEditorStyleSheet methods to replace this 
param with the URL strings for style sheet and let the editor store arrays of 
the nsICSSStyleSheet pointers.
5. I think "SwitchInsertCharToAnotherEditorOrClose" can be rewritten in JS. Or
I've been considering throwing out the whole "window tracking" behavior for the 
Insert Character dialog and simply making it modal to to each editor.

Comment 20

[Kathleen :Brade]

Comment on attachment 91561 [details] [diff] [review]
Update (after Kathy's checkin)

the changes for nsEditorRegistration.cpp have r=brade

In editor.js, I don't see where we register the DocumentStateListener; does
double-click invoke the properties dialogs?

Comment 21

[Akkana Peck]

Prior to the patch, EditorStartup registered DocumentStateListener, and
EditorSharedStartup (called by EditorStartup) registered
MessageComposeDocumentStateListener or DocumentStateListener for the html case,
DocumentStateListener for the text case, nothing for the textmail or default
cases.  I was trying to get rid of the redundancy.  But perhaps I should have
also have added 
    editorShell.RegisterDocumentStateListener( DocumentStateListener );
for the textmail and default cases.  I'll do that and attach an update, which
also cleans up this logic a bit.

Comment 22

[Akkana Peck]

Attachment: Clean up logic near the doc listener registration

Comment 23

[Kathleen :Brade]

I don't think this is correct:
+  if (editorShell.editorType == "htmlmail")
+    editorShell.RegisterDocumentStateListener(
MessageComposeDocumentStateListener );

What is currently happening (as discovered by blocker bug last week) is that
text mail is calling into Composer's initialization stuff when it should be
calling MessageComposeDocumentStateListener.

The problem actually isn't in the JS, it's in nsEditorShell.cpp where "textmail"
is almost immediately remapped to "text" type.  We need to define the "types"
outside of editorshell.  However, this can all wait.  

My only concern with this patch is that it should be written to use "textmail"
(even though it won't fix the bug) so the logic can be more easily converted to
whatever we convert it to down the road.  I expect something like this:
>+  if (editorShell.editorType == "htmlmail"
>+   || editorShell.editorType == "textmail")

Comment 24

[Akkana Peck]

Attachment: Use mail's document state listener for textmail as well as htmlmail

Comment 25

[Kathleen :Brade]

Comment on attachment 92259 [details] [diff] [review]
Use mail's document state listener for textmail as well as htmlmail

r=brade

Comment 26

[Akkana Peck]

Simon, could you please sr this?  Thanks!

Comment 27

[Simon Fraser [no longer active]]

Comment on attachment 92259 [details] [diff] [review]
Use mail's document state listener for textmail as well as htmlmail

sr=sfraser

Comment 28

[Akkana Peck]

Checked in!

This bug has always been tracking editor.js, not the whole editor ... I'm going
to mark it as fixed since it has gotten so long, and since we have other bugs
tracking other parts of the editorshell removal.