1688331 - Clean up the attachment code in the compose message window

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, task)

Description

[Alessandro Castellani [:aleca] - PTO until Nov 16]

There's some good clean up we can do for the code that handles the Attachment Bucket in the msgComposeCommand.js file.
This will be necessary to allow a better and simpler refactoring in bug 1640760.

Comment 1

[Alessandro Castellani [:aleca] - PTO until Nov 16]

Attachment: 1688331-attachment-code.diff

A simple code clean up that takes care of:

• Removes unnecessary helper methods.
• Properly defines the attachment bucket variable on load.
• Fixes the XHTML indentation.

Try run to see if I broke something: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=3361fa18be537702ab82d76e1136ff58d13cb19c

Comment 2

[Alessandro Castellani [:aleca] - PTO until Nov 16]

Attachment: 1688331-attachment-code.diff

A failing test should be fixed: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=390063ae8814e59379387b19b584d66ac64ae4fc

Sorry for the noise.

Comment 3

[Alessandro Castellani [:aleca] - PTO until Nov 16]

Still some failures, I'll fix them.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9198788 [details] [diff] [review]
1688331-attachment-code.diff

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +101,5 @@
>  var gSavedSendNowKey;
>  var gSendFormat;
>  var gContextMenu;
>  
> +var gAttachmentPane;

Maybe gAttachementBucket would be a better name. I was a bit confused as to what it was.

@@ +6660,5 @@
>  
>      listItems = attachmentsSelectionGetSortedArray();
>    } else {
>      // aSelectedOnly == false
> +    if (!gAttachmentPane.itemCoun) {

missing a t, maybe that's what's causing the test failures

Comment 5

[Thomas D. (:thomas8)]

Comment on attachment 9198788 [details] [diff] [review]
1688331-attachment-code.diff

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

Awesome. Wow, that's a big rewriting exercise, and carefully done! Thank you very much for separating out these cleanup changes; history will thank you!
There's two nits which will cause errors (one misspelt property and one old function call not replaced). Additional review = me with those fixed.
Two or three other polish nits.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1197,5 @@
>          );
>          cmdDelete.setAttribute("label", textValue);
>          cmdDelete.setAttribute("accesskey", accesskeyValue);
>  
> +        return gAttachmentPane.selectedCount;

isEnabled() expects true or false as return value, but now you're returning selectedCount which is integer (0 or positive number like 3). I understand it might still work as 0 is falsy and 3 is truthy, but is there any advantage of changing the return data type to integer instead of boolean? I find > 0 also easier to read, and it's more similar to the others.

@@ +2944,5 @@
>  function attachmentNotificationSupressed() {
>    return (
>      gDisableAttachmentReminder ||
>      gManualAttachmentReminder ||
> +    gAttachmentPane.getRowCount()

Smart!

@@ +5980,3 @@
>    } else {
>      // All attachments.
>      if (attachmentsCount() < 1) {

Skipped one conversion to gAttachmentPane.itemCount here, this function no longer exists.

@@ +6089,5 @@
>   * @param fileURL the URL (as a String) of the file to check
>   * @return true if the fileURL is already attached
>   */
>  function DuplicateFileAlreadyAttached(fileURL) {
> +  for (let i = 0; i < gAttachmentPane.getRowCount(); i++) {

Can we rewrite this and adjust the following rows accordingly?
  for (let item of gAttachmentPane.itemChildren)

@@ +6103,4 @@
>    // First, we need to clear all attachment in the compose fields
>    compFields.removeAttachments();
>  
> +  for (let i = 0; i < gAttachmentPane.getRowCount(); i++) {

Dito, I think we can rewrite with 'let item of...'  and adjust subsequent line.

@@ +6234,5 @@
>    }
>  
>    if (removedAttachments.length > 0) {
>      // Bug workaround: Force update of selectedCount and selectedItem, both wrong
>      // after item removal, to avoid confusion for listening command controllers.

Let's add the bug number here: Bug 1661507

    // Bug 1661507 workaround: Force update of selectedCount and selectedItem,
    // both wrong after item removal, to avoid confusion for listening command
    // controllers.

@@ +6660,5 @@
>  
>      listItems = attachmentsSelectionGetSortedArray();
>    } else {
>      // aSelectedOnly == false
> +    if (!gAttachmentPane.itemCoun) {

itemCount misspelled

Comment 6

[Thomas D. (:thomas8)]

Comment on attachment 9198788 [details] [diff] [review]
1688331-attachment-code.diff

Mid-air collision, sorry for the noise.

Comment 7

[Thomas D. (:thomas8)]

Comment on attachment 9198788 [details] [diff] [review]
1688331-attachment-code.diff

Something went wrong with review flags, sorry again.
r+ mkmelin
additional r+ thomas8
(both requiring some nits fixed)

Comment 8

[Alessandro Castellani [:aleca] - PTO until Nov 16]

Attachment: 1688331-attachment-code.diff

Patch updated with the suggested changes and try-run launched: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=05ac7377733cb7f22f033d98752ff07903a9bb89

The only thing I haven't update is the thruty/falsy return value for return gAttachmentPane.selectedCount;, as we've been updating other parts with this same approach of returning the integer.
We're also using the same approach for all other conditions in this area so it's good to keep the consistency.

Comment 9

[Alessandro Castellani [:aleca] - PTO until Nov 16]

Attachment: 1688331-attachment-code.diff

File linted, sorry for the noise.

Comment 10

[Alessandro Castellani [:aleca] - PTO until Nov 16]

Uh, lots of test failures >_>

Comment 11

[Alessandro Castellani [:aleca] - PTO until Nov 16]

Attachment: 1688331-attachment-code.diff

Tests were failing for a silly missing variable.
Everything looks good now: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=646835335aeb1014284687a7e43150f95612c2ee

Comment 12

[Alessandro Castellani [:aleca] - PTO until Nov 16]

Attachment: 1688331-attachment-code.diff

Little typo in the variable name.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b84ef4aee6c9
Clean up and refactor the code handling the attachment bucket in the compose window. r=mkmelin