1437305 - Implement "Initially show attachment pane" pref and UI for composition (mirroring message reader), e.g. as a more explicit drop target

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[ruga]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20100101

Steps to reproduce:

Select the icon to attach a file. 


Actual results:

A detached window appears, with navigation. After the first attachment, the compose window shows a new built-in window with the attachment list. The user can add new attachments by drag and drop into it. 


Expected results:

Users complain that the attachment machinery above is too much labour for something that can be simpler. The compose window could show the built-in attachment window by default. The legacy attachment machinery could be resumed by an about:config parameter.

Comment 1

[Jorg K (CEST = GMT+2)]

Thomas, what do you think? Reporter, we're looking at some improvements for the attachment pane (internally called lovingly "attachment bucket", you called it "new built-in window with the attachment list"). See bug 1428631.

Comment 2

[Alfred Peters [:infofrommozilla]]

(In reply to ruga from comment #0)

> Steps to reproduce:
> Select the icon to attach a file. 

> Expected results:
> 
> Users complain that the attachment machinery above is too much labour for
> something that can be simpler. The compose window could show the built-in

There is no need to click that icon.
It is enough to drag and drop a file onto the Header pane of the Composer window.
The 'built-in attachment window' then opens automatically.

Just my 2ct

Comment 3

[Jorg K (CEST = GMT+2)]

And bug 1428631 will give us an option to open the attachment bucket, even if it's empty to make it even clearer.

Comment 4

[Thomas D. (:thomas8)]

We should definitely give users an option to start compositions with an empty attachment bucket open, which makes a lot of sense if you add attachments more often than not. I imagine that could be realized in pretty much the same way as we have it for received messages, context menu entry "Initially Show Attachment Pane" when right-clicking on bucket header (I could do that in Bug 707432), hooked up with a pref.

As for drag and drop, it was mentioned somewhere by Aceman I think that we should allow the entire compose window as a drop target for adding attachments; I like that idea, too (no bug yet afasics).

Bug 1428631 (wip) will alleviate but not fully address the scenario of this bug as it will make it easy to summon the empty bucket with Alt+M keyboard shortcut.

Comment 5

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere) from comment #4)
That said, and in spite of all the sweat and pain I've already invested in the "Minimize Attachment Pane" feature requested by Jörg in Bug 1428631, I'm still suspecting that the final shape of the bucket should be this:
- Relocate "Attach | v" button into the position of current bucket header, and always show it
- Add a twisty before the button, allowing users to hide and show the bucket (also when clicking anywhere in the bucket header except for the button, same as for message reader attachment pane). So the bucket header is always there (next to from), and bucket can be toggled below that according to users needs (both empty and full).
- With attachments, button label updates to "N attachments" but functionality of button remains same, including dropdown. We allow hiding full bucket as the "N attachments" header will always be there and indicate the presence of attachments.
- When hovering "N attachments" button part, label dynamically changes to "Attach | v"
I think this integrated approach will ultimately simplify the UI and feel better than the current "minimize" approach of Bug 1428631. I thought "minimize" would be less code than integrated remake of the entire header box model, but with the benefit of hindsight, maybe I was wrong...
If we could succeed to relocate the attachment button to the right, that might be a good step to take our users along on the way to integrated approach.

Comment 6

[ruga]

The attachment bucket may be visible by default, and disabled via about:config. This would solve the core problem, with no need to wait for the necessary discussion, decision, and implementation of a method to show or hide the attachment bucket using the GUI.

Comment 7

[Thomas D. (:thomas8)]

Attachment: Patch V.1

Easy!

For users who add attachments more often than not, let's give them a choice to start compositions with expanded attachment pane.

This implements the pref and the most minimalist contextual UI for mouse users.
We already have the same pref and UI for message reader (right-click on attachment header bar), so this is the logical twin for composition.

It's not keyboard accessible from the main UI, but then, for the one time that a  keyboard-dependent user might want set this, he would have to go through config editor.

// Start compositions with expanded attachment pane
pref("mail.compose.attachments.display.start_expanded", false);

Aceman, for any given locale, the wording of the new string here must be exactly the same as their wording for
https://dxr.mozilla.org/comm-central/rev/a8eecfe6de793af00e98cb1488515199c5fb73fb/mail/locales/en-US/chrome/messenger/msgHdrViewOverlay.dtd#75
> <!ENTITY startExpandedCmd.label    "Initially Show Attachment Pane">
> <!ENTITY startExpandedCmd.accesskey    "S">

Instead of asking localizers to translate this again (and possibly end up with different wording), is there a way we could just copy the existing string into the new string for all locales? Do we have a script for such a task?

Comment 8

[Jorg K (CEST = GMT+2)]

Does this depend on bug 1428631 (or invalidate its final patch)? It's too late for string changes, but we could do it without the string changes so power users can set the pref. And no, there is absolutely nothing smart in l10n to my knowledge.

From an UI point of view I'm not sure whether starting every new message with an empty bucket showing is desirable. On what percentage of messages do you send an attachment, in general? If at all, I'd hide this option.

Comment 9

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #8)
> Does this depend on bug 1428631 (or invalidate its final patch)?

No dependency. No problem for bug 1428631, 2 rejected hunks which are trivial, and we'll have another iteration there.

> It's too
> late for string changes, but we could do it without the string changes so
> power users can set the pref.

Even if it may be too late for string changes, I believe this could still land with strings in one of two ways:
We already have this string (exactly same wording), so
1) either we can just create the new string for all locales and copy their existing string (no new translation required)
2) or we can just use the existing string for the release version and land the new string on daily where it will be re-translated in due course.
I'd actually prefer 1) regardless of string freeze because from experience, localizers can do all sorts of unexpected translations whereas here we really want two identical strings, so it'll be the safest option if we just copy their old string into the new string.

> And no, there is absolutely nothing smart in
> l10n to my knowledge.

That's sad.

> From an UI point of view I'm not sure whether starting every new message
> with an empty bucket showing is desirable. On what percentage of messages do
> you send an attachment, in general? If at all, I'd hide this option.

That's why the pref defaults to false, no changes unless you need it. You and I won't need it, but looking at the user story of this bug in comment 0, there might be others who need this. E.g. billing department sending invoices the whole day, then they want to use drag and drop with a more obvious drop target (as reported in this bug). So depending on scenarios and workflows, this is certainly helpful as an option.

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 8958726 [details] [diff] [review]
Patch V.1

I see no context menu on the attachment pane's header. No error in console visible.

Comment 11

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #10)
> Comment on attachment 8958726 [details] [diff] [review]
> Patch V.1
> 
> I see no context menu on the attachment pane's header. No error in console
> visible.

As in message reader, you can only get it on blank space of the header bar, not on text.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8958726 [details] [diff] [review]
Patch V.1

As Richard said, as hard as we click onto the attachment pane header, there is no context menu. Looks like onpopupshowing is missing in the XUL, no? I'm no XUL expert.

