157097 - embedding: command nodes hold state; Rewrite commands to use nsICommandParams

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Kathleen :Brade]

In order for editor embedding to be complete, we need to complete this task:
 * Command nodes are currently used as placeholders for holding state; they need
to be moved to the new API

Comment 1

[mjudge]

this is the same work as 121648. You have to get rid of these temporary nodes 
to call directly to the nsIEditor methods. I am marking this a dup.

*** This bug has been marked as a duplicate of 121648 ***

Comment 2

[Akkana Peck]

This clearly isn't the same thing as editorshell removal, though it is a
necessary step before the editorshell can be removed.  Reopening and adding
dependency.

Comment 3

[mjudge]

ok

Comment 4

[mjudge]

*** Bug 157114 has been marked as a duplicate of this bug. ***

Comment 5

[Charles Manske]

Clarifying summary and reassigning

Comment 6

[Kathleen :Brade]

Attachment: patch to remove state knowledge from C++ commands

This is a first pass at removing the state knowledge from commands.  The state
is still in xul and is still used.  Editorshell usage has been removed as well
(commands are Init'd with editor).

Some of the UI updating will need to be redone to use command groups and
observers (as part of another bug).

Akkana--do we have a bug to remove nsInterfaceState?  That file should be
removed soon as well (we will use nsComposerCommandsUpdater.cpp).  This is
dependent on our usage of nsEditingSession (iirc).

Comment 7

[Kathleen :Brade]

Attachment: part missing from above patch

I inadvertently left out this diff which is required with the above changes;
sorry!

Comment 8

[Kathleen :Brade]

reviews?

Comment 9

[Charles Manske]

Comment on attachment 98144 [details] [diff] [review]
patch to remove state knowledge from C++ commands

r=cmanske
I've seen a problem in setting the font color swatch in the toolbar when window
is first created.
using:
<command id="cmd_fontColor"	  state="x"/>
on line 217 of editorOverlay.xul fixes that.

Comment 10

[Charles Manske]

Comment on attachment 98162 [details] [diff] [review]
part missing from above patch

r=cmanske
I've applied both of these patches and tested them. Except for font color
issues mentioned above, setting attributes works and their states in the
toolbar or menu shows correctly.

Comment 11

[Charles Manske]

One issue I see is that first call to "goUpdateComposerCommands" when the
window is created fails at "cmdController.getCommandStateWithParams" on line
340 of ComposerCommands.js. I'm using Windows 2K and Kathy doesn't see these
dumps in Mac. This is some timing issue? That's probably the cause of the 
font color not initializing correctly upon startup when color is "default" (state
is empty string)

Comment 12

[Charles Manske]

Attachment: New patch for nsComposerController.cpp

This patch includes same changes as "patch to remove state knowledge..."
plus an important improvement: Remove the double creation of "mCommandManager"
in nsComposerController::Init(). This was causing 4 calls to 
nsComposerController::RegisterComposerCommands, twice for each of the 
nsIComposerController instances that Composer creates.
With this fix, it is called only once.

Comment 13

[Charles Manske]

I did some thorough debugging of the Composer Controller startup code, which 
resulted in "New patch for nsComposerController.cpp". It also revealed the 
problem causing the assertions noted in comment #11:
The window creation via editorOverlay.xul triggers the 
goUpdateComposerMenuItems() calls for each commandset in the overlay, and this 
happens before the "on document loaded" callback in editor.js:
NotifyDocumentCreated() -> DoCommandControllerSetting()
which sets the refcon on the nsIEditorController. 
Thus nsComposerController::GetCommandStateWithParams() fails and asserts.
This shouldn't be considered a problem because the code in
DoCommandControllerSetting() should be temporary until all the controller 
initialization is moved from nsIEditorShell to nsIEditingSession.
[A short term fix would be to save the 2nd composer controller as a member
of nsIEditorShell and set the refcon in nsEditorShell::PrepareDocumentForEditing
just as we do for the first controller in mEditorController,
but it hardly seems worth making those changes in nsEditorShell now.]

Comment 14

[Simon Fraser [no longer active]]

+  // This will create mCommandManager and register commands if not already done.
   rv = GetComposerCommandManager(getter_AddRefs(mCommandManager));  
   if (NS_FAILED(rv)) return rv;  
 
   mCommandRefCon = aCommandRefCon;     // no addref  
   
-  mCommandManager = do_CreateInstance(NS_CONTROLLERCOMMANDMANAGER_CONTRACTID, &rv);
-  if (NS_FAILED(rv)) return rv;
-
-  // register the commands.
-  rv = nsComposerController::RegisterComposerCommands(mCommandManager);
-  if (NS_FAILED(rv)) return rv;
-

As we've discussed in email, this change moves you into a world where you
commands (including JS commands) _have_ to be stateless. I.e. no holding onto
XUL nodes.

Thinking about this more, it may be dangerous, or impossible, to have JS
commands registered on a singleton nsControllerCommandManager. Those JS commands
run in the context of a window. So we might not be able to do this until all
commands on this CCM are in C++.

-NS_IMETHODIMP nsComposerController::GetCommandState(const char *aCommand,
nsICommandParams *aCommandParams)
+/* void getCommandStateWithParams (in DOMString aCommandName, inout
nsICommandParams aCommandParams); */
+NS_IMETHODIMP nsComposerController::GetCommandStateWithParams(const char
*aCommand, nsICommandParams *aCommandParams)
 {
   if (!mCommandRefCon || !mCommandManager)
     return NS_ERROR_NOT_INITIALIZED;
   return mCommandManager->GetCommandState(aCommand,aCommandParams,mCommandRefCon);
 }
 
-/* void doCommand (in DOMString aCommandName, in nsICommandParams
aCommandParams); */
-NS_IMETHODIMP nsComposerController::DoCommand(const char *aCommand,
nsICommandParams *aCommandParams)
+/* void doCommandWithParams (in DOMString aCommandName, in nsICommandParams
aCommandParams); */
+NS_IMETHODIMP nsComposerController::DoCommandWithParams(const char *aCommand,
nsICommandParams *aCommandParams)

You need some idl changes to go along with this, right?

Comment 15

[Simon Fraser [no longer active]]

Last comments were for the last attachment.

Comments on attachment 98144 [details] [diff] [review]:

+    nsString aListType(nsnull);
+    rv = editor->RemoveList(aListType);

Did you want an empty string? Just use RemoveList(nsString());

+    *outCmdEnabled = PR_TRUE;     // can always indent (I guess)

Don't we have some max indent level?

-  nsresult    GetInterfaceNode(const nsAString & nodeID, nsIEditorShell*
editorShell, nsIDOMElement **outNode);
+  nsresult    GetInterfaceNode(const nsAString & nodeID, nsIEditor* aEditor,
nsIDOMElement **outNode);

GetInterfaceNode should disappear eventually, since it's use to get at XUL nodes.

+function noUIStateUpdateNeeded(commandID)
+{
+  if (commandID == "cmd_removeNamedAnchors"
+   || commandID == "cmd_removeLinks"

Wow, that's a lot of string compares ;)

+  // these commands need to go somewhere:
+  if (command == "cmd_smiley"
+   || command == "cmd_dt"
+   || command == "cmd_dd"
+   || command == "cmd_removeList"
+   || command == "cmd_cut"
+   || command == "cmd_copy"
+   || command == "cmd_paste"
+   || command == "cmd_pasteQuote"
+   || command == "cmd_delete"
+   || command == "cmd_selectAll"
+   || command == "cmd_redo"
+   || command == "cmd_undo")
+     return;

Some of those are editor controller commands, which need to be moved to using
command params.

+        cmdController.doCommandWithParams(command, params);
+        if (params)
+          cmdController.getCommandStateWithParams(command, params);

Why the call to cmdController.getCommandStateWithParams here? Was that just for
testing?

+function doStatefulCommand(commandID, newState)
...
+    goDoCommandParams(commandID, cmdParams);
+
+    pokeMultiStateUI(commandID, cmdParams);

That call to pokeMultiStateUI shouldn't really be needed, if the command
observer and update stuff is hooked up, since you should get pinged by the
backend that the command needs updating as the command executes.


     gEditor = editorShell.editor;
 
+    DoCommandControllerSetting(gEditor);
+

I'm not clear why you need this, and, even if you do, shoudn't it happen in C++?

Comment 16

[Akkana Peck]

Comment on attachment 98144 [details] [diff] [review]
patch to remove state knowledge from C++ commands

Index: editor/ui/composer/content/editor.js
===================================================================
+  // count down to find a controller that supplies a
nsIControllerCommandManager interface
+  for (var i = numControllers-1; i >= 0 ; i --)

Does this rely on the editor being the only component which uses the new
command types?	Will there be a problem if we ever persuade browser or
mail to use the same command structure?

Does mail compose need additional changes?  I tried running mozilla -compose
with these changes, and got lots of JS strict warnings on reference to
undefined property args.*, and I when I clicked and tried to type in the
compose area I got a lot of:
pldhash: for the table at address 0x87bb95c, the given entrySize of 44 probably
favors chaining over double hashing.
and no cursor.	I get that same warning when I run -edit, but I get a caret and
can type.

Whoops, that was just the first time.  On subsequent attempts, I can type,
though I still get huge numbers of the pdlhash and JS strict problems, and an
occasional 
###!!! ASSERTION: nsVoidArray::ElementAt(index past end array) - note on bug
96108: 'aIndex < Count()', file ../../../dist/include/xpcom/nsVoidArray.h, line
78
Break: at file ../../../dist/include/xpcom/nsVoidArray.h, line 78

Probably these aren't even related to Kathy's changes ...  I'll do more
investigation.

Comment 17

[Akkana Peck]

If I back out the patches, I no longer see the pldhash messages, but I still get
the JS strict warnings from MsgComposeCommands.js when I run -compose.  So those
aren't new with this patch, but the pldhash messages probably are.

Comment 18

[Akkana Peck]

Attachment: All three diffs together, diffed from the top

I had trouble applying the patches because the filenames all came from
different levels of the tree.  So here's one big patch from the mozilla/ level
containing all of Kathy's changes.

Comment 19

[Kathleen :Brade]

from sfraser in comment 14:
>You need some idl changes to go along with this, right?
Yes, there are corresponding changes to nsIController.idl for those.  You
probably saw those in the original patch once you reviewed it.

from sfraser in comment 15:
>Did you want an empty string? Just use RemoveList(nsString());
That code was copy/pasted from DoCommand, I've changed it now to just call
DoCommand from DoCommandParams and fixed this issue in DoCommand.

>Don't we have some max indent level?
No, we don't, you'll notice that that particular line is changed only for
consistency in indenting (whitespace only change).  I think there is already a
bug on this issue.

>GetInterfaceNode should disappear eventually, ...
Good catch, it was actually removed from C++ but I missed the header. 
GetCommandNodeState and SetCommandNodeState have also been removed.

>Wow, that's a lot of string compares ;)
yep ;-)

>Why the call to cmdController.getCommandStateWithParams here? Was that just 
>for testing?
No, it's actually needed for updating the UI after invoking the command.  This
will all change when we move to observers so I didn't spend much time making the
code pretty.  Since goDoCommandParams already had the controller and the params
I added it there for efficiency.  I've added a comment that says that call
should be remoevd when we use observers.

>That call to pokeMultiStateUI shouldn't really be needed, if the command
>observer and update stuff is hooked up, since you should get pinged by the
>backend that the command needs updating as the command executes.
Yes, that's true, the poking of UI needs to be done but we'll do that in a
separate patch/bug.

>>+    DoCommandControllerSetting(gEditor);
>I'm not clear why you need this, and, even if you do, shoudn't it happen in >C++?
This is the first point where we know we have the editor (as opposed to the
editorShell).  Once we've moved to nsIEditingSession, this will change (and be
in C++).


from akkana in comment 16:
>Does this rely on the editor being the only component which uses the new
>command types?
No, we QI to editorController so I'm not too concerned with problems in browser
or mail.  But as Charley indicated yesterday, there is a problem with
distinguishing the editor controller (cut/copy/paste/etc.) from the composer
controller.  DoCommandControllerSetting() as well as GetEditorController() will
be in need some work to address that issue.

from akkana in comment 16 and comment 17:
>... I still get the JS strict warnings from MsgComposeCommands.js when I run
>-compose.  So those aren't new with this patch, but the pldhash messages
>probably are.
Correct.  I asked Simon about the pldhash messages and he said that they could /
should be ignored.  I believe the assertion about "nsVoidArray::ElementAt(index
past end array)" is also from this patch but I'm not 100% sure.

