Closed Bug 1532182 Opened 4 years ago Closed 4 years ago

simplify some code in addressingWidgetOverlay.js

Categories

(Thunderbird :: Message Compose Window, task)

task
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 68.0

People

(Reporter: aceman, Assigned: aceman)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.

Blocks: 1528840
Type: defect → enhancement
Attached patch 1532182.patch (obsolete) — Splinter Review

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

Attachment #9059665 - Flags: review?(mkmelin+mozilla)
Attached patch 1532182.patch v1.1 (obsolete) — Splinter Review
Attachment #9059665 - Attachment is obsolete: true
Attachment #9059665 - Flags: review?(mkmelin+mozilla)
Attachment #9059739 - Flags: review?(mkmelin+mozilla)
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
Attachment #9059739 - Flags: review?(mkmelin+mozilla) → review+

(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.

(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.

(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.

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

Attachment #9059739 - Attachment is obsolete: true
Attachment #9060944 - Flags: review+
Keywords: checkin-needed
Type: enhancement → task

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f07df723ca48
use more richlistbox methods in addressingWidgetOverlay.js and MsgComposeCommands.js. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0
Blocks: 1547423
You need to log in before you can comment on or make changes to this bug.