Setting the preference manually works. I'm not sure that putting this on the context menu on the header is the easiest way to discover this. Why would you expect it there?

I'd accept a patch with no UI, just the preference for the people from the billing department ;-) But let's see it working as intended first.

Comment 13

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #12)
> Comment on attachment 8958726 [details] [diff] [review]
> Patch V.1
> 
> As Richard said, as hard as we click onto the attachment pane header, there
> is no context menu.

Indeed, I forgot to copy the entire menu tag over into comm-central XUL.

> Setting the preference manually works. I'm not sure that putting this on the
> context menu on the header is the easiest way to discover this. Why would
> you expect it there?

Because we already have the same for message reader. If I want to customize an element or its behaviour, I think it's reasonable to try right-clicking on it. To improve discovery furthermore, we'd probably need a checkbox somewhere in Tools > Options, assuming we may not want this rarely-used option in the main menus.

> I'd accept a patch with no UI, just the preference for the people from the
> billing department ;-) But let's see it working as intended first.

It's better to land just the preference than nothing at all, but otoh the pref without any UI is all but undiscoverable so let's try to land this with minimal UI at least. As I explained in comment 9, we can find a way to have the string without requiring new translations, so this is independent of string freeze.

Comment 14

[Thomas D. (:thomas8)]

Attachment: Patch V.1.1

Per comment 9, we can utilize the existing string in one way or the other, so this does not depend on string freeze.

Minimal UI is exactly same as message reader (ux-consistency).

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8959467 [details] [diff] [review]
Patch V.1.1

This works, I can't find a fault in the code. That said, I don't think the right-click on the white-space in the header is great.

And I certainly won't get into any l10n complications over this.

Aceman, what do you think of this? And Richards UI review is also still missing.

Comment 16

[Richard Marti (:Paenglab)]

Comment on attachment 8959467 [details] [diff] [review]
Patch V.1.1

With this we are consistent with the attachment pane in the reader view.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8959467 [details] [diff] [review]
Patch V.1.1

