Implement a controller for cmd_SwitchTextDirection

RESOLVED FIXED in mozilla1.8beta1

Status

()

Core
Editor
P2
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

Trunk
mozilla1.8beta1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

fix the xxx from bug 85420....: cmd_SwitchTextDirection (used in both firefox
and xpfe/browser) should be a controller.
Status: NEW → ASSIGNED
Depends on: 85420
Priority: -- → P2
Target Milestone: --- → mozilla1.8beta
Created attachment 172123 [details] [diff] [review]
backend part
Attachment #172123 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #172123 - Flags: review?(brade)
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?
Simon, if there is a better way to detect the root element direction, please
tell us ;)
(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

12 years ago
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!)
Attachment #172123 - Flags: review?(brade)
Summary: Implement a contoller for cmd_SwitchTextDirection → Implement a controller for cmd_SwitchTextDirection
Attachment #172123 - Flags: superreview?(neil.parkwaycc.co.uk)
Created attachment 172160 [details] [diff] [review]
backend patch: v1.1

Thanks brade.
Attachment #172123 - Attachment is obsolete: true
Attachment #172160 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #172160 - Flags: review?(brade)
Created attachment 172164 [details] [diff] [review]
browser/ bidiui cleanup
Attachment #172164 - Flags: review?(mconnor)

Comment 8

12 years ago
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

12 years ago
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.
Attachment #172160 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+

Comment 10

12 years ago
(In reply to comment #5)
>Please lower case 's' in cmd_SwitchTextDirection.
Oh, and you forgot to do this too.
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;
Attachment #172164 - Flags: review?(mconnor) → review-
  default:
    rv = false;
    break;

would be useless, of course, time for caffeine
> 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.
Created attachment 172211 [details] [diff] [review]
browser/ bidiui cleanup: v1.1
Attachment #172164 - Attachment is obsolete: true
Attachment #172211 - Flags: review?(mconnor)
Created attachment 172214 [details] [diff] [review]
backend patch 1.2

addressing neil's comments (and moving sr=).
Attachment #172160 - Attachment is obsolete: true
Attachment #172214 - Flags: superreview+
Attachment #172214 - Flags: review?(brade)
Attachment #172160 - Flags: review?(brade)
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
Attachment #172211 - Flags: review?(mconnor) → review+
Created attachment 172249 [details] [diff] [review]
xpfe/ patch

Comment 18

12 years ago
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
Attachment #172214 - Flags: review?(brade) → review+
Attachment #172249 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #172249 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #172249 - Flags: review?(smontagu)
Attachment #172249 - Flags: review?(neil.parkwaycc.co.uk)

Updated

12 years ago
Attachment #172249 - Flags: review?(smontagu) → review+
Created attachment 172378 [details] [diff] [review]
xpfe/ patch v1.1
Attachment #172249 - Attachment is obsolete: true
Attachment #172378 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #172378 - Flags: review+
Attachment #172249 - Flags: superreview?(neil.parkwaycc.co.uk)

Updated

12 years ago
Attachment #172378 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
thanks for reviews, checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 21

12 years ago
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

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

Comment 23

12 years ago
Sorry, I misread that, you asked brade too.
r=brade on irc; later today.
You need to log in before you can comment on or make changes to this bug.