Clean up the attachment code in the compose message window
Categories
(Thunderbird :: Message Compose Window, task)
Tracking
(thunderbird_esr78 wontfix, thunderbird86 wontfix)
People
(Reporter: aleca, Assigned: aleca)
References
Details
Attachments
(1 file, 5 obsolete files)
109.39 KB,
patch
|
aleca
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•3 years ago
|
||
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
Assignee | ||
Comment 2•3 years ago
|
||
A failing test should be fixed: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=390063ae8814e59379387b19b584d66ac64ae4fc
Sorry for the noise.
Assignee | ||
Comment 3•3 years ago
|
||
Still some failures, I'll fix them.
Comment 4•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
Comment on attachment 9198788 [details] [diff] [review]
1688331-attachment-code.diff
Mid-air collision, sorry for the noise.
Comment 7•3 years ago
|
||
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)
Assignee | ||
Comment 8•3 years ago
|
||
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.
Assignee | ||
Comment 9•3 years ago
|
||
File linted, sorry for the noise.
Assignee | ||
Comment 10•3 years ago
|
||
Uh, lots of test failures >_>
Assignee | ||
Comment 11•3 years ago
|
||
Tests were failing for a silly missing variable.
Everything looks good now: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=646835335aeb1014284687a7e43150f95612c2ee
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 12•3 years ago
|
||
Little typo in the variable name.
Updated•3 years ago
|
Comment 13•3 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/b84ef4aee6c9
Clean up and refactor the code handling the attachment bucket in the compose window. r=mkmelin
Description
•