184718 - midas needs a command for setting html/css creation type

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Kathleen :Brade]

Midas needs a command for setting html/css creation type

Comment 1

[Kathleen :Brade]

Charley reminds me of these things:
>be sure to implement it as another "nsSetDocumentStateCommand" command if it
>supports setting a state param. and use nsDocumentStateCommand if it simply
>reports state

I would guess that this is setting a state rather than reporting a state.

Comment 2

[Kathleen :Brade]

Attachment: one way of handling boolean parameters (in Midas)

Comment 3

[Kathleen :Brade]

Comment on attachment 111794 [details] [diff] [review]
one way of handling boolean parameters (in Midas)

feedback? comments?

Comment 4

[Simon Fraser [no longer active]]

+          // if this is a boolean value and it's not explicitly false
+          // (e.g. no value) we default to "true" 
+          outBooleanValue = convertedParam.Equals("false",
+                                   nsCaseInsensitiveCStringComparator());

Is this how the IE api works? Should we be more lax, like allowing "yes"?

Comment 5

[Mike Kaply [:mkaply]]

I did some testing and found that if you pass a boolean via Javascript 
(true/false), Mozilla internally goes through the effort of converting it to 
text, so by the time it gets to us, it already is "true" or "false", so we 
should document this to take real booleans.

I'm wondering if going through all this boolean stuff in nsDocument is 
necessary. Would it make more sense to DoCommandParams in 
nsComposerDocumentCommand.cpp just do a GetStringValue instead of a 
GetBooleanValue and check for "true" or "false"?

That would avoid all the overhead of modifying the existing structures and code 
and things.

Comment 6

[Mike Kaply [:mkaply]]

Attachment: New type of patch

OK, this patch makes very minor changes to nsComposerDocumentCommands to use
"true" and "false" instead of booleans for what is passed to readonly and
usecss.

Looking for feedback.

Comment 7

[Mike Kaply [:mkaply]]

Comment on attachment 111794 [details] [diff] [review]
one way of handling boolean parameters (in Midas)

I'd prefer that we used string compares in the command code instead of
converting to boolean, but I understand why we are doing it this way.

r=mkaply

Comment 8

[Simon Fraser [no longer active]]

Comment on attachment 111794 [details] [diff] [review]
one way of handling boolean parameters (in Midas)

>Index: src/nsHTMLDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v
>retrieving revision 3.458
>diff -u -6 -r3.458 nsHTMLDocument.cpp
>--- src/nsHTMLDocument.cpp	20 Dec 2002 14:53:44 -0000	3.458
>+++ src/nsHTMLDocument.cpp	17 Jan 2003 03:11:28 -0000
>@@ -4157,60 +4157,64 @@
> 
> struct MidasCommand {
>   const char*  incomingCommandString;
>   const char*  internalCommandString;
>   const char*  internalParamString;
>   PRBool       useNewParam;
>+  PRBool       convertToBoolean;
> };

Using PRPackedBool would save a bunch of global data.
> PRBool
> nsHTMLDocument::ConvertToMidasInternalCommand(const nsAString & inCommandID, 
>                                               const nsAString & inParam,
>                                               nsACString& outCommandID,
>-                                              nsACString& outParam)
>+                                              nsACString& outParam,
>+                                              PRBool& outIsBoolean,
>+                                              PRBool& outBooleanValue)
> {
>   NS_ConvertUCS2toUTF8 convertedCommandID(inCommandID);
>   PRUint32 i, j;
>   for (i = 0; i < MidasCommandCount; ++i) {
>     if (convertedCommandID.Equals(gMidasCommandTable[i].incomingCommandString,
>                                   nsCaseInsensitiveCStringComparator())) {
>       // set outCommandID (what we use internally)

I don't really like the logic in this method. Right now, it's:

  for () {
    if (found) {
      do a bunch of stuff;
      return;
    }
  }

I'd prefer that the loop contents just do the finding, and you pull
the 'success' code out of the loop. It would make things easier to read.

Also, I haven't yet seen an answer about whether we should allow values of
"yes", "1" etc. to equate with "true".

Comment 9

[Kathleen :Brade]

Attachment: updated diff per sfraser's comments

Comment 10

[Kathleen :Brade]

Comment on attachment 112220 [details] [diff] [review]
updated diff per sfraser's comments

carry forward r=mkaply from previous patch

Comment 11

[Kathleen :Brade]

fixed