Closed Bug 451960 Opened 12 years ago Closed 10 years ago

s/observes=/command=/g in /editor/ui/*

Categories

(SeaMonkey :: Composer, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sgautherie, Assigned: ewong)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 3 obsolete files)

Bug 446281 comment 14:
[
neil@parkwaycc.co.uk   2008-08-14 15:36:51 PDT

I wonder why these are still observes= instead of command=
(I didn't try to see whether command= works or not).
]
Blocks: 621861
"Found 202 matching lines in 5 files"

***

See bug 613435 as an example.
Summary: Replace |observes=| by |command=| in <EditorContextMenuOverlay.xul> → s/observes=/command=/g in /editor/ui/*
Whiteboard: [good first bug]
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #523878 - Flags: review?(neil)
This is so going to bitrot me, but as my patches probably won't make it into 2.1, due to string freeze, I suppose it doesn't really matter.
(In reply to comment #3)
> This is so going to bitrot me, but as my patches probably won't make it into
> 2.1, due to string freeze, I suppose it doesn't really matter.

I would think it would matter.  I'll wait until you check in your patch
then I'll fix whatever needs fixing.  It's not like this bug is in 
a hurry to be fixed. :)
Attachment #523878 - Flags: review?(neil)
Attachment #523878 - Attachment is obsolete: true
(In reply to comment #4)
> It's not like this bug is in a hurry to be fixed. :)

It's not, but now that your patch is done...
Or your could do replacement only now and postpone reformatting.
Comment on attachment 523878 [details] [diff] [review]
s/observes=/command=/g in /editor/ui/*

>-    <observes element="paragraph-select-container" attribute="disabled"/>
>-    <observes element="cmd_paragraphState" attribute="state" onbroadcast="onParagraphFormatChange(this.parentNode, 'cmd_paragraphState')"/>
>+    <command element="paragraph-select-container" attribute="disabled"/>
>+    <command element="cmd_paragraphState" attribute="state" onbroadcast="onParagraphFormatChange(this.parentNode, 'cmd_paragraphState')"/>
This change is wrong.

The other changes which are wrong relate to cmd_renderedHTMLEnabler. This element is badly named, as it's really a broadcaster that is used to disable elements when the focus is in the subject field in a message, and doesn't have an action. This means that we shouldn't use command= as it inhibits any oncommand event on the element or its ancestors. (It doesn't appear to inhibit the event on child nodes, but that's not so useful as command= is designed for menuitems and keys and they don't have children anyway.)
Attachment #523878 - Flags: review-
Attachment #524056 - Flags: review?(neil)
Comment on attachment 524056 [details] [diff] [review]
s/observes=/command=/g in /editor/ui/* (v2)

>diff --git a/editor/ui/composer/content/editor.xul b/editor/ui/composer/content/editor.xul
The two changes in this file are wrong ones that you forgot to revert.

>     <menupopup id="fontStyleMenuPopup" onpopupshowing="initFontStyleMenu(this)">
Sadly initFontStyleMenu doesn't work with command=
So all the changes in this menupopup have to be removed, pending a followup.

>+    <menuitem id="fontColor"
>+              label="&formatFontColor.label;" 
>+              accesskey="&formatFontColor.accesskey;"
>+              command="cmd_fontColor"
>+              oncommand="EditorSelectColor('Text', null);"
Can't have both command and oncommand on the same element. I'm not quite sure what the point of the cmd_fontColor element is, but let's keep it observes=.

>+    <menuitem id="removeStylesMenuitem"
>+              key="removestyleskb"
>+              command="cmd_removeStyles"
>+              position="6"/>
>+    <menuitem id="removeLinksMenuitem"
>+              key="removelinkskb"
>+              command="cmd_removeLinks"
>+              position="7"/>
>+    <menuitem id="removeNamedAnchorsMenuitem"
>+              label="&formatRemoveNamedAnchors.label;"
>+              key="removenamedanchorskb"
>+              accesskey="&formatRemoveNamedAnchors.accesskey;"
>+              command="cmd_removeNamedAnchors"
>+              position="8"/>
These changes don't work for some reason that I didn't bother to figure out.

>-  <menuitem id="objectProperties"   oncommand="goDoCommand('cmd_objectProperties')"   observes="cmd_renderedHTMLEnabler"/>
>+  <menuitem id="objectProperties"
>+            oncommand="goDoCommand('cmd_objectProperties')"
>+            observes="cmd_renderedHTMLEnabler"/>
You didn't undo the edit you made to this correctly.

>   <toolbarbutton id="AlignPopupButton"
>                  class="formatting-button"
>                  tooltiptext="&AlignPopupButton.tooltip;"
>                  type="menu"
>-                 observes="cmd_align">
>+                 command="cmd_align">
I'd like to remove the changes to menubuttons. I'm not sure they're useful.

>+      <menuitem id="InsertLinkItem"
>+                class="menuitem-iconic"
>+                command="cmd_link"
>+                oncommand="goDoCommand('cmd_link')"
...
>+      <menuitem id="InsertTableItem"
>+                class="menuitem-iconic"
>+                command="cmd_table"
>+                oncommand="goDoCommand('cmd_table')"
I said before you don't want both command and oncommand. Fortunately in these cases the command= does all the work and the oncommand is now unnecessary.

>   <toolbarbutton id="smileButtonMenu"
>                  class="formatting-button"
>                  tooltiptext="&SmileButton.tooltip;"
>                  type="menu"
>-                 observes="cmd_smiley">
>+                 command="cmd_smiley">
Again, I don't think this one needs to be changed.
Attachment #524056 - Flags: review?(neil) → review-
(In reply to comment #8)

> Can't have both command and oncommand on the same element. I'm not quite sure
> what the point of the cmd_fontColor element is, but let's keep it observes=.

cmd_fontColor was meant to be reusable elsewhere. The handler is here because
the color boxes accept a shift-click to select directly the last selected
color, IIRC.
Attachment #524056 - Attachment is obsolete: true
Attachment #524623 - Flags: review?(neil)
Comment on attachment 524623 [details] [diff] [review]
s/observes=/command=/g in /editor/ui/* (v3)

>-      <menuitem id="InsertLinkItem" class="menuitem-iconic" observes="cmd_link"
>-                oncommand="goDoCommand('cmd_link')" label="&linkToolbarCmd.label;"
>-                tooltiptext="&linkToolbarCmd.tooltip;"   />
>-      <menuitem id="InsertAnchorItem" class="menuitem-iconic" observes="cmd_anchor"
>-                oncommand="goDoCommand('cmd_anchor')" label="&anchorToolbarCmd.label;"
>-                tooltiptext="&anchorToolbarCmd.tooltip;" />
>-      <menuitem id="InsertImageItem"  class="menuitem-iconic" observes="cmd_image"
>-                oncommand="goDoCommand('cmd_image')" label="&imageToolbarCmd.label;"
>-                tooltiptext="&imageToolbarCmd.tooltip;"  />
>-      <menuitem id="InsertHRuleItem"  class="menuitem-iconic" observes="cmd_hline"
>-                oncommand="goDoCommand('cmd_hline')" label="&hruleToolbarCmd.label;"
>-                tooltiptext="&hruleToolbarCmd.tooltip;"  />
>-      <menuitem id="InsertTableItem"  class="menuitem-iconic" observes="cmd_table"
>-                oncommand="goDoCommand('cmd_table')" label="&tableToolbarCmd.label;"
>-                tooltiptext="&tableToolbarCmd.tooltip;"  />
>+      <menuitem id="InsertLinkItem"
>+                class="menuitem-iconic"
>+                observes="cmd_link"
>+                oncommand="goDoCommand('cmd_link')"
>+                label="&linkToolbarCmd.label;"
>+                tooltiptext="&linkToolbarCmd.tooltip;"/>
>+      <menuitem id="InsertAnchorItem"
>+                class="menuitem-iconic"
>+                observes="cmd_anchor"
>+                oncommand="goDoCommand('cmd_anchor')"
>+                label="&anchorToolbarCmd.label;"
>+                tooltiptext="&anchorToolbarCmd.tooltip;"/>
>+      <menuitem id="InsertImageItem"
>+                class="menuitem-iconic"
>+                observes="cmd_image"
>+                oncommand="goDoCommand('cmd_image')"
>+                label="&imageToolbarCmd.label;"
>+                tooltiptext="&imageToolbarCmd.tooltip;"/>
>+      <menuitem id="InsertHRuleItem"
>+                class="menuitem-iconic"
>+                observes="cmd_hline"
>+                oncommand="goDoCommand('cmd_hline')"
>+                label="&hruleToolbarCmd.label;"
>+                tooltiptext="&hruleToolbarCmd.tooltip;"/>
>+      <menuitem id="InsertTableItem"
>+                class="menuitem-iconic"
>+                observes="cmd_table"
>+                oncommand="goDoCommand('cmd_table')"
>+                label="&tableToolbarCmd.label;"
>+                tooltiptext="&tableToolbarCmd.tooltip;"/>
These you changed from observes="" to command="", then you reformatted, then I asked you to remove the oncommand="", but instead you changed the command="" back to observes="" ... rest of the patch is OK though.
Sorry about that.  Your instructions were very clear.
It's my brain that was befuddled.  The next patch
should address that.
Attachment #524623 - Attachment is obsolete: true
Attachment #524848 - Flags: review?(neil)
Attachment #524623 - Flags: review?(neil)
Attachment #524848 - Flags: review?(neil) → review+
Keywords: checkin-needed
Comment on attachment 524848 [details] [diff] [review]
s/observes=/command=/g in /editor/ui/* (v4)
[Checked in: Comment 14]

http://hg.mozilla.org/comm-central/rev/b36443e20146
Attachment #524848 - Attachment description: s/observes=/command=/g in /editor/ui/* (v4) → s/observes=/command=/g in /editor/ui/* (v4) [Checked in: Comment 14]
Keywords: checkin-needed
(In reply to comment #8)
> These changes don't work for some reason that I didn't bother to figure out.
Turned out to be the same problem as the font style submenu - command confuses the code by synchronising some attributes that observes doesn't.
(In reply to comment #1)
> "Found 202 matching lines in 5 files"

Down to "Found 54 matching lines in 3 files" :-)


(In reply to comment #6)
> The other changes which are wrong relate to cmd_renderedHTMLEnabler. This
> element is badly named, as it's really a broadcaster that is used to disable
> elements when the focus is in the subject field in a message, and doesn't have
> an action. This means that we shouldn't use command= as it inhibits any
> oncommand event on the element or its ancestors. (It doesn't appear to inhibit
> the event on child nodes, but that's not so useful as command= is designed for
> menuitems and keys and they don't have children anyway.)

Ftr,
http://mxr.mozilla.org/comm-central/search?string=cmd_renderedHTMLEnabler&case=1&find=%2Fcompose
"Found 72 matching lines in 9 files" (including /mail and /suite)
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/ComposerCommands.js#463
465 var nsDummyHTMLCommand =
469     return (IsDocumentEditable() && IsEditingRenderedHTML());
478     dump("Hey, who's calling the dummy command?\n");

https://developer.mozilla.org/en/XUL_Tutorial/Broadcasters_and_Observers
Neil, should I file a separate bug to rename cmd_renderedHTMLEnabler?
And possibly change it to a broadcaster instead of a (dummy) command?


(In reply to comment #8)
> So all the changes in this menupopup have to be removed, pending a followup.
[And following comments.]

We should (re-)check the few remaining cases to summarize what's left to do (here or in follow-ups).
(In reply to comment #16)
> Neil, should I file a separate bug to rename cmd_renderedHTMLEnabler?
> And possibly change it to a broadcaster instead of a (dummy) command?
Yeah, it should definitely become a broadcaster, and probably be renamed too.
Comment on attachment 524848 [details] [diff] [review]
s/observes=/command=/g in /editor/ui/* (v4)
[Checked in: Comment 14]

>     <!-- label and accesskey are set in InitObjectProperties -->
>-    <menuitem id="objectProperties_cm" observes="cmd_objectProperties"/>
>+    <menuitem id="objectProperties_cm"
>+              command="cmd_objectProperties"/>
This doesn't work :-( Problem caused by incorrect comment - InitObjectPropertiesMenuitem also tries to set the disabled state.
Resolving.  Any remnant issues can be spun off into a new bug.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to neil@parkwaycc.co.uk from comment #17)
> Yeah, it should definitely become a broadcaster, and probably be renamed too.

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