(In reply to Richard Marti (:Paenglab) from comment #16)
> With this we are consistent with the attachment pane in the reader view.
I didn't even know that existed :-(

Comment 18

[Thomas D. (:thomas8)]

Here's the existing strings - are we able to do patches against the localization repository?
The list looks short enough to just copy them over into the new string.

https://dxr.mozilla.org/l10n-central/search?q=startExpandedCmd.label&redirect=true

Comment 19

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #18)
> Here's the existing strings - are we able to do patches against the
> localization repository?

Apparently yes:
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Patching_a_Localization

> The list looks short enough to just copy them over into the new string.
> 
> https://dxr.mozilla.org/l10n-central/search?q=startExpandedCmd.
> label&redirect=true

Comment 20

[Richard Marti (:Paenglab)]

Instead of such things you could also rename in messengercompose.xul the label="&initiallyShowAttachmentPane.label;" to label="&startExpandedCmd.label;", the same naturally to the accesskey, and add the msgHdrViewOverlay.dtd to the messengercompose.xul.

Like this, no dangerous things are done in the locales.

Comment 21

[:aceman]

Comment on attachment 8959467 [details] [diff] [review]
Patch V.1.1

Review of attachment 8959467 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/app/profile/all-thunderbird.js
@@ +536,5 @@
>  // Number of recipient rows shown by default
>  pref("mail.compose.addresswidget.numRowsShownDefault", 3);
>  
> +// Start compositions with expanded attachment pane
> +pref("mail.compose.attachments.display.start_expanded", false);

We have no mail.compose.attachments. pref tree yet so this seems to be overkill to add 2 new levels (also 'display') here.
The current style of attachment prefs is 
"mail.compose.attachment_pane_display_initially".

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5336,5 @@
> +  let initiallyShowItem =
> +    document.getElementById("attachmentHeaderContext_initiallyShowItem");
> +  let showInitially = Services.prefs.getBoolPref(
> +    "mail.compose.attachments.display.start_expanded");
> +  initiallyShowItem.setAttribute("checked", showInitially);

Isn't the attribute set automatically on clicking the item? Do you really need to set it on popupshowing? I'd assume you could just set the starting value in ComposeStartup (where you show the pane now).

::: mail/components/compose/content/messengercompose.xul
@@ +1223,5 @@
>            <hbox id="attachments-header-box"
>                  align="center"
>                  tooltiptext="&attachmentBucketHeader.tooltip;"
> +                onclick="attachmentBucketHeaderOnClick();"
> +                context="attachmentHeaderContext">

The context menu is only shown on the very narrow part between the "X attachments" label and the X (close) button. Is that intentional? That doesn't seem very discoverable.

Comment 22

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #20)
> Instead of such things you could also rename in messengercompose.xul the
> label="&initiallyShowAttachmentPane.label;" to
> label="&startExpandedCmd.label;", the same naturally to the accesskey, and
> add the msgHdrViewOverlay.dtd to the messengercompose.xul.
> 
> Like this, no dangerous things are done in the locales.

Yes, we could do this for the ESR60 branch version.

Comment 23

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #21)
> > +pref("mail.compose.attachments.display.start_expanded", false);
> We have no mail.compose.attachments. pref tree yet so this seems to be
> overkill to add 2 new levels (also 'display') here.
> The current style of attachment prefs is 
> "mail.compose.attachment_pane_display_initially".
We have mailnews.attachments.display.start_expanded for the attachment pane when reading a message. Maybe that was the motivation.

Comment 24

[:aceman]

But there is also mailnews.attachments.display.view so that is a different situation.

Comment 25

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #21)
> Comment on attachment 8959467 [details] [diff] [review]
> Patch V.1.1

Aceman's reviews have become more critical ;-)

> Review of attachment 8959467 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/app/profile/all-thunderbird.js
> @@ +536,5 @@
> >  // Number of recipient rows shown by default
> >  pref("mail.compose.addresswidget.numRowsShownDefault", 3);
> >  
> > +// Start compositions with expanded attachment pane
> > +pref("mail.compose.attachments.display.start_expanded", false);
> 
> We have no mail.compose.attachments. pref tree yet so this seems to be
> overkill to add 2 new levels (also 'display') here.

Future-proof isn't overkill, and the number of levels doesn't hurt in any way, does it?
And as Jörg says, deliberate pref path/pref name analogy to the message reader pref.
We certainly don't want two totally different pref names and paths for essentially the same pref.

> The current style of attachment prefs is 
> "mail.compose.attachment_pane_display_initially".

That's how we create messy and non-scalable pref systems.

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +5336,5 @@
> > +  let initiallyShowItem =
> > +    document.getElementById("attachmentHeaderContext_initiallyShowItem");
> > +  let showInitially = Services.prefs.getBoolPref(
> > +    "mail.compose.attachments.display.start_expanded");
> > +  initiallyShowItem.setAttribute("checked", showInitially);
> 
> Isn't the attribute set automatically on clicking the item? Do you really
> need to set it on popupshowing? I'd assume you could just set the starting
> value in ComposeStartup (where you show the pane now).

I think it's common practice to update menus with onpopupshowing.
And it's required and helpful for this pref case because when user changes the pref from options, we still want the next use of the context menu to show the right thing, and not lose sync with the pref.

> ::: mail/components/compose/content/messengercompose.xul
> @@ +1223,5 @@
> >            <hbox id="attachments-header-box"
> >                  align="center"
> >                  tooltiptext="&attachmentBucketHeader.tooltip;"
> > +                onclick="attachmentBucketHeaderOnClick();"
> > +                context="attachmentHeaderContext">
> 
> The context menu is only shown on the very narrow part between the "X
> attachments" label and the X (close) button. Is that intentional?

Yes. "5 Attachments" in the header (as well as their size) represents attachments and I'll add the attachment list context menu there.

> That doesn't seem very discoverable.

Safely discoverable would only work in main menus or options.
We show a tooltip "Attachment Pane" when hovering the header including blank space. I think from there it's possible to try right-clicking anywhere if you're searching for more options.
Also, this was a much bigger problem for message reader, and many users complained, but stopped complaining after we implemented that option. So I believe they found it somehow.

Comment 26

[Thomas D. (:thomas8)]

The corresponding message reader bug is bug 647036. Discoverability was probably eased by the fact that the context menu was first shipped and advertised in Jim's addon (5000 users) before coming into core.

Comment 27

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #22)
> (In reply to Richard Marti (:Paenglab) from comment #20)
> > Instead of such things you could also rename in messengercompose.xul the
> > label="&initiallyShowAttachmentPane.label;" to
> > label="&startExpandedCmd.label;", the same naturally to the accesskey, and
> > add the msgHdrViewOverlay.dtd to the messengercompose.xul.
> > 
> > Like this, no dangerous things are done in the locales.

What would be dangerous about patching locales to just copy the existing string into our new string?

> Yes, we could do this for the ESR60 branch version.

https://dxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/msgHdrViewOverlay.dtd

Loading 95 strings to get just one sounds a bit resource-expensive, so we should avoid that for a release version...
I guess our best choice is to just copy the strings directly in the locales, and I don't see any danger with that.

Otherwise, I think the following hack would be less expensive:
- get the existing string somewhere near application startup*, and save it somewhere (global variable, temporary pref).
- get that global variable or temp-pref only when we need it: onpopupshowing of the context menu here

* it looks as if msgHdrViewOverlay.dtd is loaded even when message reader isn't shown (but I'm not sure):
https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWidgets.xml#8

Comment 28

[Jorg K (CEST = GMT+2)]

Thomas, Aceman, can we please wrap this bug up. Are there any issues from comment #21 that need to be addressed?
Re. the pref name: Preferences are a mess in general. We also have mail.compose.addresswidget.numRowsShownDefault, so how about mail.compose.attachments.display_pane_initially?

Once we have this landed, we discuss the possibility to uplift this.

Comment 29

[:aceman]

(In reply to Thomas D. (currently busy elsewhere) from comment #25)
> Aceman's reviews have become more critical ;-)

Because we should be doing polishing bugs, not crash landing enhancements at this time in the release cycle.

> > We have no mail.compose.attachments. pref tree yet so this seems to be
> > overkill to add 2 new levels (also 'display') here.
> Future-proof isn't overkill, and the number of levels doesn't hurt in any
> way, does it?

The number of levels matter, consider them folders. If you have folders X/Y and X/Z, why would you create X/A/B/C to store 1 new file?
Future-proof? So how many tens of new prefs do to expect to create in this new namespace of 3 levels?

> We certainly don't want two totally different pref names and paths for
> essentially the same pref.

I don't think the features are very related. Most user will have one enabled, while the other one not.
 
> > The current style of attachment prefs is 
> > "mail.compose.attachment_pane_display_initially".
> 
> That's how we create messy and non-scalable pref systems.

We didn't create it, we just follow it now. Your counter proposal with 2 new levels is not sound.
How do you propose to migrate existing prefs to mail.compose.attachment.* to cleanup the supposed mess?
 
> > ::: mail/components/compose/content/MsgComposeCommands.js
> > @@ +5336,5 @@
> > > +  let initiallyShowItem =
> > > +    document.getElementById("attachmentHeaderContext_initiallyShowItem");
> > > +  let showInitially = Services.prefs.getBoolPref(
> > > +    "mail.compose.attachments.display.start_expanded");
> > > +  initiallyShowItem.setAttribute("checked", showInitially);
> > 
> > Isn't the attribute set automatically on clicking the item? Do you really
> > need to set it on popupshowing? I'd assume you could just set the starting
> > value in ComposeStartup (where you show the pane now).
> 
> I think it's common practice to update menus with onpopupshowing.

Yes, for menuitems that we need to generate, or where the XUL doesn't provide the feature.
But I think XUL can toggle 'checked' attribute automatically.
Yeah, when the new messy HTML world arrives and we have to open-code all these features, then you will have to reimplement all the tiny details:)

> And it's required and helpful for this pref case because when user changes
> the pref from options, we still want the next use of the context menu to
> show the right thing, and not lose sync with the pref.

Ah, dynamic picking of pref changes of a totally edge case pref. I don't think we need to support this.
 
> > ::: mail/components/compose/content/messengercompose.xul
> > The context menu is only shown on the very narrow part between the "X
> > attachments" label and the X (close) button. Is that intentional?
> 
> Yes. "5 Attachments" in the header (as well as their size) represents
> attachments and I'll add the attachment list context menu there.

What do you mean? You specifically excluded the label and the size label from showing this context menu. Will you change that?

Comment 30

[:aceman]

(In reply to Jorg K (GMT+1) from comment #28)
> Thomas, Aceman, can we please wrap this bug up. Are there any issues from
> comment #21 that need to be addressed?

It seems so.

> Re. the pref name: Preferences are a mess in general. We also have
> mail.compose.addresswidget.numRowsShownDefault, so how about
> mail.compose.attachments.display_pane_initially?

One new level for attachments would not be as bad. There is more of a chance to add some new pref to it, than expecting to populate 2 more levels. Will we add 20 new prefs so it makes sense to group them into at least 4 branches (mail.compose.attachments.display.X, mail.compose.attachments.display.Y, mail.compose.attachments.Z.A, mail.compose.attachments.Z.A)? Without 4 groups it makes not much sense to have so many levels.

Comment 31

[:aceman]

(In reply to Thomas D. (currently busy elsewhere) from comment #27)
> https://dxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/
> messenger/msgHdrViewOverlay.dtd
> 
> Loading 95 strings to get just one sounds a bit resource-expensive, so we
> should avoid that for a release version...

Sure, if you implement a better way. This is just the backup plan that we can do straight away.
All the others need work that we can't afford to do right now.

> I guess our best choice is to just copy the strings directly in the locales,
> and I don't see any danger with that.

Sure, show us the patch. But I don't know who has the commit rights into all those localization directories.
Also don't forget that you will only copy the existing string from ESR60 branch to the new ID and in a different file.
For trunk, we land the string untranslated so that localizers pick it up. They may need to translate it differently than the existing string.
 
> Otherwise, I think the following hack would be less expensive:
> - get the existing string somewhere near application startup*, and save it
> somewhere (global variable, temporary pref).
> - get that global variable or temp-pref only when we need it: onpopupshowing
> of the context menu here

Would sound crazy for a permanent solution, but if it is ~5 lines of code, we may consider it for the 60 branch. Show us the patch :)

Comment 32

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #31)
> (In reply to Thomas D. (currently busy elsewhere) from comment #27)
> > https://dxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/
> > messenger/msgHdrViewOverlay.dtd
> > 
> > Loading 95 strings to get just one sounds a bit resource-expensive, so we
> > should avoid that for a release version...
> 
> Sure, if you implement a better way. This is just the backup plan that we
> can do straight away.

Yeah, maybe needed as a simple way out.

> All the others need work that we can't afford to do right now.
> 
> > I guess our best choice is to just copy the strings directly in the locales,
> > and I don't see any danger with that.
> 
> Sure, show us the patch. But I don't know who has the commit rights into all
> those localization directories.

If it's not one person who could do all, we'd face challenges.

> Also don't forget that you will only copy the existing string from ESR60
> branch to the new ID and in a different file.
> For trunk, we land the string untranslated so that localizers pick it up.
> They may need to translate it differently than the existing string.

Huh??? How can two identical strings at the same UI location, same meaning and context, have two different translations?
That would only be possible when translators deviate strongly from the given string ("Show the attachment pane at start of reading"), meaning that both of their strings would be wrong. But looking at the translations which I an read, they look right.

> > Otherwise, I think the following hack would be less expensive:
> > - get the existing string somewhere near application startup*, and save it
> > somewhere (global variable, temporary pref).
> > - get that global variable or temp-pref only when we need it: onpopupshowing
> > of the context menu here
> 
> Would sound crazy for a permanent solution, but if it is ~5 lines of code,
> we may consider it for the 60 branch. Show us the patch :)

I'm running out of time and energy to work more on these bugs, so maybe the backup plan is best.

Comment 33

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #30)
> (In reply to Jorg K (GMT+1) from comment #28)
> > Thomas, Aceman, can we please wrap this bug up. Are there any issues from
> > comment #21 that need to be addressed?
> 
> It seems so.
> 
> > Re. the pref name: Preferences are a mess in general. We also have
> > mail.compose.addresswidget.numRowsShownDefault, so how about
> > mail.compose.attachments.display_pane_initially?

Yes, that's much better. For consistency, I'd prefer
mail.compose.attachments.display_start_expanded
so that at least searching for start_expanded will return both prefs, which do exactly the same thing albeit in different UI spots.

> One new level for attachments would not be as bad. There is more of a chance
> to add some new pref to it, than expecting to populate 2 more levels. Will
> we add 20 new prefs so it makes sense to group them into at least 4 branches
> (mail.compose.attachments.display.X, mail.compose.attachments.display.Y,
> mail.compose.attachments.Z.A, mail.compose.attachments.Z.A)? Without 4
> groups it makes not much sense to have so many levels.

We don't know what we're going to do in the future. But it doesn't take much to imagine that the display of attachments might need more fine-grained control, even for addons. It's very easy to imagine more prefs:
mail.compose.attachments.auto-zip
mail.compose.attachments.auto-shrink-images
mail.compose.attachments.auto-sort
mail.compose.attachments.display.dock
mail.compose.attachments.display.large-icons
mail.compose.attachments.display.tooltip-preview
mail.compose.attachments.display.viewmode (details, list, preview etc., as in explorer)

So I don't see why we shouldn't make this future-proof now, as it doesn't seem to hurt in any way, it's just a different spelling of the pref, which allows us to use this pref branch if we need it lateron, which does not look unlikely.

mail.compose.attachments.display_start_expanded
mail.compose.attachments.display.start_expanded

What's wrong with the latter, regardless if we'll really add more or not? We might very well, but obviously we can't foretell now...

Comment 34

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #29)
> (In reply to Thomas D. (currently busy elsewhere) from comment #25)
> > We certainly don't want two totally different pref names and paths for
> > essentially the same pref.
> 
> I don't think the features are very related. Most user will have one
> enabled, while the other one not.

Huh? It's exactly the same feature, only in different spots of the UI.
It's not about how the user sets them, it's about having a more systematic pref naming system where the same things shouldn't have different names. I think that's pretty obvious.

> > > The current style of attachment prefs is 
> > > "mail.compose.attachment_pane_display_initially".
> > 
> > That's how we create messy and non-scalable pref systems.
> 
> We didn't create it, we just follow it now. Your counter proposal with 2 new
> levels is not sound.

I didn't see much to follow here. Afasics, this is the first pref of category attachments.display.

> How do you propose to migrate existing prefs to mail.compose.attachment.* to
> cleanup the supposed mess?

We all know that migrating existing prefs isn't advisable, and I never postulated that.
That doesn't mean that we can't start a more organized, future-proof pref hierarchy now.
I assume when Jim created the attachments.display pref branch for message reader, he had things in mind. These things won't be much different for composition.

> > > ::: mail/components/compose/content/MsgComposeCommands.js
> > > @@ +5336,5 @@
> > > > +  let initiallyShowItem =
> > > > +    document.getElementById("attachmentHeaderContext_initiallyShowItem");
> > > > +  let showInitially = Services.prefs.getBoolPref(
> > > > +    "mail.compose.attachments.display.start_expanded");
> > > > +  initiallyShowItem.setAttribute("checked", showInitially);
> > > 
> > > Isn't the attribute set automatically on clicking the item? Do you really
> > > need to set it on popupshowing? I'd assume you could just set the starting
> > > value in ComposeStartup (where you show the pane now).
> > And it's required and helpful for this pref case because when user changes
> > the pref from options, we still want the next use of the context menu to
> > show the right thing, and not lose sync with the pref.
> 
> Ah, dynamic picking of pref changes of a totally edge case pref. I don't
> think we need to support this.

Huh? Edge case or not, there's nothing more confusing than a pref which I change in config editor and which doesn't take effect immediately, but instead results in the pref and the UI losing sync.
And the costs of ensuring sync always are totally zero, I'm failing to see the problem of syncing the menu checkmark from a single boolean pref each time the menu is opened. As you said, it's an edge case pref so it's not like users are doing this every day, and even if they did, a single pref call doesn't slow them down in any way.
So let's please avoid cans of worms where users start filing bugs that they changed the pref from config editor but it doesn't work as it's not shown in the UI. Also in message reader it works exactly like this, I just copied the code.

> > > ::: mail/components/compose/content/messengercompose.xul
> > > The context menu is only shown on the very narrow part between the "X
> > > attachments" label and the X (close) button. Is that intentional?
> > 
> > Yes. "5 Attachments" in the header (as well as their size) represents
> > attachments and I'll add the attachment list context menu there.
> 
> What do you mean? You specifically excluded the label and the size label
> from showing this context menu. Will you change that?

I'd rather not. I wouldn't want to expose this edge case pref on any regular context menu, because this is a once-only or once-in-a-while setting which would be just clutter on any other menu most of the time.
I mean that I will hook up the attachment list context menu with attachment-specific actions to the "5 attachments" and "4 MB" strings because those strings represent the attachments, and I don't see much point of adding our edge case menu there as permanent clutter. Please check attachment header bar in message reader where everything is exactly the same, and nobody has ever complained.

Comment 35

[Thomas D. (:thomas8)]

Attachment: TB60-only Patch V.2 (with string hack)

(In reply to :aceman from comment #31)
> > Otherwise, I think the following hack would be less expensive:
> > - get the existing string somewhere near application startup*, and save it
> > somewhere (global variable, temporary pref).
> > - get that global variable or temp-pref only when we need it: onpopupshowing
> > of the context menu here
> 
> Would sound crazy for a permanent solution, but if it is ~5 lines of code,
> we may consider it for the 60 branch. Show us the patch :)

Here we go, is 2 lines ok?

I also changed the name of the pref, but I really think it's wrong in terms of sustainability and doesn't help in any way.

Comment 36

[Thomas D. (:thomas8)]

Attachment: Patch V.2 (with proper strings)

Here's the new patch for TRUNK.

I renamed the pref to reduce the pref branch level as requested by Aceman and in line with Jörg's proposal. I still think it's wrong wrt long-term sustainability and systematism.

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 8960109 [details] [diff] [review]
Patch V.2 (with proper strings)

Hmm, the |initiallyShowItem.setAttribute("checked", ...| is still there which Aceman wanted removed. So I'll let him handle this review.

Comment 38

[Jorg K (CEST = GMT+2)]

Comment on attachment 8960103 [details] [diff] [review]
TB60-only Patch V.2 (with string hack)

I'd prefer not to do ugly hacks like this one. How about instead of using "Initially Show Attachment Pane" we use "Show attachment pane" which we already have:
https://hg.mozilla.org/comm-central/rev/4b5c6a135ad5#l1.12
Would that be the end of the world?

The hack puts the string we need into a preference in mailWindow.js, but what happens if you start a compose window from the command line as I frequently do for testing. Does the pref get initialised then?

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f87ea7a2a923
Implement "Initially show attachment pane" pref and UI for composition (string only). r=jorgk, ui-r=Paenglab DONTBUILD

Comment 40

[Jorg K (CEST = GMT+2)]

String for TB 60 beta:
https://hg.mozilla.org/releases/comm-beta/rev/3568dcc5e0aadf365b87f749641eaeb0106ac609

I'll send an apologetic notification to the l10n mailing list. Better one week late than discussing here two weeks more.

Comment 41

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #40)
> String for TB 60 beta:
> https://hg.mozilla.org/releases/comm-beta/rev/
> 3568dcc5e0aadf365b87f749641eaeb0106ac609
> 
> I'll send an apologetic notification to the l10n mailing list. Better one
> week late than discussing here two weeks more.

Thanks, yes, that's the better way forward as we're striving to round off our attachment pane UX efforts.

I wish we could push the attachment button to the right which would just need some migration code...

Comment 42

[:aceman]

(In reply to Thomas D. (currently busy elsewhere) from comment #32)
> > Also don't forget that you will only copy the existing string from ESR60
> > branch to the new ID and in a different file.
> > For trunk, we land the string untranslated so that localizers pick it up.
> > They may need to translate it differently than the existing string.
> 
> Huh??? How can two identical strings at the same UI location, same meaning
> and context, have two different translations?
> That would only be possible when translators deviate strongly from the given
> string ("Show the attachment pane at start of reading"), meaning that both
> of their strings would be wrong. But looking at the translations which I an
> read, they look right.

Just do not assume anything about translations. The object looks differently at the two places to they easily can be translated differently. 'pane' is convenient generic term in English, but other languages may have better words for those different displays of the attachment list.

(In reply to Thomas D. (currently busy elsewhere) from comment #34)
> (In reply to :aceman from comment #29)
> > I don't think the features are very related. Most user will have one
> > enabled, while the other one not.
> 
> Huh? It's exactly the same feature, only in different spots of the UI.
> It's not about how the user sets them, it's about having a more systematic
> pref naming system where the same things shouldn't have different names. I
> think that's pretty obvious.

It is absolutely not the same feature. One is adding attachments, one is reading them.
 
> > > > The current style of attachment prefs is 
> > > > "mail.compose.attachment_pane_display_initially".
> > > That's how we create messy and non-scalable pref systems.
> > We didn't create it, we just follow it now. Your counter proposal with 2 new
> > levels is not sound.
> I didn't see much to follow here. Afasics, this is the first pref of
> category attachments.display.

Yes, and as you want to create a new category (even 2 levels), then I ask what the justification is and what the future plans are.
Randomly creating new categories is something I'm against.
 
> > How do you propose to migrate existing prefs to mail.compose.attachment.* to
> > cleanup the supposed mess?
> We all know that migrating existing prefs isn't advisable, and I never
> postulated that.

We have a system for migrating prefs, so it works fine for upgrades. It just complicates downgrading to older TB versions for people.
 
> > Ah, dynamic picking of pref changes of a totally edge case pref. I don't
> > think we need to support this.
> 
> Huh? Edge case or not, there's nothing more confusing than a pref which I
> change in config editor and which doesn't take effect immediately, but
> instead results in the pref and the UI losing sync.
> And the costs of ensuring sync always are totally zero, I'm failing to see
> the problem of syncing the menu checkmark from a single boolean pref each
> time the menu is opened.

The cost isn't zero. Fighting with what the XUL does automatically but we force it to some other value manually is something anybody reading the code will wonder about and will have to keep in mind.

> As you said, it's an edge case pref so it's not
> like users are doing this every day, and even if they did, a single pref
> call doesn't slow them down in any way.
> So let's please avoid cans of worms where users start filing bugs that they
> changed the pref from config editor but it doesn't work as it's not shown in
> the UI. Also in message reader it works exactly like this, I just copied the
> code.

So if you agree it is an edge case why do you suddenly say a ton of users will notice the UI is not in sync when they edit the pref via config editor. We surely have more important prefs where the syncing is not done. I think we specifically do not promise any pref changes in config editor will take effect immediately without restart. It is mostly done for prefs where it makes sense.
 
> > > > ::: mail/components/compose/content/messengercompose.xul
> > > > The context menu is only shown on the very narrow part between the "X
> > > > attachments" label and the X (close) button. Is that intentional?
> I'd rather not. I wouldn't want to expose this edge case pref on any regular
> context menu, because this is a once-only or once-in-a-while setting which
> would be just clutter on any other menu most of the time.
> I mean that I will hook up the attachment list context menu with
> attachment-specific actions to the "5 attachments" and "4 MB" strings
> because those strings represent the attachments, and I don't see much point
> of adding our edge case menu there as permanent clutter. Please check
> attachment header bar in message reader where everything is exactly the
> same, and nobody has ever complained.

Totally incomparable. In the message reader the menu is shown for almost (except the 'X attachments' label) the whole bar above the attachment list, which is much of the width of TB window, or at least the whole width of the message reader pane.
Your menu is shown on a blank space that is 1cm wide (for me) between the 2 labels where it may not appear to the user that it is something to click at.
I never knew there is a menu on the message reader attachment pane even when it is that large. This new menu will be totally non-existent to me (thus to many users too).
But I do not care much, I just pointed it out, in case it wasn't intentional.

Comment 43

[:aceman]

Attachment: patch v2.1

The patch no longer applied due to the landed strings and some change in the prefs file.

Comment 44

[Jorg K (CEST = GMT+2)]

You could rebase it "as a service" so we can move on.

Comment 45

[:aceman]

Comment on attachment 8961951 [details] [diff] [review]
patch v2.1

Review of attachment 8961951 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/app/profile/all-thunderbird.js
@@ +536,5 @@
>  // Number of recipient rows shown by default
>  pref("mail.compose.addresswidget.numRowsShownDefault", 3);
>  
> +// Start compositions with expanded attachment pane
> +pref("mail.compose.attachments.display_start_expanded", false);

Does the pref name make sense? "display_start_expanded"? Maybe "display_at_start" ? It looks like you want to keep the added level "display" and just changed . to _. But that quite changes the meaning.
Jorg's proposal of "mail.compose.attachments.display_pane_initially" also seemed more readable to me.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5344,5 @@
>    // in attachment bucket right now, which is required for updating them.
>    updateReorderAttachmentsItems();
>  }
>  
> +function attachmentHeaderContextOnPopupShowing() {

I think I can learn to live with this. At least it has the benefit of working right (in sync) when multiple compose windows are open in parallel and user changes the pref in one. Another edge edge-case ;)

@@ +5348,5 @@
> +function attachmentHeaderContextOnPopupShowing() {
> +  let initiallyShowItem =
> +    document.getElementById("attachmentHeaderContext_initiallyShowItem");
> +
> +    initiallyShowItem.setAttribute("checked", Services.prefs.getBoolPref(

Bad indent?

Comment 46

[:aceman]

(In reply to Jorg K (GMT+1) from comment #44)
> You could rebase it "as a service" so we can move on.

I already did :)

Comment 47

[Jorg K (CEST = GMT+2)]

Can we fix the nits here and get this landed? Or should I do that? We need to start thinking about a TB 60 beta 2. There's also still bug 1428631 that should go out in TB 60.

Comment 48

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #45)
> Review of attachment 8961951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/app/profile/all-thunderbird.js
> @@ +536,5 @@
> >  // Number of recipient rows shown by default
> >  pref("mail.compose.addresswidget.numRowsShownDefault", 3);
> >  
> > +// Start compositions with expanded attachment pane
> > +pref("mail.compose.attachments.display_start_expanded", false);
> 
> Does the pref name make sense? "display_start_expanded"? Maybe
> "display_at_start" ? It looks like you want to keep the added level
> "display" and just changed . to _. But that quite changes the meaning.
> Jorg's proposal of "mail.compose.attachments.display_pane_initially" also
> seemed more readable to me.

I am deliberately using corresponding names for these corresponding prefs.
Both prefs have exactly the same wording in the UI, and they do exactly the same thing, namely to ensure that attachment panel is open at the startup of message reader or composition respectively:

mailnews.attachments.display.start_expanded
mail.compose.attachments.display.start_expanded

Sorry, but according to my understanding of coding style, readability, and maintainability, using two totally unrelated paths and names for what is essentially the same feature (only in different UI locations) is just mind-boggling, and I don't want to get flogged or laughed at by later generations seeing such wild and disorganized proliferation. Corresponding pref path/name makes it easy for administrators to search and find logically related prefs, notwithstanding the fact that they might want to set different values for each. Know one pref, know the other. That's helpful, isn't it?
I think this pref name is 100% correct: We're tweaking the *display* of the attachment pane (not attachments themselves), so that it always *starts in expanded view* (note that "start" is a verb in the pref name).

Jim has done a lot of attachment-related work and he certainly introduced the |attachments.display.*| pref branch in message reader for a reason. I have provided as many as 7 instant examples how we might use both levels of the new pref branch in the future, pls see comment 33. It's telling that Jörg is using auto-shrink addon which is among the future prefs which I listed. For some others like |mail.compose.attachments.display.dock| and |mail.compose.attachments.display.viewmode|, we already have bugs on record. In view of that, it would be foolish *not* to introduce the future-proof .display. pref branch now. And I've never heard that the mere existence of a pref branch (.display.) eats any extra resources that we'd have to worry about, at worst it could add one sub-object to the pref system object.

So if you still insist on changing the pref name into something less consistent and harder to understand and maintain for admins and future devs, please do it yourself.
 
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +5344,5 @@
> >    // in attachment bucket right now, which is required for updating them.
> >    updateReorderAttachmentsItems();
> >  }
> >  
> > +function attachmentHeaderContextOnPopupShowing() {
> 
> I think I can learn to live with this. At least it has the benefit of
> working right (in sync) when multiple compose windows are open in parallel
> and user changes the pref in one. Another edge edge-case ;)

Hallelujah! I think it's good and safe practice to make any changes apply immediately everywhere and not to rely on later helper methods to update things. You never know which consumer might try to read your settings/prefs before you have updated them at some random later point in time to be in sync with reality (addons come to mind, but also core code).

> @@ +5348,5 @@
> > +function attachmentHeaderContextOnPopupShowing() {
> > +  let initiallyShowItem =
> > +    document.getElementById("attachmentHeaderContext_initiallyShowItem");
> > +
> > +    initiallyShowItem.setAttribute("checked", Services.prefs.getBoolPref(
> 
> Bad indent?

No, that's deliberate to keep the whole pref name in one line (and avoid folding it into several one-word lines); is there a better way?

Comment 49

[Thomas D. (:thomas8)]

Attachment: String-only patch: "Initially Show Attachment Pane" (LANDED in comment 39+40)

For easy reference, here's the string-only patch which Jörg separated off and landed in comment 40.

Comment 50

[Jorg K (CEST = GMT+2)]

This

+function attachmentHeaderContextOnPopupShowing() {
+  let initiallyShowItem =
+    document.getElementById("attachmentHeaderContext_initiallyShowItem");
+
+    initiallyShowItem.setAttribute("checked", Services.prefs.getBoolPref(
+      "mail.compose.attachments.display_start_expanded"));
+}

should be this:

+function attachmentHeaderContextOnPopupShowing() {
+  let initiallyShowItem =
+    document.getElementById("attachmentHeaderContext_initiallyShowItem");
+
+  initiallyShowItem.setAttribute("checked", Services.prefs.getBoolPref(
+    "mail.compose.attachments.display_start_expanded"));
+}

Aceman, can we please settle on a pref name. Maybe you want to prepare the final patch.

Comment 51

[Thomas D. (:thomas8)]

Attachment: Patch V.3 (for better ux-discovery, also add |Intially Show Attachment Pane| to attachment list context menu)

This implements Richard's proposal of adding |Initially Show Attachment Pane| to the attachment list context menu, which will be shown when right-clicking on |X Attachments| and file size labels in the attachment header bar (after bug 707432). Meaning you can now right-click anywhere in the header bar to get this menu. So this addresses reviewer's common complaint that the blank space of composition's attachment pane header is too small and undiscoverable as a click target.

As we now have more than one consumer for the new menu, I had to change the inner logics a bit to avoid duplicate code, but I think a full-blown command would be overkill (and wouldn't work well), so I took some inspiration from other code and used <broadcaster> instead to sync the checkmark and function call across any consumers.

This had aceman's r+ already but due to the new logic with broadcaster I'm asking for another round - soft and gentle, please... ;-)

As for the pref name saga, pls find my detailed explanation in comment 48, which refers back to comment 33 with numerous examples of future prefs, some of them have bugs already.

All other nits addressed or otherwise sorted out.

Comment 52

[Thomas D. (:thomas8)]

Attachment: Patch V.3.1 (include bug 707432: add attachment list context menu to |X attachments| and size labels)

Let's make reviewers' life easier by combining this with bug 707432 so that we can have the full UX, just hooking up the attachment list context menu in the attachment bar header.

Pls refer to comment 51 for full explanation of this patch.

Comment 53

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #50)
> This
> ...
> should be this:
> 
> +function attachmentHeaderContextOnPopupShowing() {
> +  let initiallyShowItem =
> +    document.getElementById("attachmentHeaderContext_initiallyShowItem");
> +
> +  initiallyShowItem.setAttribute("checked", Services.prefs.getBoolPref(
> +    "mail.compose.attachments.display_start_expanded"));
> +}

Oh, thanks, now I see (but it took long to realize as you didn't mention the topic of indent). Anyway, code has changed, so this is no longer there afasics.

Comment 54

[Thomas D. (:thomas8)]

I wasn't sure about strings, I did not add them again because somehow they seem to be in trunk already. But I thought Jörg had only landed them on comm-beta, no?

Comment 55

[Thomas D. (:thomas8)]

Fwiw, I had to change the access key to avoid duplicate access key in the attachment list context menu. This is NOT a string change which requires changing the string id because we're only changing this in our own locale, en-US.

Speaking of strings, I did re-use the same string which we have in the header-blank-space menu because they can never be different from the same string in the list context menu. Locales just have to pick the right access key for themselves which works at both spots, so essentially to pick one which works on the list context menu, and the same access key will then apply to the attachment pane header-blank-space menu too. So I think it's alright as it is, let's not over-engineer this.

Comment 56

[:aceman]

Yes, I said the string already landed (I was on trunk of course) so I removed them from the patch. It is OK if they are in a separate attachment, as you did now.

Thanks for fixing the indent (which I pointed out in comment 45).

I gave 2 proposals for the pref name in comment 45. If you choose one of them, you could have landed the patch straight away.

Now I don't know what comment 51 want's to achieve (other then render the review invalid). I thought we wanted to backport this to beta too as the strings were already landed. Not so for bug 707432 so I don't know how that one is related and why we should amend the patch here due to it. It may not apply to beta then.

So as other more critical breakage has arrived, I do not promise to look at this soon (now would have to study some other totally new bug).

Comment 57

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere) from comment #54)
> I wasn't sure about strings, I did not add them again because somehow they
> seem to be in trunk already. But I thought Jörg had only landed them on
> comm-beta, no?
https://hg.mozilla.org/comm-central/rev/f87ea7a2a923 (comment #39)
https://hg.mozilla.org/releases/comm-beta/rev/3568dcc5e0aadf365b87f749641eaeb0106ac609 (comment #40)

Comment 58

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #56)
> Yes, I said the string already landed (I was on trunk of course) so I
> removed them from the patch. It is OK if they are in a separate attachment,
> as you did now.

Oh, I overlooked comment 39, thanks Jörg.

> Thanks for fixing the indent (which I pointed out in comment 45).

I think that just removed as the code changed...

> I gave 2 proposals for the pref name in comment 45. If you choose one of
> them, you could have landed the patch straight away.

(In reply to :aceman from comment #45)
> Does the pref name make sense? "display_start_expanded"? Maybe
> "display_at_start" ?

display_at_start imo is much less clear about what it does. It could even be misunderstood that we're displaying the content of attachments at the start of composition in some sort of preview.

> It looks like you want to keep the added level
> "display" and just changed . to _. But that quite changes the meaning.

I actually just want to keep the original pref path and name, for logical consistency with the message reader pref and easy maintenance (future-proof), and I've offered a new and detailed explanation why, in comment 48, and many specific examples of how we can use that pref branch in the future, see comment 33.

> Jorg's proposal of "mail.compose.attachments.display_pane_initially" also
> seemed more readable to me.

Yes, this is nice and readable, but needlessly inconsistent with the corresponding message reader pref, and not future proof if we decide to add more display.* prefs (some bugs already on record). I still think that prefs that have the same effect should have the same name and a similar path. If you think otherwise, please change it yourself, as I don't want to be associated with changes that make prefs even more disorganized and random than they already are.

> Now I don't know what comment 51 want's to achieve (other then render the
> review invalid).

Let me read that as a question (as it reads somewhat odd as a statement, certainly I'm not just doing this with the intention of rendering reviews invalid).
I tried to explain in comment 51 why the patch had to be slightly changed, and I thought that explanation is quite explicit, so please re-read comment 51. Richard and others (including you, no?) have requested to enlarge the click target for getting the |Initially Show Attachment Pane| context menu in composition. So now we have two flavors of this menu, which both need to be updated before showing them. To avoid code duplication of manual updating, I'm now using a <broadcaster> which simplifies the code and ensures presence and syncing of the checkmark.

In fact, let me double-check, maybe I can simplify this even more radically (onpopupshowing maybe not needed due to menus' observes attributes with central broadcaster, which we update on every click).

> I thought we wanted to backport this to beta too as the
> strings were already landed.

Yes. There's nothing in the new patch which can't be backported. I'm not adding or changing any strings that would prevent the backport.

> Not so for bug 707432 so I don't know how that
> one is related

Yes, they are closely related. But I'm just adding two context="msgComposeAttachmentListContext" attributes to include bug 707432 here, which is a two-liner. 

> and why we should amend the patch here due to it.
> It may not apply to beta then.

Richard had reservations in the other bug which are actually reservations about this bug. It will still apply to beta.

> So as other more critical breakage has arrived, I do not promise to look at
> this soon (now would have to study some other totally new bug).

No, there's nothing much new here, no need to study the other bug which is just two lines hooking up an existing context menu.

Comment 59

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere) from comment #58)
> In fact, let me double-check, maybe I can simplify this even more radically
> (onpopupshowing maybe not needed due to menus' observes attributes with
> central broadcaster, which we update on every click).

Scrap that. We have to update from pref onpopupshowing in case the pref gets changed from config editor. I could be simplified if we could somehow make the broadcaster observe the pref itself, but I don't know how.
But the current code works correctly afasics and it's quite straightforward.

Comment 60

[Jorg K (CEST = GMT+2)]

Comment on attachment 8962675 [details] [diff] [review]
Patch V.3.1 (include bug 707432: add attachment list context menu to |X attachments| and size labels)

Two things:
First: Let's not switch reviewer here.
Second: I don't like it. "Initially show attachment pane" should only show on the header, not on the right-click menu on the bucket itself. Like this, it doesn't match message reading.

Please take note: Due to changes in M-C, non-restartless add-ons, like Calendar and all the add-ons used in Mozmill testing don't work any more. So we've lost all Mozmill testing and can't guarantee the proper functioning or anything any more.

I have closed the tree and cannot admit any more landings, unless they fix other bustage.

We are also frantically removing overlays before M-C will disable them and TB will be completely busted. Without Mozmill, we cannot even test the overlay removal any more, although for the next couple of days, we can work around that.

So we really have *NO TIME* for any of this right now. If anything, let's go with the r+ version 2.1 Aceman attached, I don't care about the pref name.

Comment 61

[Jorg K (CEST = GMT+2)]

Oh, I forgot to say, there is a "preference observer": Services.prefs.addObserver("somePref", callback);
in case that should still be relevant.

Comment 62

[Richard Marti (:Paenglab)]

Comment on attachment 8962675 [details] [diff] [review]
Patch V.3.1 (include bug 707432: add attachment list context menu to |X attachments| and size labels)

UI-r+ only when "Initially show attachment pane" is only shown in the header.

Comment 63

[Thomas D. (:thomas8)]

Attachment: Patch V.4 (hide menuitem from bucket context, header bar: 100% height for context click targets)

As Jörg and Richard requested, I am now hiding |Initially Show Attachment Pane| menu item from the bucket context, and the |X Attachments| and size labels now sit in their own hboxes to ensure 100% height in the header for these context click targets (per Richard's bug 707432 comment 9).

Hiding a menuitem is easy, hiding a menuseparator in a sustainable way that doesn't break when addons meddle with items and separators is pretty non-trivial.

Message reader has a number of helper functions to solve this problem, so I took some inspiration from there:

https://dxr.mozilla.org/comm-central/rev/08bab4937197974ee06ea7e78fbaae1bfea71623/mail/base/content/nsContextMenu.js#398-414
> initSeparators: function CM_initSeparators() {

https://dxr.mozilla.org/comm-central/rev/08bab4937197974ee06ea7e78fbaae1bfea71623/mail/base/content/nsContextMenu.js#903-953
> hideIfAppropriate: function CM_hideIfAppropriate(aSeparatorID) {
> ...
> shouldShowSeparator : function CM_shouldShowSeparator(aSeparatorID) {
> ...
> checkLastSeparator: function CM_checkLastSeparator(aPopup) {

I condensed all of these into a single generic function to sort out separators which can be used for any other menu, too. Forget the internals - it just works!

Comment 64

[:aceman]

Comment on attachment 8963376 [details] [diff] [review]
Patch V.4 (hide menuitem from bucket context, header bar: 100% height for context click targets)

Review of attachment 8963376 [details] [diff] [review]:
-----------------------------------------------------------------

What a short patch it was and now it is this monster :(

If you need the new helpers, I'd like to see how they will be used at other places.

::: mail/app/profile/all-thunderbird.js
@@ +536,5 @@
>  // Number of recipient rows shown by default
>  pref("mail.compose.addresswidget.numRowsShownDefault", 3);
>  
> +// Start compositions with expanded attachment pane
> +pref("mail.compose.attachments.display.start_expanded", false);

Getting back to the 4-level pref name?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5368,5 @@
> + * Two visible separators in a row: the first one will be hidden.
> + * Visible separator at the very end of the popup will be hidden.
> + * Mark separators hidden by this function so that it can be undone later.
> + */
> +function handleMenuSeparators(aPopupID, aShow) {

Nice helper. Isn't there one already somewhere? Or if not, we should put this in some better place.

@@ +5382,5 @@
> +       sep.removeAttribute("hiddenUndoMarker");
> +     })
> +  } else {
> +    separators.forEach(sep => {
> +      sep.removeAttribute("hiddenUndoMarker");

Are you sure about this? What if separator was hidden by a previous run and we are hiding the separators again (aShow = false) ? Why would yuo drop the attribute now?

@@ +5401,5 @@
> +      if (!sibling || sibling.tagName == "menuseparator") {
> +        // If there's no visible element after the separator
> +        // or there's a visible menuseparator right after the current separator:
> +        // Hide the current separator.
> +        sep.setAttribute("hidden","true");

You used the .hidden property elsewhere, why using attribute here?

@@ +5427,5 @@
> + * the pref and the broadcaster for keeping checked attribute in sync.
> + *
> + */
> +function toggleInitiallyShowAttachmentPane(aCheckboxElement) {
> +  let showInitially = !!aCheckboxElement.getAttribute("checked"); // ensure boolean

aCheckboxElement.getAttribute("checked") == "true", no !! games. It would evaluate to true even when "checked=false".

::: mail/components/compose/content/messengercompose.xul
@@ +227,5 @@
>    <broadcaster id="args" value="editorType=default"/>
> +  <broadcaster id="viewAddressPicker"
> +               type="checkbox" autoCheck="false"
> +               oncommand="toggleAddressPicker();"/>
> +  <broadcaster id="brc_toggleInitiallyShowAttachmentPane"

So why is suddenly an observer needed? You promoted getting it all right via onpopupshowing calls.

Comment 65

[Jorg K (CEST = GMT+2)]

Comment on attachment 8961951 [details] [diff] [review]
patch v2.1

I'm going to land this now with the indentation fixed and the pref name changed. Aceman convinced me that there's no point creating mostly empty branches by using many dot separators. See for example here:
https://dxr.mozilla.org/comm-central/rev/cc738342cb1c3f4f91009fd38f762a9249d7eb39/mail/test/mozmill/account/test-account-values.js#228

This is a neat improvement so people who really want it can create new messages with the bucket already open. It doesn't warrant a lot of code and it should be consistent with the preview pane where you also need to click onto white-space to enable the feature.

This has missed two betas, so I will get this into TB 60 beta 3 now. It also blocks more important enhancements in bug 1428631. Thomas, if you really think that we need a more sophisticated solution, please file a follow-up bug.

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/62f3d0d324c4
Implement "Initially show attachment pane" pref and UI for composition. r=aceman, ui-r=Paenglab

Comment 67

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 3):
https://hg.mozilla.org/releases/comm-beta/rev/5ace949bc4a44fc83629d557dc51301a7d337303