Comment 20

[Akkana Peck]

> I asked Simon about the pldhash messages and he said that they could /
> should be ignored.

Can we please remove them or make them #ifdef DEBUG_someonewhocares as part of
this patch, then?  They're a huge amount of output (like, 100 lines just from
typing a few chars in composer) and swamp all other output, not to mention
slowing down typing noticably.

Comment 21

[Simon Fraser [no longer active]]

Brendan put in that pldhash stuff.

Comment 22

[Kathleen :Brade]

Attachment: combined patch without the controller change from cmanske

Here is the updated patch with added comments, etc from comments 14-17.  It
does not include cmanske's patch from above.

Comment 23

[Charles Manske]

Comment on attachment 98759 [details] [diff] [review]
combined patch without the controller change from cmanske

r=cmanske

Comment 24

[Brendan Eich [:brendan]]

sfraser: why do you think those pldhash.c DEBUG messages should be ignored? 
What's the point of using double hashing if it wastes space compared to using
plhash.[ch].  If the code you're using is several layers above the pldhash
usage, please point me at the layer of pldhash -- that's the one that should
probably use plhash.c.

Yeesh, doesn't anyone think about the content of a DEBUG message?  Has that coin
been so debased?  I didn't write those messages for my own amusement, or just to
annoy people with meaningless drivel.

