Closed Bug 1535265 Opened 6 years ago Closed 6 years ago

Reduce the number of unnecessarily-customisable toolbars

Categories

(Thunderbird :: Toolbars and Tabs, enhancement)

Product:
Thunderbird
Component:
Toolbars and Tabs
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(1 file, 6 obsolete files)

1535265-toolbars-6.diff
99.19 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review

We've got way too many toolbars and many of them are not even used as a bar of tools.

The message header toolbar is an odd place to begin since it actually has things that someone might customise, but that's where I began.

Attached patch 1535265-toolbars-1.diff (obsolete) — — Splinter Review

In this patch nine toolbars have been converted away from <toolbar>.

  • Task actions toolbar
  • iMIP toolbar
  • Folder pane toolbar (removed completely)
  • Content tab toolbar
  • Chrome tab toolbar
  • Gloda tab toolbar
  • Attachments toolbar
  • Message display toolbar (single-message and multi-message)
Attachment #9051137 - Attachment is obsolete: true
Attachment #9051449 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9051449 [details] [diff] [review] 1535265-toolbars-1.diff Review of attachment 9051449 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/multimessageview.xhtml @@ +34,5 @@ > src="chrome://messenger/content/multimessageview.js"/> > </head> > <body> > <div id="heading_wrapper"> > + <vbox id="header-view-toolbox" class="inline-toolbox" doesn't this need to be xul:vbox? ::: mail/themes/linux/mail/messenger.css @@ +330,5 @@ > fill: var(--lwt-toolbarbutton-icon-fill, currentColor); > fill-opacity: var(--toolbarbutton-icon-fill-opacity); > } > > +[labelalign="end"] .toolbarbutton-1, I'm concerned simply macthing on attribute value for ALL element might be quite expensive. Can we restrict it to the one(s) you use?
Comment on attachment 9051449 [details] [diff] [review] 1535265-toolbars-1.diff Review of attachment 9051449 [details] [diff] [review]: ----------------------------------------------------------------- Other than that, looks good. r=mkmelin
Attachment #9051449 - Flags: review?(mkmelin+mozilla) → review+
Attached patch 1535265-toolbars-2.diff (obsolete) — — Splinter Review

Richard, please check the toolbars listed in comment 2 and see if they still look alright. I've removed a lot of fluff from the CSS that I don't think is used any more after this change.

Attachment #9051449 - Attachment is obsolete: true
Attachment #9053205 - Flags: ui-review?(richard.marti)
Attachment #9053205 - Flags: review+

Wait, what? Dow do you determine which toolbar is no benefit over vbox? Also why is the folder pane toolbar removed? How are users then supposed to put items there?

Comment on attachment 9053205 [details] [diff] [review] 1535265-toolbars-2.diff r- because: - the Folder Pane toolbar is gone. - the header view toolbar is no more customizable and now tooo tall with icons above text. - the attachment toolbar is also too tall. Please set text beside icon like before the patch. - the multimessage header buttons are too tall too. - the IMIP bar buttons are also too tall. - the task actions toolbar buttons have the same issue. Maybe it should still be possible to configure how the buttons should look (text only, icon only, icon above text or icon beside text). And please let the Folder Pane toolbar and the header view toolbar customizable like they are.
Attachment #9053205 - Flags: ui-review?(richard.marti) → ui-review-

