1417856 - Reorder Attachments: Alphabetical sorting should be enabled regardless of selection

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

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

Comment 1

[Thomas D. (:thomas8)]

Attachment: Patch V.1: Enable attachment sorting always, with dynamic labels

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

Comment 2

[Thomas D. (:thomas8)]

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 3

[Richard Marti (:Paenglab)]

Comment on attachment 8929377 [details] [diff] [review]
Patch V.1: Enable attachment sorting always, with dynamic labels

Lgtm, thanks.

Comment 4

[Thomas D. (:thomas8)]

Attachment: Patch V.1.1: Enable attachment sorting always, with dynamic labels

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.

Comment 5

[:aceman]

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 ;)

Comment 6

[Thomas D. (:thomas8)]

Attachment: Patch V.2: (streamlined; nitfixes) Enable attachment sorting always, with dynamic labels

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

Comment 7

[:aceman]

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 :)

Comment 8

[Jorg K (CEST = GMT+2)]

Check this in before it rots?

Comment 9

[:aceman]

Sure, it is ready.
We have the tests in bug 1416474.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d9a5bbca4663
Enable attachments sorting regardless of selection. r=aceman, ui-r=Paenglab

Comment 11

[Jorg K (CEST = GMT+2)]

Thomas, please supply a full patch next time, this one had no user/author. I added it.

Comment 12

[Thomas D. (:thomas8)]

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