Closed
Bug 279416
Opened 20 years ago
Closed 20 years ago
Implement a controller for cmd_SwitchTextDirection
Categories
(Core :: DOM: Editor, defect, P2)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla1.8beta1
People
(Reporter: asaf, Assigned: asaf)
References
Details
Attachments
(3 files, 4 obsolete files)
(deleted),
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Brade
:
review+
asaf
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
asaf
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
fix the xxx from bug 85420....: cmd_SwitchTextDirection (used in both firefox
and xpfe/browser) should be a controller.
Assignee | ||
Updated•20 years ago
|
Assignee | ||
Comment 1•20 years ago
|
||
Attachment #172123 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #172123 -
Flags: review?(brade)
Comment 2•20 years ago
|
||
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?
Assignee | ||
Comment 3•20 years ago
|
||
Simon, if there is a better way to detect the root element direction, please
tell us ;)
Assignee | ||
Comment 4•20 years ago
|
||
(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•20 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)
Updated•20 years ago
|
Summary: Implement a contoller for cmd_SwitchTextDirection → Implement a controller for cmd_SwitchTextDirection
Assignee | ||
Updated•20 years ago
|
Attachment #172123 -
Flags: superreview?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 6•20 years ago
|
||
Thanks brade.
Attachment #172123 -
Attachment is obsolete: true
Attachment #172160 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #172160 -
Flags: review?(brade)
Assignee | ||
Comment 7•20 years ago
|
||
Attachment #172164 -
Flags: review?(mconnor)
Comment 8•20 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•20 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•20 years ago
|
||
(In reply to comment #5)
>Please lower case 's' in cmd_SwitchTextDirection.
Oh, and you forgot to do this too.
Comment 11•20 years ago
|
||
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-
Comment 12•20 years ago
|
||
default:
rv = false;
break;
would be useless, of course, time for caffeine
Assignee | ||
Comment 13•20 years ago
|
||
> 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.
Assignee | ||
Comment 14•20 years ago
|
||
Attachment #172164 -
Attachment is obsolete: true
Attachment #172211 -
Flags: review?(mconnor)
Assignee | ||
Comment 15•20 years ago
|
||
addressing neil's comments (and moving sr=).
Attachment #172160 -
Attachment is obsolete: true
Attachment #172214 -
Flags: superreview+
Attachment #172214 -
Flags: review?(brade)
Assignee | ||
Updated•20 years ago
|
Attachment #172160 -
Flags: review?(brade)
Comment 16•20 years ago
|
||
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+
Assignee | ||
Comment 17•20 years ago
|
||
Comment 18•20 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+
Assignee | ||
Updated•20 years ago
|
Attachment #172249 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Updated•20 years ago
|
Attachment #172249 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #172249 -
Flags: review?(smontagu)
Attachment #172249 -
Flags: review?(neil.parkwaycc.co.uk)
Updated•20 years ago
|
Attachment #172249 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 19•20 years ago
|
||
Attachment #172249 -
Attachment is obsolete: true
Attachment #172378 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #172378 -
Flags: review+
Assignee | ||
Updated•20 years ago
|
Attachment #172249 -
Flags: superreview?(neil.parkwaycc.co.uk)
Updated•20 years ago
|
Attachment #172378 -
Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Assignee | ||
Comment 20•20 years ago
|
||
thanks for reviews, checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 21•20 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•20 years ago
|
||
Sure, but it's editor moa you need really.
Comment 23•20 years ago
|
||
Sorry, I misread that, you asked brade too.
Assignee | ||
Comment 24•20 years ago
|
||
r=brade on irc; later today.
You need to log in
before you can comment on or make changes to this bug.
Description
•