Last Comment Bug 279416 - Implement a controller for cmd_SwitchTextDirection
: Implement a controller for cmd_SwitchTextDirection
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: P2 normal with 1 vote (vote)
: mozilla1.8beta1
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
: sairuh (rarely reading bugmail)
Mentors:
Depends on: bidiui
Blocks:
  Show dependency treegraph
 
Reported: 2005-01-22 14:37 PST by Mano (::mano, needinfo? for any questions; not reading general bugmail)
Modified: 2005-02-09 10:15 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
backend part (8.32 KB, patch)
2005-01-22 14:41 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
backend patch: v1.1 (8.21 KB, patch)
2005-01-23 06:37 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
neil: superreview+
Details | Diff | Review
browser/ bidiui cleanup (10.44 KB, patch)
2005-01-23 07:20 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mconnor: review-
Details | Diff | Review
browser/ bidiui cleanup: v1.1 (13.25 KB, patch)
2005-01-23 15:24 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mconnor: review+
Details | Diff | Review
backend patch 1.2 (7.98 KB, patch)
2005-01-23 15:44 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
brade: review+
asaf: superreview+
Details | Diff | Review
xpfe/ patch (10.09 KB, patch)
2005-01-24 03:18 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
smontagu: review+
Details | Diff | Review
xpfe/ patch v1.1 (15.30 KB, patch)
2005-01-25 12:10 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
asaf: review+
neil: superreview+
Details | Diff | Review

Description Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-22 14:37:20 PST
fix the xxx from bug 85420....: cmd_SwitchTextDirection (used in both firefox
and xpfe/browser) should be a controller.
Comment 1 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-22 14:41:50 PST
Created attachment 172123 [details] [diff] [review]
backend part
Comment 2 Christian :Biesinger (don't email me, ping me on IRC) 2005-01-22 15:14:06 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-22 16:39:01 PST
Simon, if there is a better way to detect the root element direction, please
tell us ;)
Comment 4 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-22 16:42:46 PST
(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 2005-01-22 17:04:23 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-23 06:37:52 PST
Created attachment 172160 [details] [diff] [review]
backend patch: v1.1

Thanks brade.
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-23 07:20:08 PST
Created attachment 172164 [details] [diff] [review]
browser/ bidiui cleanup
Comment 8 neil@parkwaycc.co.uk 2005-01-23 08:25:32 PST
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 neil@parkwaycc.co.uk 2005-01-23 08:41:49 PST
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 neil@parkwaycc.co.uk 2005-01-23 08:51:32 PST
(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] 2005-01-23 09:56:01 PST
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] 2005-01-23 10:00:04 PST
  default:
    rv = false;
    break;

would be useless, of course, time for caffeine
Comment 13 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-23 10:25:24 PST
> 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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-23 15:24:15 PST
Created attachment 172211 [details] [diff] [review]
browser/ bidiui cleanup: v1.1
Comment 15 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-23 15:44:52 PST
Created attachment 172214 [details] [diff] [review]
backend patch 1.2

addressing neil's comments (and moving sr=).
Comment 16 Mike Connor [:mconnor] 2005-01-23 16:01:11 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-24 03:18:51 PST
Created attachment 172249 [details] [diff] [review]
xpfe/ patch
Comment 18 Kathleen Brade 2005-01-24 06:11:24 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-25 12:10:29 PST
Created attachment 172378 [details] [diff] [review]
xpfe/ patch v1.1
Comment 20 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-25 17:41:59 PST
thanks for reviews, checked in.
Comment 21 Eyal Rozenberg 2005-02-04 06:44:48 PST
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 neil@parkwaycc.co.uk 2005-02-04 10:05:41 PST
Sure, but it's editor moa you need really.
Comment 23 neil@parkwaycc.co.uk 2005-02-04 10:06:16 PST
Sorry, I misread that, you asked brade too.
Comment 24 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-02-09 10:15:48 PST
r=brade on irc; later today.

Note You need to log in before you can comment on or make changes to this bug.