1646875 - Cleanup cmd_toggleReturnReceipt (Bug 1644345 followup) and stop discarding identity changes without asking (incl. Return Receipt)

Status: ASSIGNED

(Thunderbird :: Message Compose Window, task)

Description

[Thomas D. (:thomas8)]

Bug 1644345 was a bit rushed and slightly over-engineered, trying to square the circles, which led to somewhat confusing conditionals with obscure inner workings and usage limitations for toggleReturnReceipt(). Let's clean up here a little bit, and fix a pre-existing UX problem where manual identity changes including their subsequent Return Receipt changes etc. will be discarded without asking when composition is closed.

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1646875_toggleReturnReceiptCmdCleanup.diff (requires bug 1646857)

Cleanup cmd_toggleReturnReceipt and ask to save identity changes (incl. associated changes of Return Receipt), so this comes with minor improvements of behaviour.

• it was not very obvious from the complex conditionals added in Bug 1644345 that if you wanted to programmatically toggle return receipt without setting gReceiptOptionChanged = true; (e.g. at composition startup), absurdly you could not use toggleReturnReceipt() for toggling; instead you had to do the actual toggling outside the toggle function and then run the so-called toggle function to update only the UI. This counter-intuitive usage limitation has now been sorted.
• There are two ways of solving such problem where same function is used programmatically and by user, and only the latter requires updating gXXXChanged = true: Either pass it into the function with an extra argument (more complex callers), or handle the gXXXChanged outside the function (as realized in this patch, as we currently have only a single caller - the command - which needs gReceiptOptionChanged = true).
• we shouldn't check for availability of elements that we know for sure will be available at that point in code flow.
• ternary operator can make for neat & slim code
• if we are breaking existing (addon?) callers anyway by changing the type of the function argument (used to be event.target, now boolean), that's a good opportunity to change an old function name to camelCase.
• add and improve comments
• remove some unneeded lines

Comment 2

[Thomas D. (:thomas8)]

Comment on attachment 9157813 [details] [diff] [review]
1646875_toggleReturnReceiptCmdCleanup.diff (requires bug 1646857)

This patch requires bug 1646857 (current patch: attachment 9157792 [details] [diff] [review]).

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9157813 [details] [diff] [review]
1646875_toggleReturnReceiptCmdCleanup.diff (requires bug 1646857)

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1891,5 @@
>  function updateViewItems() {
>    goUpdateCommand("cmd_toggleAttachmentPane");
>  }
>  
> +/* Update selected commands in Options menu. We call this whenever the menu is

/* with one star is start of a multi-line code comment. 

For documentation comments, use /** (and newline)

::: mail/components/compose/content/messengercompose.xhtml
@@ +2141,5 @@
>                        type="description"
> +                      disableautoselect="true"
> +                      disableonsend="true"
> +                      onkeypress="fromKeyPress(event);"
> +                      oncommand="msgIdentityOnCommand();">

I don't think only changing identity should "count" as a content change. I.e. if that's all you did we should still close without a prompt.

Comment 4

[Jorg K (CEST = GMT+2)]

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

I don't think only changing identity should "count" as a content change.
I.e. if that's all you did we should still close without a prompt.

I don't agree with that at all. Any user action causing any aspect of the mail to change need to be saved. Imagine that someone writes a long e-mail and saves a draft. Later they return, write some more, save, then change the From:. They close the mail, open the draft later on and send it.

Oops, it went out via the wrong identity :-( - BTW, that's not working since TB 52.

Alex, how do you see this from an UX point of view?

Comment 5

[Thomas D. (:thomas8)]

(In reply to Jorg K (CEST = GMT+2) from comment #4)

Any user action causing any aspect of the mail to change needs to be saved.

+1 to all of comment 4.
We cannot decide on behalf of the user which changes are worth keeping or not.

We'll prompt even if a single space was added to message body, so randomly not prompting for important things like change of sender identity (and some of the associated changes like vCard, returnReceipt, etc.) - or sometimes prompting and sometimes not - would violate ux-consistency and ux-error-prevention by causing ux-mode-errors (UI ends up in a different state than expected). It's also a significant violation of privacy if the message is sent with wrong sender because of silently discarding user's explicit intentions/actions.

This is also relevant for workflows when closing Thunderbird: I can start an important email and get interrupted prematurely (even after just changing identity), then if TB just discards the message, I won't get that prompt to remind me of finishing the message, and that task reminder will be gone without trace. Otoh, I cannot imagine users opening dozens of compose windows for the sole sake of having them silently discarded later...

Comment 6

[Alessandro Castellani [:aleca]]

Alex, how do you see this from an UX point of view?

Thanks for the ping on this, and thanks to Thomas for working on optimizing this section.

IMO, the optimal solution would be for TB to be smart and update content changed variable only if the content actually change.
We shouldn't prompt the user to save a draft if he changes the identity and:

• It's a blank new message without content.
• The new and old identity don't come with any prepopulated recipients.

If these conditions are true, the user only changed identity without any real content change, so we can safely discard the compose window.
We should prompt the user if:

• There's pre-existing content in the mail body.
• There are pre-existing recipients.
• The new/old identity modifies the list of recipients.
• The use switches to a Custom address.

If we want to avoid all these conditions and keep the section "simple", I'm okay with prompting the user even if only the identity changed.

Comment 7

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani (:aleca) from comment #6)

Alex, how do you see this from an UX point of view?
If we want to avoid all these conditions and keep the section "simple", I'm okay with prompting the user even if only the identity changed.

Keeping it simple sounds like a good idea.