/be

Comment 25

[Simon Fraser [no longer active]]

akk: can you get a stack for one of those hash debug statements? I think they
are probably coming from an nsSupportsHashtable, but I'm not sure.

Comment 26

[Charles Manske]

Attachment: Addition patch to set refCon on Composer controllers

I suggest the changes in this patch to do:
1. Store temporary nsIEditorController ptrs for both CommandControllers created

in C++ in nsEditorShell.cpp. Then set the refCon for both of them just as we
currently do only for the first one (nsIEditorController), but not the 2nd
nsComposerController. This is what Simon suggested at the end of comment #16.
Now this is all temporary, of course, but similar code will need to be 
implemented in nsEditingSession, so we should do it correctly now in 
editorShell.

2. There is a 2nd nsComposerController that is created in by 
SetupComposerWindowCommands() in ComposerCommands.js. This is attached to the 
toplevel "window", not "window._content" and is needed for command processing
when in HTML Source mode. It is *this* controller that needs its refCon set
in editor.js in what I have renamed as "DoWindowCommandControllerSetting()".
Without this patch, that command controller never has the refcon set correctly.

That is why we see the flurry of exception errors in the debug window after 
applying the "combined patch...".

Comment 27

[Charles Manske]

Note that the changes in editor.js in my "Addition patch..." are the same as 
brade's "combined patch..." except for:
1. Renaming "DoCommandControllerSetting" to "DoWindowCommandControllerSetting"
2. Using "window" instead of "window._content" for the "controllers" property.
3. Removing the call from the Message Composer's "NotifyDocumentCreated" method,
since that window never installed that 2nd ComposerController.
Again note that this is still temporary code; the 2nd nsComposerController 
needs to be replaced by a new class that does automatically register all the
HTML formating commands as nsComposerController now does.

