Closed Bug 606683 Opened 14 years ago Closed 4 years ago

Allow customization of toolbars in Composer and formatting toolbar in MailNews Composition

Categories

(SeaMonkey :: Composer, defect)

defect
Not set
normal

Tracking

(seamonkey2.53+ fixed, seamonkey2.57esr? affected)

RESOLVED FIXED
seamonkey 2.75
Tracking Status
seamonkey2.53 + fixed
seamonkey2.57esr ? affected

People

(Reporter: iannbugzilla, Assigned: iannbugzilla)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: SM2.53.3)

Attachments

(6 files, 11 obsolete files)

22.35 KB, patch
iannbugzilla
: review+
Details | Diff | Splinter Review
6.50 KB, patch
iannbugzilla
: review+
stefanh
: ui-review+
Details | Diff | Splinter Review
19.96 KB, patch
neil
: review+
Details | Diff | Splinter Review
680 bytes, patch
neil
: review+
Details | Diff | Splinter Review
71.56 KB, patch
frg
: review+
Details | Diff | Splinter Review
60.51 KB, patch
frg
: review+
Details | Diff | Splinter Review
At the moment the toolbars in the Composer and MailNews Composition windows are not customizable - that should be fixed.
Attachment #485506 - Flags: feedback?(stefanh)
I did look at putting on an ignore whitespace change patch but less than 1K smaller.
Attachment #485506 - Flags: feedback?(jh)
Comment on attachment 485506 [details] [diff] [review]
Make toolbars customizable patch v0.1

I have not looked at the code (except when I tracked the console warning), so this is more of a ui-behaviour feedback. Overall, it works fine - nice job :-)

But there are a few things that we need to settle:

