1532182 - simplify some code in addressingWidgetOverlay.js

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, task)

Description

[:aceman]

While looking at bug 1527547 I noticed some small changes that can be done in the TB address widget code, mail/components/compose/content/addressingWidgetOverlay.js.

Comment 1

[:aceman]

Attachment: 1532182.patch

We can use more methods of richlistbox instead of opencoding loops over the items and hardcoding element names (like 'richlistitem'), which we already had to change at least once (when listitem got removed).

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=241550877&revision=45e80933f1624cc3af3549916f40c5207c0a38fc

Comment 2

[:aceman]

Attachment: 1532182.patch v1.1

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9059739 [details] [diff] [review]
1532182.patch v1.1

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

r=mkmelin with the below

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3346,5 @@
>    const mailTypes = [ "addr_to", "addr_cc", "addr_bcc" ];
>  
>    // Enable the send buttons if anything usable was entered into at least one
>    // recipient field.
> +  for (let row = 1; row <= awGetNumberOfRecipients(); row++) {

awGetNumberOfRecipients is just getting a constant, so I'd rather go the other way around and just remove that function

@@ +5455,1 @@
>      for (let i = 1; i <= maxRecipients; i++) {

just switch to i <= top.MAX_RECIPIENTS, and get rid of awGetNumberOfRecipients too

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +651,5 @@
>  
>  function awGetListItem(row) {
>    var listbox = document.getElementById("addressingWidget");
> +  if (listbox && row > 0)
> +    return listbox.getItemAtIndex(row - 1);

gah, it's really unfortunate we have a 1-based indexing for these. should really refactor it to be 0-based so we don't have to do the mappping

Comment 4

[:aceman]

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3346,5 @@

const mailTypes = [ "addr_to", "addr_cc", "addr_bcc" ];

// Enable the send buttons if anything usable was entered into at least one
// recipient field.

• for (let row = 1; row <= awGetNumberOfRecipients(); row++) {

awGetNumberOfRecipients is just getting a constant, so I'd rather go the
other way around and just remove that function

Yes, but that constant is managed in the addressingWidgetOverlay.js, which is also reused in the mailinglist editor.

I actually like this that it is hidden in a helper and not accessed directly in the MsgComposeCommands.js code. This was the single remaining access. I think it's good that we only use the provided aw* functions.
What do you think?
We even had awGetMaxRecipients() that did the same so it looks like the accessor is popular ;)

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +651,5 @@

function awGetListItem(row) {
var listbox = document.getElementById("addressingWidget");

• if (listbox && row > 0)
• return listbox.getItemAtIndex(row - 1);

gah, it's really unfortunate we have a 1-based indexing for these. should
really refactor it to be 0-based so we don't have to do the mappping

But then also the ids of the address rows need to be reworked, which encode the row number (1-based). I can try it in a followup to see if it is beneficial.

Comment 5

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #4)

Yes, but that constant is managed in the addressingWidgetOverlay.js, which
is also reused in the mailinglist editor.

Looking some more, it's not even a constant! It should rather be someting like gRecipientRows. Is the "top." even needed?

I actually like this that it is hidden in a helper and not accessed directly
in the MsgComposeCommands.js code. This was the single remaining access. I
think it's good that we only use the provided aw* functions.
What do you think?

Well it's all aw*ful, so I'm not sure I care enough here. It should all be reworked.
I still think it's clearer to access a global value instead of going through a function to access it.

Comment 6

[:aceman]

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

Looking some more, it's not even a constant! It should rather be someting like gRecipientRows. Is the "top." even needed?

No, it is a counter of the number of rows in the widget.

Also, this may be why the other functions enumerate the rows starting from 1. MAX_RECIPIENT == 1 means there is one row and it's id ends in 1. If it would be 0, we would need to do the subtraction somewhere anyway.

Comment 7

[:aceman]

Attachment: 1532182.patch v1.2

I have dropped the problematic hunk. I can look at it again in the bug investigating the 'top.' in top.MAX_RECIPIENTS.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f07df723ca48
use more richlistbox methods in addressingWidgetOverlay.js and MsgComposeCommands.js. r=mkmelin