Comment 28

[Brendan Eich [:brendan]]

Re comment #25, nsSupportsHashtable is based on plhash, not on pldhash.

/be

Comment 29

[Akkana Peck]

Comment on attachment 98801 [details] [diff] [review]
Addition patch to set refCon on Composer controllers

> +,  mEditorController1(nsnull)
> +,  mEditorController2(nsnull)

Can you rename these to something more descriptive, like mEditorController and
mComposerController or mHTMLEditorController?

> -    nsCOMPtr<nsISupports> shellAsISupports = do_QueryInterface((nsIEditorShell*)this);
> +    // nsCOMPtr<nsISupports> shellAsISupports = do_QueryInterface((nsIEditorShell*)this);

Please remove rather than commenting out, unless there's some reason it's
likely to come back (in which case add a comment to that effect).

With those changes, r=akkana.

Comment 30

[Charles Manske]

Attachment: Additional patch to set refCon on Composer controllers v2

Use "mEditorController" and "mComposerController" instead of
"nsEditorController1"
and "nsEditorController2"
delete some cruft.

Comment 31

[Kathleen :Brade]

Comment on attachment 98806 [details] [diff] [review]
Additional patch to set refCon on Composer controllers v2

r=brade I'm ok with these changes (though I hate to add to editorshell).  I
have incorporated them into my stuff and added a few comments.

note also that calling DoWindowCommandControllerSetting doesn't really change
anything since the refcon isn't currently used by any of those commands.

Comment 32

[Simon Fraser [no longer active]]

Comment on attachment 98759 [details] [diff] [review]
combined patch without the controller change from cmanske

sr=sfraser

Comment 33

[Kathleen :Brade]

the above patches have been checked in
future / remaining work will be covered in a new bug

Comment 34

[sairuh (rarely reading bugmail)]

rs vrfy (see comment 33 ;)