1: The format toolbar buttons have no label text (and we don't want them to have label text). The icons also only comes in one size (anything else would of course be strange) so the choices for "Settings for this toolbar" doesn't make sense here.

2: When switching between modes (normal, html tags, html source,  preview), your choice of showing/hiding the format toolbar doesn't persist (try for example to hide the format toolbar in "normal" mode, then switch to "html tags" and back). I'm also not clear how we should handle the format toolbar in the different modes - but we should probably treat all modes as they have the same toolbar ("persisting through modes")? Need then to consider the fact that the source mode view then would have a disabled format toolbar... :-/

3: Showing/hiding a toolbar by selecting/deselecting in the context menu doesn't update the menuitem in the View menu. Doing it from the View Menu updates the context menuitems, though

4: When you have the customize sheet in front of you, it's possible to type in the editable area - but you knew that already since we talked about it :-)

5:
-  gEditorToolbarPrefListener = new nsPrefListener(kEditorToolbarPrefs);

EditorShutdown() does gEditorToolbarPrefListener.shutdown(), so I get a warning in the error console when closing the editor window:
----------------------------------------------
Error: gEditorToolbarPrefListener is undefined
Source File: chrome://editor/content/editor.js
Line: 673
----------------------------------------------

Minusing since I think we need to settle the behaviour in 1-4 first.
Attachment #485506 - Flags: feedback?(stefanh) → feedback-
Comment on attachment 485506 [details] [diff] [review]
Make toolbars customizable patch v0.1

Sorry for the delay, first had to get a clean tree...

(In reply to comment #0)
> At the moment the toolbars in the Composer and MailNews Composition windows are
> not customizable

AFAICS MailNews compose windows are already partly customizable, "only" the ability to customize the Formatting Toolbar was added. Care to update the bug summary (which will probably also help the check-in comment)? Just asking, maybe I'm wrong. ;-)

It seems Stefan already did a good job at checking the behavior (much better than what I would have come up with, in fact). Since I'm generally happy with it I give a f+ if you address the View menu issue, but if in doubt, take Stefan's feedback (check-in comment etc.).

One more To Do for you (just FTR, I'm sure you'd have taken care of that eventually): Help needs to be adapted, either here or in a follow-up.

(In reply to comment #2)
> 1: The format toolbar buttons have no label text (and we don't want them to
> have label text).

We don't? Why? If the user decides to go with text labels, shouldn't that s/he be able to achieve that? Or am I getting you wrong?

> The icons also only comes in one size (anything else would of
> course be strange) so the choices for "Settings for this toolbar" doesn't make
> sense here.

I wonder whether any choice we make here affects all themes (i.e. whether there's only one switch, so if we disabled it for Classic and Modern, would third-party themes be able to provide bigger icons?). More generally speaking, as long as selecting another option doesn't completely break the display, I'd be OK if we allowed selecting a size that we don't actually provide icons for in the bundled themes.

> 2: When switching between modes (normal, html tags, html source,  preview),
> your choice of showing/hiding the format toolbar doesn't persist

True.

> we should probably treat all modes as they have the same
> toolbar ("persisting through modes")? Need then to consider the fact that the
> source mode view then would have a disabled format toolbar... :-/

Actually I'd support that idea. I see that the content is needlessly jumping around because of the showing and hiding when switching modes. From my POV it's not a blocker, though, i.e. you could do that in a follow-up or even leave it like it is.

> 3: Showing/hiding a toolbar by selecting/deselecting in the context menu
> doesn't update the menuitem in the View menu. Doing it from the View Menu
> updates the context menuitems, though

Good catch. This indeed needs to be fixed.

> 4: When you have the customize sheet in front of you, it's possible to type in
> the editable area

Hmm, isn't that actually a feature? ;-) No seriously, does this have any negative implications?

Note: I skimmed through the patch, but didn't even attempt to do a code review. Ratty might be a good candidate.

Thanks for working on this!
Attachment #485506 - Flags: feedback?(jh) → feedback+
(In reply to comment #3)
> > 1: The format toolbar buttons have no label text (and we don't want them to
> > have label text).
> 
> We don't? Why? If the user decides to go with text labels, shouldn't that s/he
> be able to achieve that? Or am I getting you wrong?

I don't think the text will fit into the current toolbar... That is, we don't have any text labels and we (imo) don't want any.

> 
> > The icons also only comes in one size (anything else would of
> > course be strange) so the choices for "Settings for this toolbar" doesn't make
> > sense here.
> 
> I wonder whether any choice we make here affects all themes (i.e. whether
> there's only one switch, so if we disabled it for Classic and Modern, would
> third-party themes be able to provide bigger icons?). More generally speaking,
> as long as selecting another option doesn't completely break the display, I'd
> be OK if we allowed selecting a size that we don't actually provide icons for
> in the bundled themes.

Yes, but why would you want to give users an option that doesn't have any effect?


> > 4: When you have the customize sheet in front of you, it's possible to type in
> > the editable area
> 
> Hmm, isn't that actually a feature? ;-) No seriously, does this have any
> negative implications?

It's a bit weird that you can type behind a pane since when you're customizing all menus are disabled - you don't really want a user do do anything but customizing (and we don't do that in mailNews iirc).
So, where are we regarding this? I think we should have it in a beta, so requesting status2.1.

As for "Settings for this toolbar", I think we could just leave that one for now as it doesn't break anything if we have/keep it. If someone really wants the labels (with or without icons) despite the waste of space (think users with visual disabilities), I think they should be able to activate it. And bigger icons, if the theme provides them (again I don't think it matters that we don't have some in the default themes!).

As for the ability to type while customizing: I'll leave that to Stefan and Ian; I would think that such nits could go to a follow-up, but then that's just my opinion.
Summary: Allow customization of toolbar in Composer and MailNews Composition → Allow customization of toolbars in Composer and formatting toolbar in MailNews Composition
Depends on: 635792
I'm going to be trying to split the patches into easily reviewable patches, so this is the first one.

This patch:
* Moves some common commandset overlays into editorOverlay.
* Moves a common broadcaster into editorOverlay.
* Moves a singular use broadcaster out of editorOverlay into editor.
* Renames the Editor/Compose toolbars to have a common name so that code can be shared and moved into editorOverlay.
Attachment #514376 - Flags: review?(neil)
Comment on attachment 514376 [details] [diff] [review]
Tidy up some broadcasters and commandsets

I'm not sure that changing the toolbar's id is a good idea. The rest looks OK.
(In reply to comment #8)
> Comment on attachment 514376 [details] [diff] [review]
> Tidy up some broadcasters and commandsets
> 
> I'm not sure that changing the toolbar's id is a good idea. The rest looks OK.

To effectively share code those ids need to be the same across the 3 windows (editor, text editor and message compose), unless you know another way of doing it.
Attachment #514376 - Flags: review?(neil)
Changes since last version:
* No renaming of toolbar ids and keeping separate <command>s for editor vs message compose toggling.
Attachment #514376 - Attachment is obsolete: true
Attachment #514671 - Flags: review?(neil)
Comment on attachment 514671 [details] [diff] [review]
Tidy up some broadcasters and commandsets v2

>+  <broadcasterset id="editorBroadcasters">
>     <broadcaster id="Communicator:WorkMode"/>
>+    <broadcaster id="args" value="about:blank"/>
>+
>+    <!-- view menu -->
>+    <command id="cmd_viewEditModeToolbar"
>+             oncommand="goToggleToolbar('EditModeToolbar','cmd_viewEditModeToolbar');"
>+             checked="true"/>
[Huh, I wonder why this command lives in a broadcasterset...]

>+  <commandset id="tasksCommands">
>+    <commandset id="globalEditMenuItems"/>
>+    <commandset id="selectEditMenuItems"/>
>+    <commandset id="undoEditMenuItems"/>
>+    <commandset id="clipboardEditMenuItems"/>
>+    <command id="toggleSidebar"/>
>+  </commandset>
I think it would be neater to overlay editorCommands instead.

> <!ENTITY % editorDTD SYSTEM "chrome://editor/locale/editor.dtd">
> %editorDTD;
>+<!ENTITY % editorOverlayDTD SYSTEM "chrome://editor/locale/editorOverlay.dtd">
>+%editorOverlayDTD;
IMHO it would be better to move the string into editor.dtd
(given that the overlay doesn't use the string...)

>-<broadcasterset id="composeBroadcasters">
>+<broadcasterset id="editorBroadcasters">
This change doesn't look right, since it's in compose.xul, not editor.xul - unless you can explain why, of course...
(In reply to comment #11)
> Comment on attachment 514671 [details] [diff] [review]
> Tidy up some broadcasters and commandsets v2
> 
> >+  <broadcasterset id="editorBroadcasters">
> >     <broadcaster id="Communicator:WorkMode"/>
> >+    <broadcaster id="args" value="about:blank"/>
> >+
> >+    <!-- view menu -->
> >+    <command id="cmd_viewEditModeToolbar"
> >+             oncommand="goToggleToolbar('EditModeToolbar','cmd_viewEditModeToolbar');"
> >+             checked="true"/>
> [Huh, I wonder why this command lives in a broadcasterset...]
There are a whole load that live in a broadcasterset in editorOverlay.xul

> >-<broadcasterset id="composeBroadcasters">
> >+<broadcasterset id="editorBroadcasters">
> This change doesn't look right, since it's in compose.xul, not editor.xul -
> unless you can explain why, of course...
Well editorOverlay.xul is providing an overlay for the broadcasterset, I could have left them with an id of "broadcasterset" I suppose...
Attachment #514671 - Flags: review?(neil)
Changes since v2:
* Moved commands from broadcasterset to commandset.
* Overlay using editorCommands rather than tasksCommands.
* Moved entity from editorOverlay.dtd to editor.dtd.
* Changed id for broadcastersets.
Attachment #514671 - Attachment is obsolete: true
Attachment #515238 - Flags: review?(neil)
One change I missed but I have fixed locally, in messengercompose.xul changed broadcaster_throbber to Editor:Throbber in the navigator-throbber button observes element.
Attached patch toolbar item tidy up (obsolete) — Splinter Review
This patch:
* Turns menulist and stack elements in toolbar's children into toolbaritems
* Adds missing classes to toolbaritems/buttons.
* Minor tidy ups.
Attachment #515405 - Flags: review?(neil)
Attachment #515405 - Flags: review?(neil)
Attached patch toolbar item tidy up v1.1 (obsolete) — Splinter Review
Changes since v1.0:
* Also included changes made to editorSmileyOverlay.xul
Attachment #515405 - Attachment is obsolete: true
Attachment #515407 - Flags: review?(neil)
Comment on attachment 515407 [details] [diff] [review]
toolbar item tidy up v1.1

>-function EditorSelectFontSize()
I don't think it's safe to change this because it's shared code (the menulist itself is OK because Thunderbird have forked editorOverlay.xul).
[There's actually a better way of doing this by setting the value attribute on each of the menuitems so that you can simply read the menulist value.]

>+  <toolbarbutton id="AlignPopupButton"
>+                 class="toolbarbutton-1"
Why the class?

>+  <toolbaritem id="paragraph-select-container"
>+               class="chromeclass-toolbar-additional"
>+               tooltiptext="&ParagraphSelect.tooltip;"
>+               observes="cmd_renderedHTMLEnabler">
>+    <menulist id="ParagraphSelect"
>+              class="toolbar-focustarget"
>+              crop="right"
>+              observes="paragraph-select-container">
Not sure this will work. (Not even sure what it's trying to do.)

>   </menulist>
>+  </toolbaritem>
I assume the indentation will be sorted out at some point?
(In reply to comment #17)
> Comment on attachment 515407 [details] [diff] [review]
> toolbar item tidy up v1.1
> 
> >+  <toolbarbutton id="AlignPopupButton"
> >+                 class="toolbarbutton-1"
> Why the class?
I have another patch coming up that switches the style sheet from using the toolbar id to using the toolbarbutton/item class.
> 
> >+  <toolbaritem id="paragraph-select-container"
> >+               class="chromeclass-toolbar-additional"
> >+               tooltiptext="&ParagraphSelect.tooltip;"
> >+               observes="cmd_renderedHTMLEnabler">
> >+    <menulist id="ParagraphSelect"
> >+              class="toolbar-focustarget"
> >+              crop="right"
> >+              observes="paragraph-select-container">
> Not sure this will work. (Not even sure what it's trying to do.)
We use it elsewhere for keeping the disable attribute in sync.
> 
> >   </menulist>
> >+  </toolbaritem>
> I assume the indentation will be sorted out at some point?
I could, but I was trying to avoid a lot of whitespace change and taking blame of massive sections of code.
(In reply to comment #18)
> (In reply to comment #17)
> > Comment on attachment 515407 [details] [diff] [review]
> > toolbar item tidy up v1.1
> > 
> > >+  <toolbarbutton id="AlignPopupButton"
> > >+                 class="toolbarbutton-1"
> > Why the class?
> I have another patch coming up that switches the style sheet from using the
> toolbar id to using the toolbarbutton/item class.

It will look the same, right?
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > Comment on attachment 515407 [details] [diff] [review]
> > > toolbar item tidy up v1.1
> > > 
> > > >+  <toolbarbutton id="AlignPopupButton"
> > > >+                 class="toolbarbutton-1"
> > > Why the class?
> > I have another patch coming up that switches the style sheet from using the
> > toolbar id to using the toolbarbutton/item class.
> 
> It will look the same, right?

That is the intention, yes.
Attachment #515407 - Flags: review?(neil)
Attached patch toolbar item tidy up v1.2 (obsolete) — Splinter Review
Changes since v1.1:
* Removed FontSize changes - will spin those off to another bug.
* Changed class for AlignPopup, InsertPopup and absolutePostion buttons.
Attachment #515407 - Attachment is obsolete: true
Attachment #515412 - Flags: review?(neil)
This patch (when applied on top of the toolbar item tidy up patch):
* Switched the style sheets to use class rather than toolbar id to determine style.
Attachment #515415 - Flags: ui-review?(stefanh)
Attachment #515415 - Flags: review?(neil)
(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 515407 [details] [diff] [review])
> > >+  <toolbarbutton id="AlignPopupButton"
> > >+                 class="toolbarbutton-1"
> > Why the class?
> I have another patch coming up that switches the style sheet from using the
> toolbar id to using the toolbarbutton/item class.
OK, but it was a really bad choice of class. (The latest patch is still a bad choice of class, since it's intended for use with chrome flags. So you should really think up a new class name.)

> > >+  <toolbaritem id="paragraph-select-container"
> > >+               class="chromeclass-toolbar-additional"
> > >+               tooltiptext="&ParagraphSelect.tooltip;"
> > >+               observes="cmd_renderedHTMLEnabler">
> > >+    <menulist id="ParagraphSelect"
> > >+              class="toolbar-focustarget"
> > >+              crop="right"
> > >+              observes="paragraph-select-container">
> > Not sure this will work. (Not even sure what it's trying to do.)
> We use it elsewhere for keeping the disable attribute in sync.
OK, but you should then avoid putting any attributes on the toolbaritem, since the observes will copy those too (particularly bad in the case of the toolbar-focustarget class).

> > >   </menulist>
> > >+  </toolbaritem>
> > I assume the indentation will be sorted out at some point?
> I could, but I was trying to avoid a lot of whitespace change and taking blame
> of massive sections of code.
In that case, would it be better putting the <toolbaritem> at the "wrong" indentation too, so at least that block is internally consistent?
Comment on attachment 515415 [details] [diff] [review]
Switch from toolbar id to class patch v1.0

> #FormatToolbar > toolbarbutton > .toolbarbutton-text {
>   display: none;
> }
So, are you planning on giving the buttons text and setting the format toolbar to default to icons only?
(In reply to comment #23)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (From update of attachment 515407 [details] [diff] [review])
> > > >+  <toolbarbutton id="AlignPopupButton"
> > > >+                 class="toolbarbutton-1"
> > > Why the class?
> > I have another patch coming up that switches the style sheet from using the
> > toolbar id to using the toolbarbutton/item class.
> OK, but it was a really bad choice of class. (The latest patch is still a bad
> choice of class, since it's intended for use with chrome flags. So you should
> really think up a new class name.)

editorPrimaryToolbar.css already uses toolbarbutton-1 to assign a list-style-image or are you saying this is wrong?
(In reply to comment #24)
> Comment on attachment 515415 [details] [diff] [review]
> Switch from toolbar id to class patch v1.0
> 
> > #FormatToolbar > toolbarbutton > .toolbarbutton-text {
> >   display: none;
> > }
> So, are you planning on giving the buttons text and setting the format toolbar
> to default to icons only?

I plan on changing the above with the actual customisation patch.
Attachment #515412 - Flags: review?(neil)
Attached patch toolbar item tidy up v1.3 (obsolete) — Splinter Review
Changes since toolbar item tidy up v1.2:
* Added new class "format-bar-class" for use with toolbarbuttons/items typically found on the format toolbar.
* Changed start indentation vele for new toolbaritems so the block is internally consistent.
* Changed observe lines to observe specific attribute.
Attachment #515412 - Attachment is obsolete: true
Attachment #515533 - Flags: review?(neil)
Attachment #515415 - Flags: ui-review?(stefanh)
Attachment #515415 - Flags: review?(neil)
Changes since Switch from toolbar id to class patch v1.0:
* Changed class used to "format-bar-class"
Attachment #515415 - Attachment is obsolete: true
Attachment #515534 - Flags: ui-review?(stefanh)
Attachment #515534 - Flags: review?(neil)
(In reply to comment #25)
> (In reply to comment #23)
> > (In reply to comment #18)
> > > (In reply to comment #17)
> > > > (From update of attachment 515407 [details] [diff] [review])
> > > > >+  <toolbarbutton id="AlignPopupButton"
> > > > >+                 class="toolbarbutton-1"
> > > > Why the class?
> > > I have another patch coming up that switches the style sheet from using the
> > > toolbar id to using the toolbarbutton/item class.
> > OK, but it was a really bad choice of class. (The latest patch is still a bad
> > choice of class, since it's intended for use with chrome flags. So you should
> > really think up a new class name.)
> editorPrimaryToolbar.css already uses toolbarbutton-1 to assign a
> list-style-image or are you saying this is wrong?
That's different, since those are primary toolbar buttons... the ones that have traditionally been the large images above text. Although eventually I guess we could get rid of that class, since all toolbarbuttons will work the same in the end, the only difference being the toolbar modes.

(In reply to comment #26)
> (In reply to comment #24)
> > (From update of attachment 515415 [details] [diff] [review])
> > > #FormatToolbar > toolbarbutton > .toolbarbutton-text {
> > >   display: none;
> > > }
> > So, are you planning on giving the buttons text and setting the format toolbar
> > to default to icons only?
> I plan on changing the above with the actual customisation patch.
Right, it was just helpful for you to clarify that.
Comment on attachment 515534 [details] [diff] [review]
Switch from toolbar id to class patch v1.1

>+toolbarbutton.format-bar-class {
"-class" from the Department of Redundancy department :-)
Mind you, my original suggestion was toolbarbutton.formatting-button ;-)
Attachment #515534 - Flags: review?(neil) → review+
Comment on attachment 515533 [details] [diff] [review]
toolbar item tidy up v1.3

>+                 class="chromeclass-toolbar-additional format-bar-class"
OK I'm confused. Why do you need the chromeclass-toolbar-additional class?
Comment on attachment 515533 [details] [diff] [review]
toolbar item tidy up v1.3

r=me if you remove the chromeclass-additional-toolbar class. (I'll let you decide what you want to do with the format-bar-class class.)
Attachment #515533 - Flags: review?(neil) → review+
(In reply to comment #13)
> Created attachment 515238 [details] [diff] [review]
> Tidy up some broadcasters and commandsets v2.1
> 
> Changes since v2:
> * Moved commands from broadcasterset to commandset.
> * Overlay using editorCommands rather than tasksCommands.
> * Moved entity from editorOverlay.dtd to editor.dtd.
> * Changed id for broadcastersets.

I've noticed the debugQATextEditor.xul uses <commands> rather than <commandset>, is it best to change that?
Changes since toolbar item tidy up v1.3:
* Removed chromeclass-additional-toolbar class.
* Renamed format-bar-class class to formatting-button.
Attachment #515533 - Attachment is obsolete: true
Attachment #515745 - Flags: review+
Attachment #515534 - Flags: ui-review?(stefanh)
Changes since Switch from toolbar id to class patch v1.1:
* Renamed class format-bar-class to formatting-button.
Attachment #515534 - Attachment is obsolete: true
Attachment #515748 - Flags: ui-review?(stefanh)
Attachment #515748 - Flags: review+
Comment on attachment 515238 [details] [diff] [review]
Tidy up some broadcasters and commandsets v2.1

>+  <broadcasterset id="editorOverlayBroadcasters">
>+    <broadcaster id="Editor:Throbber" busy="false"/>
>   </broadcasterset>
Not sure it's worth sharing one broadcaster. Also I would just call the set editorBroadcasters (compare nav/mailBroadcasters).

>-  <command id="cmd_showComposeToolbar" oncommand="goDoCommand('cmd_showComposeToolbar')"/>
>-  <command id="cmd_showFormatToolbar" oncommand="goDoCommand('cmd_showFormatToolbar')"/>
>-  <command id="toggleSidebar"/>
You didn't remove the code for these.

>-<broadcasterset id="composeBroadcasters">
>-  <broadcaster id="Editor:Throbber" busy="false"/>
>+<!-- broadcaster nodes are appended here from the overlays -->
>+<broadcasterset id="editorOverlayBroadcasters">
I don't think Editor:Throbber belongs in this window. Mind you, the compose throbber doesn't seem to be hooked up to the progress listener, either directly or via the (missing) broadcaster_throbber element.

>                   <menuitem id="menu_showComposeToolbar"
>                             type="checkbox"
>                             label="&showComposeToolbarCmd.label;"
>                             accesskey="&showComposeToolbarCmd.accesskey;"
>-                            command="cmd_showComposeToolbar"
>-                            checked="true"/>
>+                            command="cmd_viewComposeToolbar"/>
>                   <menuitem id="menu_showFormatToolbar"
>                             type="checkbox"
>                             label="&showFormatToolbarCmd.label;"
>                             accesskey="&showFormatToolbarCmd.accesskey;"
>-                            command="cmd_showFormatToolbar"
>-                            checked="true"/>
>+                            command="cmd_viewFormatToolbar"/>
For a moment I thought this was going to be a problem with persistence, but I think it's only a niggle for someone with a hidden toolbar (the checkbox will reappear until the toolbar is toggled twice.)
Attachment #515238 - Flags: review?(neil) → review-
(In reply to comment #36)
> Mind you, the compose throbber doesn't seem to be hooked up to the progress
> listener, either directly or via the (missing) broadcaster_throbber element.
Filed bug 637630.
Comment on attachment 515748 [details] [diff] [review]
Switch from toolbar id to class patch v1.2 [Checked in: Comment 44]

jftr, I will try to get to this in 1-2 days from now
Changes since Tidy up some broadcasters and commandsets v2.1:
* Removed Editor:Throbber broadcaster from overlay.
* Removed js code relating to cmd_showComposeToolbar and cmd_showFormatToolbar.
* Gave broadcastersets different ids.
* Changed from commands to commandsets in debugQATextEditorShell.xul.
Attachment #515238 - Attachment is obsolete: true
Attachment #516079 - Flags: review?(neil)
Comment on attachment 516079 [details] [diff] [review]
Tidy up some broadcasters and commandsets v2.2 [Checked in: Comment 42]

Note to self: make -C suite/debugQA chrome doens't work.
Attachment #516079 - Flags: review?(neil) → review+
Comment on attachment 515748 [details] [diff] [review]
Switch from toolbar id to class patch v1.2 [Checked in: Comment 44]

+toolbarbutton.formatting-button {

Do you have .formatting-button's that are not toolbarbuttons (not my call really, just curious on why you need the extra selector)?
Attachment #515748 - Flags: ui-review?(stefanh) → ui-review+
Comment on attachment 516079 [details] [diff] [review]
Tidy up some broadcasters and commandsets v2.2 [Checked in: Comment 42]

http://hg.mozilla.org/comm-central/rev/369ad40b3fc1
Attachment #516079 - Attachment description: Tidy up some broadcasters and commandsets v2.2 → Tidy up some broadcasters and commandsets v2.2 [Checked in: Comment 42]
Comment on attachment 515745 [details] [diff] [review]
toolbar item tidy up v1.4 [Checked in: Comment 43]

http://hg.mozilla.org/comm-central/rev/a4afd59a66b2
Attachment #515745 - Attachment description: toolbar item tidy up v1.4 → toolbar item tidy up v1.4 [Checked in: Comment 43]
Comment on attachment 515748 [details] [diff] [review]
Switch from toolbar id to class patch v1.2 [Checked in: Comment 44]

http://hg.mozilla.org/comm-central/rev/118029396d1b
Attachment #515748 - Attachment description: Switch from toolbar id to class patch v1.2 → Switch from toolbar id to class patch v1.2 [Checked in: Comment 44]
(In reply to comment #41)
> Comment on attachment 515748 [details] [diff] [review]
> Switch from toolbar id to class patch v1.2 [Checked in: Comment 44]
> 
> +toolbarbutton.formatting-button {
> 
> Do you have .formatting-button's that are not toolbarbuttons (not my call
> really, just curious on why you need the extra selector)?

There are toolbaritems which have the formatting-button class, but neither of our themes style them, but that doesn't mean a theme out there won't want to.
Comment on attachment 515745 [details] [diff] [review]
toolbar item tidy up v1.4 [Checked in: Comment 43]

>diff --git a/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd b/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd

>-<!ENTITY alignjustifyButton.tooltip "Align text along left and right margins">
>+<!ENTITY alignJustifyButton.tooltip "Align text along left and right margins">

Please be careful changing editor/ - this change broke the Thunderbird tree (MozMill tests) as this caused a missing string in our version of editorOverlay.xul.

(I'd also query the real benefit of changing the string name like this, as although it gives a prettier name, it also causes more work for l10n).

I fixed the Thunderbird tree with this: http://hg.mozilla.org/comm-central/rev/c2ea4943b532
Depends on: 638964
When I was doing the other css changes for switching from toolbar id to class, it looks like I missed one.
Attachment #517045 - Flags: review?(neil)
Comment on attachment 517045 [details] [diff] [review]
Fix css for formatting-button menulist v1.0 [Checked in: Comment 49]

>+.formatting-button menulist:-moz-lwtheme {
Please can you insert a > as we know the menulist will always be a direct child of the toolbaritem in this case. r=me with that fixed.
Attachment #517045 - Flags: review?(neil) → review+
Comment on attachment 517045 [details] [diff] [review]
Fix css for formatting-button menulist v1.0 [Checked in: Comment 49]

Checked in with > added.

http://hg.mozilla.org/comm-central/rev/7c2bd99a597d
Attachment #517045 - Attachment description: Fix css for formatting-button menulist v1.0 → Fix css for formatting-button menulist v1.0 [Checked in: Comment 49]
clearing status request; IanN if this is close to being ready and has no remaining l10n impact, feel free to re-request.
Depends on: 694027
Depends on: 849359

This patch:

  • Removes now unneeded prefs and prefs UI
  • Adds code for showing / hiding UI when toolbars are being customised in Composer / Text Editor
  • Makes EditToolbar's elements removable
  • Centralises popups in editorOverlay.xul
  • Adds cut, copy, past and find as optional buttons for Text Editor (find doesn't work - see Bug 1628412)
  • Lets small icons be used in Composer / Text Editor for Classic Theme
Attachment #485506 - Attachment is obsolete: true
Attachment #9139207 - Flags: review?(frgrahl)
Blocks: 1628873
Depends on: 1628905
Blocks: 1628928
Comment on attachment 9139207 [details] [diff] [review]
Make EditToolbar customisable
 [Checked in: Comment 54]
Target 2.74a1 2.53.3

LGTM Previous comments from version 0.1 seem all the be addressed.

A few notes/NITs:
> Lets small icons be used in Composer / Text Editor for Classic Theme

In Modern gives you a small SeaMonkey throbber and normal composer icons. Add small Modern icons in a follow-up I think. 

> editor/ui/composer/content/editorOverlay.xul
> -    <command id="cmd_viewCompToolbar"     oncommand="goToggleToolbar('EditToolbar','cmd_viewCompToolbar');"         checked="true"/>

While we touch the code I would reformat and line up vertically. Edit 28.04. Given that it was a removal ignore and lets leave the line below alone.

Formatting in the XUL files in now inconsistent but you probably know and want to prerserve "BLAME". This would need a full reformat and can be done later when the codebase in better shape.
Attachment #9139207 - Flags: review?(frgrahl)
Attachment #9139207 - Flags: review+
Attachment #9139207 - Flags: approval-comm-release+
Attachment #9139207 - Flags: approval-comm-esr60+
Attached patch Make FormatToolbar customisable (obsolete) — Splinter Review

This patch:

  • Removes unneeded prefs, prefs UI and prefs panel
  • Makes FormatToolbar's elements removable
  • Adds labels/titles to FormatToolbar's elements so they have names in the customize Toolbar dialog
  • Adds hideinmenu attribute so toolbars can be hidden in the context menu
  • Remove unused helper functions ShowHideToolbarSeparators and ShowHideToolbarButtons

There are string removals (17) and additions (24) in this patch.

Attachment #9144499 - Flags: review?(frgrahl)
Attachment #9144499 - Flags: approval-comm-release?
Attachment #9144499 - Flags: approval-comm-esr60?
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/6da7d36ce03e
Allow customization of toolbars in Composer and formatting toolbar in MailNews Composition - EditToolbar part. r=frg DONTBUILD
Attachment #9139207 - Attachment description: Make EditToolbar customisable → Make EditToolbar customisable [Checked in: Comment 54] Target 2.74a1 2.53.3
Comment on attachment 9144499 [details] [diff] [review]
Make FormatToolbar customisable

As discussed. Error with plaintext repliesForwards:

>Timestamp: 5/3/2020, 6:20:39 PM
>Error: document.getElementById(...) is null
>Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js Line: 1312
Attachment #9144499 - Flags: review?(frgrahl)
Attachment #9144499 - Flags: review-
Attachment #9144499 - Flags: approval-comm-release?
Attachment #9144499 - Flags: approval-comm-esr60?

Changes since last version:

  • Fixes issue when changing format or composing in plain text (including to newsgroups)
Attachment #9144499 - Attachment is obsolete: true
Attachment #9145311 - Flags: review?(frgrahl)
Attachment #9145311 - Flags: approval-comm-release?
Attachment #9145311 - Flags: approval-comm-esr60?
Comment on attachment 9145311 [details] [diff] [review]
Make FormatToolbar customisable v1.1
 [Checked in: Comment 58]
Target 2.75a1 2.53.3

LGTM. 
Just be aware that this introduces a possible problem. The user can move formatting toolbar items to the mail toolbar and these will not be disabled/hidden then. I think we can live with it and can look into it in a follow-up bug.
Attachment #9145311 - Flags: review?(frgrahl)
Attachment #9145311 - Flags: review+
Attachment #9145311 - Flags: approval-comm-release?
Attachment #9145311 - Flags: approval-comm-release+
Attachment #9145311 - Flags: approval-comm-esr60?
Attachment #9145311 - Flags: approval-comm-esr60+
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/e66503accfba
Allow customization of toolbars in Composer and formatting toolbar in MailNews Composition - formatting toolbar part. r=frg DONTBUILD

See you in a follow-up

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Attachment #9145311 - Attachment description: Make FormatToolbar customisable v1.1 → Make FormatToolbar customisable v1.1 [Checked in: Comment 58] Target 2.75a1 2.53.3
Keywords: leave-open
Whiteboard: SM2.53.3

Target 2.53.3
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/d56c427fa432aef93b3b5766083dc236b37f5ffe
Allow customization of toolbars in Composer and formatting toolbar in MailNews Composition - formatting toolbar part. r=frg a=frg

https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/cc63f354deafae7a2147e25b82e2289386dce2c4
Allow customization of toolbars in Composer and formatting toolbar in MailNews Composition - EditToolbar part. r=frg a=frg

Target Milestone: --- → seamonkey 2.75
You need to log in before you can comment on or make changes to this bug.