279416 - Implement a controller for cmd_SwitchTextDirection

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Asaf Romano (gone)]

fix the xxx from bug 85420....: cmd_SwitchTextDirection (used in both firefox
and xpfe/browser) should be a controller.

Comment 1

[Asaf Romano (gone)]

Attachment: backend part

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 172123 [details] [diff] [review]
backend part

editor/libeditor/base/nsEditor.cpp
+	     nsAutoString attr(NS_LITERAL_STRING("dir"));

NS_NAMED_LITERAL_STRING(attr, "dir");

but it seems like you aren't using this?

Comment 3

[Asaf Romano (gone)]

Simon, if there is a better way to detect the root element direction, please
tell us ;)

Comment 4

[Asaf Romano (gone)]

(In reply to comment #2)
> (From update of attachment 172123 [details] [diff] [review] [edit])
> editor/libeditor/base/nsEditor.cpp
> +	     nsAutoString attr(NS_LITERAL_STRING("dir"));
> 
> NS_NAMED_LITERAL_STRING(attr, "dir");
> 
> but it seems like you aren't using this?

Ooops, I forgot to remove those two, thanks.

Comment 5

[Kathleen :Brade]

Comment on attachment 172123 [details] [diff] [review]
backend part

Index: editor/idl/nsIEditor.idl
 * SwitchTextDirection should begin with a lower-case 's' like other methods in
the idl.

Index: editor/libeditor/base/nsEditor.cpp
 * I know that this file isn't 100% consistent but please use the style where
curly braces are on separate lines (I believe the file is > 50% this style). 
This means that an if would have the { below the i rather than on the same
line.
 * In nsEditor::SwitchTextDirection() you have this line:
	  rv = NS_OK;
You should remove it since you are guaranteed that rv is already NS_OK.

 * For nsSwitchTextDirectionCommand::IsCommandEnabled, I would combine the
assignment into one line:
 *outCmdEnabled = (editor) ? PR_TRUE : PR_FALSE; (or just "editor" without
using PR_TRUE/FALSE).

 * In nsSwitchTextDirectionCommand::GetCommandStateParams, I like to see
variables initialized; please init canSwitchTextDirection to false (or true).

Index: editor/libeditor/base/nsEditorController.cpp
 * Please lower case 's' in cmd_SwitchTextDirection.  That seems most
consistent with other commands (or point me to a list of commands that you are
consistent with).

r=brade with those changes.  Thanks!
(Please submit a new patch so that it can easily be applied and backed out for
testing purposes.  Thanks!)

Comment 6

[Asaf Romano (gone)]

Attachment: backend patch: v1.1

Thanks brade.

Comment 7

[Asaf Romano (gone)]

Attachment: browser/ bidiui cleanup

Comment 8

[[e-mail]]

Comment on attachment 172164 [details] [diff] [review]
browser/ bidiui cleanup

>+                oncommand="goDoCommand('cmd_SwitchTextDirection');"/>
Shouldn't this say command="cmd_switchTextDirection" so as to disable
correctly?

>     <command id="cmd_SwitchTextDirection"
For consistency, needs to be renamed with a lower case S (and fix the key and
its command attribute too).

For the Suite, I'm just wrondering whether the definitions of the command, key
and menuitem belong in utilityOverlay.js

Comment 9

[[e-mail]]

Comment on attachment 172160 [details] [diff] [review]
backend patch: v1.1

>+  nsCOMPtr<nsIDOMElement> rootElement;
>+  nsresult rv = GetRootElement(getter_AddRefs(rootElement));
>+  if (NS_SUCCEEDED(rv))
>+  {
>+    if (rootElement)
>+    {
Editor does't seem to be consistent here so if brade is happy I'd prefer using
an early return i.e.
if (NS_FAILED(rv))
  return rv;
Also, I know it appears to be editor style, but if brade is happy, you should
be able to rely on rv instead of doing extra null checks.

>+      // Get the current body direction from its frame
>+      nsCOMPtr<nsIContent> content = do_QueryInterface(rootElement);
This is null-safe so you don't have null-test rootElement first, but I'd move
the test for content here. Also you might want to use the two-arg form of
do_QueryInterface.

>+  *outCmdEnabled = editor? PR_TRUE : PR_FALSE;
Other controllers seem to use editor != nsnull; which I guess must have been
whar brade meant.

Comment 10

[[e-mail]]

(In reply to comment #5)
>Please lower case 's' in cmd_SwitchTextDirection.
Oh, and you forgot to do this too.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 172164 [details] [diff] [review]
browser/ bidiui cleanup

>Index: browser/base/content/browser-context.inc
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser-context.inc,v
>retrieving revision 1.7
>diff -u -p -8 -r1.7 browser-context.inc
>--- browser/base/content/browser-context.inc	30 Nov 2004 08:22:43 -0000	1.7
>+++ browser/base/content/browser-context.inc	23 Jan 2005 15:17:17 -0000
>@@ -223,15 +223,15 @@
>       <menuitem id="context-metadata"
>                 label="&metadataCmd.label;"
>                 accesskey="&metadataCmd.accesskey;"
>                 oncommand="gContextMenu.showMetadata();"/>
>       <menuseparator hidden="true" id="context-sep-bidi"/>
>       <menuitem hidden="true" id="context-bidi-text-direction-toggle"
>                 label="&bidiSwitchTextDirectionItem.label;"
>                 accesskey="&bidiSwitchTextDirectionItem.accesskey;"
>-                oncommand="SwitchTextEntryDirection(gContextMenu.target)"/>
>+                oncommand="goDoCommand('cmd_SwitchTextDirection');"/>

same throughout, make the change as brade requested.

>Index: browser/base/content/utilityOverlay.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/utilityOverlay.js,v
>retrieving revision 1.11
>diff -u -p -8 -r1.11 utilityOverlay.js
>--- browser/base/content/utilityOverlay.js	30 Nov 2004 08:22:43 -0000	1.11
>+++ browser/base/content/utilityOverlay.js	23 Jan 2005 15:17:38 -0000

>@@ -282,17 +283,17 @@ function goUpdateGlobalEditMenuItems()

>-  // XXX: implement controller for cmd_SwitchTextDirection
>  document.getElementById('cmd_SwitchTextDirection').setAttribute('disabled', !document.commandDispatcher.focusedElement);
>+  if (gBidiUI)
>    goUpdateCommand('cmd_SwitchTextDirection');

this looks weird, shouldn't the diff show two lines?  And the indentation is
wrong.


>+
>+function isBidiEnabled(){
>+  var rv = false;
>+
>+  var systemLocale;
>+  try {
>+    var localeService = Components.classes["@mozilla.org/intl/nslocaleservice;1"]
>+                                 .getService(Components.interfaces.nsILocaleService);

nit: indentation off by one here.

>+    systemLocale = localeService.getSystemLocale().getCategory("NSILOCALE_CTYPE").substr(0,3);
>+    rv = /^(he|ar|syr|fa|ur)-/.test(systemLocale);

I don't know if I really like this change you've slipped in here.  If a locale
prefix needed to be added, how would anyone really know how to add this.  At
the least, there needs to be a comment explaining this.  Also, unless I'm
reading the regexp wrong, syr will never get matched, because you're trying to
match syr- with syr.

Since this is only called once I don't think we have a big problem in doing
something much more readable:

switch (systemLocale) {
  case "ar-":
  case "fa-":
  case "he-":
  case "syr":
  case "ur-":
    rv = true;
    break;
  default:
    rv = false;
    break;
}

Next issue: I realize its a hidden pref, but its no longer "live" with this
change.  Should we add a pref observer here?

>+  if (!rv) {
>+    // check the overriding pref
>+    var mPrefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
>+    try {
>+      rv = mPrefs.getBoolPref("bidi.browser.ui");
>+    }
>+    catch (e){}
>+  }
>+
>+  return rv;
>+}


the catch() should be on the same line as the closing bracket, like you do
eight lines above.

In fact, this is a bunch of unnecessary code.  In the same file, there's a
helper function to handle this for you.  You just need something like the
following.

if (!rv)
  rv = getBoolPref('bidi.browser.ui', false);

return rv;

Comment 12

[Mike Connor [:mconnor]]

  default:
    rv = false;
    break;

would be useless, of course, time for caffeine

Comment 13

[Asaf Romano (gone)]

> Next issue: I realize its a hidden pref, but its no longer "live" with this
> change.  Should we add a pref observer here?
 
It was never a 'live' pref; we're checking for this pref on delayedStartup() in
order to decide which [main] menu items should be visiable. An observer might be
a nice enhancement.

> I don't know if I really like this change you've slipped in here.  If a locale
> prefix needed to be added, how would anyone really know how to add this.  At
> the least, there needs to be a comment explaining this.
> Also, unless I'm
> reading the regexp wrong, syr will never get matched, because you're trying to
> match syr- with syr.


Well <sigh />, I forgot to remove the call to substr which (combained with the
string comp.) should be slower than any regex.

Comment 14

[Asaf Romano (gone)]

Attachment: browser/ bidiui cleanup: v1.1

Comment 15

[Asaf Romano (gone)]

Attachment: backend patch 1.2

addressing neil's comments (and moving sr=).

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 172211 [details] [diff] [review]
browser/ bidiui cleanup: v1.1

>+  // check the overriding pref
>+  if (!rv)
>+      rv = getBoolPref("bidi.browser.ui");
>+

where'd the wacky 4-space indent come from here?

other than that, r=me

Comment 17

[Asaf Romano (gone)]

Attachment: xpfe/ patch

Comment 18

[Kathleen :Brade]

Comment on attachment 172214 [details] [diff] [review]
backend patch 1.2

this line needs a space after the , :
  return aParams->SetBooleanValue(STATE_ENABLED,canSwitchTextDirection);

thanks! r=brade

Comment 19

[Asaf Romano (gone)]

Attachment: xpfe/ patch v1.1

Comment 20

[Asaf Romano (gone)]

thanks for reviews, checked in.

Comment 21

[Eyal Rozenberg]

Brade/Neil:

I believe the comment before the idl entry for switchTextDirection should be
changed to:

"switches the direction of the editor's document body from left-to-right to
right-to-left or vice versa"

so that there will be no way to interpret it as changing the current paragraph
direction rather than the direction of the entire message.

So is it okay with you that I ask Asaf to check this comment change in?

Is 

Comment 22

[[e-mail]]

Sure, but it's editor moa you need really.

Comment 23

[[e-mail]]

Sorry, I misread that, you asked brade too.

Comment 24

[Asaf Romano (gone)]

r=brade on irc; later today.