1700278 - Convert pills' contextual functions (cut/copy/delete) to generic internal functions of <mail-recipients-area>, and related cleanup (esp.: stop passing focus pill argument all over the place)

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, task)

Description

[Thomas D. (:thomas8)]

Pills' contextual actions (cut/copy/delete) are connected in odd ways, and passing a focus-pill element argument which isn't really needed and sometimes ends up nowhere.
Let's create a bit of order, simplify the call chain and convert them into generic internal functions of <mail-recipients-area>.
This will also make it easier to expose these actions on the main menu, e.g. for bug 1687432.

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1700278_rewritePillsContextActions.diff

The patch.

• Simplify pills' contextual cut/copy/delete call chain by eliminating unneeded
  no-op focus pill element argument, which will make them context-independent.
• Make pills' cut/copy/delete actions internal functions of <mail-recipients-area>.
• Hook up pills' actions in the layout layer to generic onCommand handlers in
  MsgComposeCommands.js.
• Specify focused pill as default parameter for pill argument of
  removeSelectedPills().

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9210949 [details] [diff] [review]
1700278_rewritePillsContextActions.diff

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

Good work, this works and I like the clean up.
Just a little nit comment about the inline selector.

We should definitely jump on bug 1667617 and cover all of these with tests before the next ESR.

::: mail/base/content/mailWidgets.js
@@ +2999,5 @@
>       * @param {boolean} [moved=false] - Whether the method was originally called
>       *   from the moveSelectedPills.
>       */
>      removeSelectedPills(
> +      pill = this.querySelector("mail-address-pill:focus"),

Mhh...I'm not sure about this, it feels weird.
We never use a selector as a fallback default attribute.
This is a purely code styling nit, I think we should ask Magnus about it.

Comment 3

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani [:aleca] from comment #2)

 removeSelectedPills(

•  pill = this.querySelector("mail-address-pill:focus"),
  

Mhh...I'm not sure about this, it feels weird.
We never use a selector as a fallback default attribute.
This is a purely code styling nit, I think we should ask Magnus about it.

This is terse and correct, because the focused pill selector is the default argument for the pill argument.
I understand that we are encouraged to write terse and correct code.
The alternative would be this:

if (!pill) {
  pill = this.querySelector("mail-address-pill:focus"),
}

That's 2 extra lines and less readability because you can't tell from looking at the function description or the arguments that pill has a default argument. I don't see why we should ask for doing the correct thing.

Comment 4

[Alessandro Castellani [:aleca]]

I don't see why we should ask for doing the correct thing.

I'm not saying that that's not correct, i was simply questioning the code formatting as, as far as I know, we never used that styling, and keeping code styling consistent across a large application is important.
Usually, we do this:
let selectedPill = pill || this.querySelector("mail-address-pill:focus").

As I said, it's a nit so if you wanna push it go ahead.

Comment 5

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani [:aleca] from comment #4)

I'm not saying that that's not correct, i was simply questioning the code formatting as, as far as I know, we never used that styling, and keeping code styling consistent across a large application is important.

Absolutely!

Usually, we do this:
let selectedPill = pill || this.querySelector("mail-address-pill:focus").
As I said, it's a nit so if you wanna push it go ahead.

Sorry, that was Friday evening, I must have been grumpy! That lengthy default parameter was looking a bit weird indeed.
Thank you for insisting on consistent code formatting!
On second thoughts, we can just rip that pill argument out, which will declutter a number of callers!

Comment 6

[Thomas D. (:thomas8)]

Attachment: 1700278_rewritePillsContextActions.diff

2nd iteration, more cleanup!

• Remove focused pill argument from removeSelectedPills(), as we can always retrieve the focused pill from inside this function. Typically as long as any pills are selected, the focus will also be on a pill (which may or may not be selected). Even when using context or main menus, the pill focus is not lost.
• If there's no selected pill, there's nothing to remove, so let's bail out.
• I'm now using the first selected pill as a fallback for the unlikely event that there's no focused pill.
• Declutter callers: removeSelectedPills(), moveSelectedPills().
• Shrinked dropAddressPill() to a one-liner with a single argument, the label where the drop occurs. This function was calling moveSelectedPills(mailRecipientsArea.getAllSelectedPills()[0],...) - not ideal, to iterate an unlimited number of pills just to get the first one... In fact, 5 out of 6 lines in that function were just clutter, as well as 2/3 of its arguments. I'm still not happy with the general design around dropAddressPill(), but at least it's now de-cluttered.

Complete revised summary of this patch:

• Simplify pills' contextual cut/copy/delete/move-to-* call chain by eliminating
  no-op focus pill argument, which will make them context-independent.
• Make pills' cut/copy/delete actions internal functions of <mail-recipients-area>.
• Hook up pills' actions in the layout layer to generic onCommand handlers in
  MsgComposeCommands.js.
• Retrieve the focused pill via querySelector at the only place where we need it,
  removeSelectedPills(), and handle lost focus gracefully.
• Cleanup dropAddressPills() and remove unneeded arguments; more to be done.

Comment 7

[Alessandro Castellani [:aleca]]

Comment on attachment 9212060 [details] [diff] [review]
1700278_rewritePillsContextActions.diff

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

This is good, great improvements here, thank you so much.
Just a couple of small linting errors.

All of these features should be covered by tests.
I'm gonna take care of creating those in a follow up bug.

::: mail/base/content/mailWidgets.js
@@ +3032,3 @@
>        focusType = "next",
>        select = false,
>        moved = false

nit linting error: These attributes fit all in one line.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5011,5 @@
>      }
>    }
>  }
>  
> +

nit linting error: remove extra blank line.

Comment 8

[Thomas D. (:thomas8)]

Attachment: 1700278_rewritePillsContextActions.diff

Thank you for the appreciative review!

Two linting nits fixed, otherwise unchanged, forward aleca's r+ from comment 7.

Comment 9

[Thomas D. (:thomas8)]

Attachment: 1700278_rewritePillsContextActions.diff

Remove a leftover focus pill function comment, and 2 other nits (rename item -> pill). r+ from comment 7.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5ba627148586
Convert pills' contextual actions (cut/copy/delete) to generic internal functions of <mail-recipients-area> CE, and related cleanup. r=aleca