(In reply to :aceman from comment #6)

Also why is the folder pane toolbar removed?
How are users then supposed to put items there?

They are not. Why on earth would anyone do that?

Maybe some background is useful here. One of the problematic xbl removals still to do is related to toolbar.xml. This is all code that didn't over the years move to do similar things as what Firefox does. Like how Thunderbird's customization is very very different (and for many things buggy) compared to the Firefox one.

We have a bunch of things that are customizable toolbars "just because". There is no end use value in being able to customize e.g. the attachment bar. Even the main toolbars do not really give much customization value: look at compose - what you can add from there is just nonsense.

So I think the direction should be toward customizable toolbars more like what Firefox has (not sure if it can be exactly the same). But for the UI it should also be more about providing sensible buttons and focusing on providing real features, not keeping back developments by keeping too much distraction code such as letting people add Copy, Paste, or a Print button.

As for the header view toolbar, that I can imagine someone wanting to customize - but I still think providing sensible defaults there will keep almost everyone happy.

(In reply to Magnus Melin [:mkmelin] from comment #8)

(In reply to :aceman from comment #6)

Also why is the folder pane toolbar removed?
How are users then supposed to put items there?
They are not. Why on earth would anyone do that?

Because it was specifically added for the purpose. It is empty by default, but you can put e.g. folder mode picker there.
See bug 700976.

So I think the direction should be toward customizable toolbars more like what Firefox has (not sure if it can be exactly the same). But for the UI it should also be more about providing sensible buttons and focusing on providing real features, not keeping back developments by keeping too much distraction code such as letting people add Copy, Paste, or a Print button.

OK, but then make that technical decision publicly and in wider group, not in a random cleanup bug falsely claiming they are unused. They are used and useful. If we have to drop them, at least make it clear where the blame lies.

I told you there'd be complaints. :-P

(In reply to Richard Marti (:Paenglab) from comment #7)

  • the header view toolbar is no more customizable and now tooo tall with
    icons above text.
  • the attachment toolbar is also too tall. Please set text beside icon like
    before the patch.
  • the multimessage header buttons are too tall too.
  • the IMIP bar buttons are also too tall.
  • the task actions toolbar buttons have the same issue.

Huh. I had them looking as expected. Maybe I screwed something up again afterwards.

Okay, it's a platform thing. I did think a lot of that CSS was oddly in platform-specific folders.

Attached patch 1535265-toolbars-3.diff (obsolete) — — Splinter Review

Now with the styling applied to all platforms, and actually passing mozmill.

Attachment #9053205 - Attachment is obsolete: true
Comment on attachment 9053468 [details] [diff] [review] 1535265-toolbars-3.diff Looks better now. I'm missing the Folder pane toolbar as I used it to switch the folder views. How about only show this item on this "toolbar"? And made the "toolbar" hideable as it is now. In the message header "toolbar" on messages with multiple recipient, the "Reply" and the "Smart Reply" buttons are shown. Couldn't we show only the "Smart Reply" button which also has the "Reply" option?

(In reply to :aceman from comment #9)

Because it was specifically added for the purpose. It is empty by default,
but you can put e.g. folder mode picker there.

Talk about non-intuitive UI. I would not have figured this out.

OK, but then make that technical decision publicly and in wider group, not
in a random cleanup bug falsely claiming they are unused.

Haven't the follow-Firefox-as-much-as-possible policy been in place forever? I don't think this bug ever said unused. Unneeded is something else.

Attachment #9053468 - Flags: review?(mkmelin+mozilla)
Attached patch 1535265-toolbars-4.diff (obsolete) — — Splinter Review

Now with the folder pane toolbar hard-wired.

Attachment #9053468 - Attachment is obsolete: true
Attachment #9053468 - Flags: review?(mkmelin+mozilla)
Attachment #9056493 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9056493 [details] [diff] [review] 1535265-toolbars-4.diff Review of attachment 9056493 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/messenger.xul @@ +451,5 @@ > > <box id="messengerBox" orient="horizontal" flex="1" minheight="100" height="100" persist="height"> > <vbox id="folderPaneBox" minwidth="125" width="200" persist="width"> > <sidebarheader id="folderPaneHeader" hidden="true" align="center"/> > + <vbox id="folderPane-toolbox"> it it slightly odd that the Customize context menu still shows, but that will allow customizing the main toolbar it seems ::: mail/base/content/msgHdrView.inc.xul @@ +18,5 @@ > fit alongside each other. --> > <hbox id="expandedHeadersTopBox" flex="1"> > > + <vbox id="header-view-toolbox"> > + <hbox id="header-view-toolbar"> I think we need to add more of the items to Other actions, if they aren't going to be available. At least tag, Add as event and Add as task
Attachment #9056493 - Flags: review?(mkmelin+mozilla)
Attached patch 1535265-toolbars-5.diff (obsolete) — — Splinter Review
Attachment #9056493 - Attachment is obsolete: true
Comment on attachment 9057156 [details] [diff] [review] 1535265-toolbars-5.diff r? for the latest changes and then I think this is ready to go.
Attachment #9057156 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9057156 [details] [diff] [review] 1535265-toolbars-5.diff Review of attachment 9057156 [details] [diff] [review]: ----------------------------------------------------------------- r=mkmelin
Attachment #9057156 - Flags: review?(mkmelin+mozilla) → review+

If you remove the disabled tests in test-header-toolbar.js, you will lose the ability to reproduce the crash from bug 1530207.

Let's see if we get that fixed first.

Depends on: 1530207

I've just worked out that the changes to mail/test/mozmill/message-header/test-header-toolbar.js remove all of the tests from that file, so the file itself might as well be removed too.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/806cef92b2a0
Stop using <toolbar> where it provides no benefit over <hbox>; r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/661294aa1fcd follow-up - fix linting errors; rs=me DONTBUILD

https://hg.mozilla.org/comm-central/rev/220013624fc6ff0751026eec71c6dad2a55585bd
Backed out 2 changesets (bug 1535265) for test failures. r=backout

Sorry, but this made the tree real colourful. I'd say the Z3 is certainly caused here, maybe the Z2 too. That failed before on Windows, and that might be related to M-C action:
2ccc66480643 Andreea Pavel — Backed out changeset 643f81697dae (bug 1444447) because it causes graphical corruption in menus, see bug 1444447 comment 25 a=backout

In chronological order:
Push without this patch:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=13e57aaba5a0119ca8658b1f22202bf7e3d02b39
Push with this patch:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=e72379771e3f31c9099175254bdde76d48458556
Push without this patch after backout:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=220013624fc6ff0751026eec71c6dad2a55585bd

I don't see a try run for this bug and you weren't available on IRC, it's Good Friday after all, hence the backout.

That returned the tree to the "usual" failures.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Okay, I fixed the brokenness. Now with a recent Try run instead of the out-of-date one that had me thinking there was a recent one.

Attachment #9057156 - Attachment is obsolete: true
Attachment #9059980 - Flags: review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3bdcf6d1a445
Stop using <toolbar> where it provides no benefit over <hbox>; r=mkmelin

Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Is there a reason the message header toolbar in particular was rendered non-customizable in this patch? It's probably the single most important toolbar to customize, since it has all the buttons for operating on a single message, and is in a much more space-constrained environment than, say, the main toolbar. This is especially important for non-English locales, where the button text is often longer than English. Previously, you could switch to "icons only" or remove some buttons you don't need, but with this patch, there appears to be nothing you can do. Even in English, it causes problems the Vertical 3-pane layout unless you make the message pane pretty wide.

I can understand removing some of the other toolbars (though I don't like it, since part of the reason for those being customizable was for easy extension hooks), but not the message header toolbar. Since Thunderbird still has one customizable toolbar - the main one - it's not like we can totally get rid of the <toolbar> element anyway. The additional cost/pain of keeping 2 <toolbar>s around seems pretty minimal compared to keeping 1 around.

So given Jim's comment and the discussion on the Maildev list, are we going to revert some of this stuff?

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(geoff)

I have no preference either way, and so I'm leaving this debate to the person who requested the change in the first place.

Flags: needinfo?(geoff)
Blocks: 1553427

It's true the message header toolbar is more of a toolbar than the other ones.

Let's continue over in bug 1553427.

Flags: needinfo?(mkmelin+mozilla)
Regressions: 1556122
Blocks: 1556261
Regressions: 1556936
Regressions: 1559601
Regressions: 1669063
Blocks: 1425656
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: