Closed Bug 1417856 Opened 2 years ago Closed 2 years ago

Reorder Attachments: Alphabetical sorting should be enabled regardless of selection

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 59.0

People

(Reporter: bugzilla2007, Assigned: bugzilla2007)

References

Details

Attachments

(1 file, 2 obsolete files)

This is a followup to enhance bug 663695.

Alphabetically sorting the entire list of attachments makes sense even when there's no or only one attachment selected, e.g. when right-clicking on attachment-list whitespace > Reorder Attachments...

So we need to adjust the isEnabled function of cmd_sortAttachmentsToggle, which also needs a new simple helper function attachmentsGetSortOrder(), similar to attachmentsSelectionGetSortOrder() which only works on selections.

When no item or one item is selected, the menu label needs to change to "Sort: A-Z|Z-A" (instead of Sort Selection: A-Z|Z-A).
Blocks: 663695
I couldn't resist.

Implemented behaviour:

* with no selection or only one selected attachment, allow sorting the entire list
* with selection of 2 attachments or more, sort selection only
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Attachment #8929377 - Flags: ui-review?(richard.marti)
Attachment #8929377 - Flags: review?(acelists)
One nit found, accesskey for sorting selection Z-A has a flaw.

Also, if someone could please enlighten me why clicking "Sort..." with mouse does NOT close the panel, but using Alt+o access key does. And I seem to remember that at some stage it did NOT do that, but that's also seen with only bug 663695, so it's not caused here.
Comment on attachment 8929377 [details] [diff] [review]
Patch V.1: Enable attachment sorting always, with dynamic labels

Lgtm, thanks.
Attachment #8929377 - Flags: ui-review?(richard.marti) → ui-review+
Aceman, this might be an easy review, and would be nice to land soon to complement bug 663695. This has ui-r+ by Richard already.

Essentially, I'm just tweaking existing functions to distinguish between two selection patterns:
A) <= 1 selected attachment --> sort entire list
B) > 1 selected attachment  --> sort selection only

I removed data- from custom attributes as suggested by Richard.

I renamed attachmentsSelectedItemsGetSortedArray() to
          attachmentsSelectionGetSortedArray(),
to shorten/simplify the function name, and in analogy to other helper functions on attachmentsSelection.

For A), I then added a new helper function attachmentsGetSortedArray(), in analogy to existing attachmentsSelectionGetSortedArray().

(In reply to Thomas D. from comment #2)
> One nit found, accesskey for sorting selection Z-A has a flaw.

Fixed.

> Also, if someone could please enlighten me why clicking "Sort..." with mouse
> does NOT close the panel, but using Alt+o access key does. And I seem to
> remember that at some stage it did NOT do that, but that's also seen with
> only bug 663695, so it's not caused here.
Attachment #8929377 - Attachment is obsolete: true
Attachment #8929377 - Flags: review?(acelists)
Attachment #8931910 - Flags: ui-review+
Attachment #8931910 - Flags: review?(acelists)
Comment on attachment 8931910 [details] [diff] [review]
Patch V.1.1: Enable attachment sorting always, with dynamic labels

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

So much code for a supposedly trivial change :)

I see you rename functions and attributes and play with the labels.
But can you merge attachmentsGetSortedArray with attachmentsSelectionGetSortedArray and attachmentsSelectionGetSortOrder with attachmentsGetSortOrder? They do the same just on a different input set. You could use an argument as in attachmentsGetSortOrder.

And we should get the tests finished ;)
Attachment #8931910 - Flags: feedback+
Blocks: 1416474
Flags: in-testsuite?
OS: Unspecified → All
Hardware: Unspecified → All
(In reply to :aceman from comment #5)
> Comment on attachment 8931910 [details] [diff] [review]
> So much code for a supposedly trivial change :)

Indeed... the "Reorder Attachments" panel of my bug 663695 required 1000 new lines...

> I see you rename functions and attributes and play with the labels.
> But can you merge attachmentsGetSortedArray with
> attachmentsSelectionGetSortedArray

I had deliberately not merged them because it minimally reduces micro-performance and I didn't want a two-argument function.
But we can do it to shrink code (as seen in this patch).

> and attachmentsSelectionGetSortOrder with
> attachmentsGetSortOrder?

These are already merged, as you say. Do you want to eliminate the one-liner container functions, too?

I want to keep these container functions:
> attachmentsSelectionGetSortedArray()
> attachmentsSelectionGetSortOrder()
- They are just one-liners calling the generic functions with aSelectedOnly = true argument.
- They improve readability a lot, because "true" arguments are not readable without looking them up on the function itself.
- They mimic an object model structure:
attachments.getSortedArray()
attachments.selection.getSortedArray()

> They do the same just on a different input set. You
> could use an argument as in attachmentsGetSortOrder.

I was shy to use two arguments, but as it's wrapped in a container, and otherwise using sane default values, it works without more caller complexity.

> And we should get the tests finished ;)

Sorry, I haven't had time to get round to tests. Maybe we could land simple tests and do the rest later.
Attachment #8931910 - Attachment is obsolete: true
Attachment #8931910 - Flags: review?(acelists)
Attachment #8933566 - Flags: review?(acelists)
Comment on attachment 8933566 [details] [diff] [review]
Patch V.2: (streamlined; nitfixes) Enable attachment sorting always, with dynamic labels

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

Ok, I will probably survive the one-line wrappers because you call them from multiple places.
Thanks :)
Attachment #8933566 - Flags: review?(acelists) → review+
Check this in before it rots?
Keywords: checkin-needed
Sure, it is ready.
We have the tests in bug 1416474.
Flags: in-testsuite? → in-testsuite+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d9a5bbca4663
Enable attachments sorting regardless of selection. r=aceman, ui-r=Paenglab
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Thomas, please supply a full patch next time, this one had no user/author. I added it.
Target Milestone: --- → Thunderbird 59.0
(In reply to Jorg K (GMT+1) from comment #11)
> Thomas, please supply a full patch next time, this one had no user/author. I
> added it.

Thank you, and thanks everyone! Technical problem of TortoiseHg, see pm.
You need to log in before you can comment on or make changes to this bug.