unifinder column sort order inconsistent behaviour

ASSIGNED
Assigned to

Status

--
minor
ASSIGNED
7 years ago
7 years ago

People

(Reporter: rochajoel, Assigned: rochajoel)

Tracking

Lightning 1.0

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 565484 [details] [diff] [review]
Patch for correcting this behaviour

User Agent: Opera/9.80 (Windows NT 5.1; U; en) Presto/2.9.168 Version/11.51

Steps to reproduce:

Selected all columns on unifinder (one at a time)


Actual results:

Sort is inconsistent. The first column is sorted ascending the second is sorted descending the third is sorted descending, and so on and so on...


Expected results:

IMHO All the columns should be sorted ascending when selected for the first time. Sorted descending if they are "double" clicked.
Confirming behavior.
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → rochajoel
Status: NEW → ASSIGNED
Comment on attachment 565484 [details] [diff] [review]
Patch for correcting this behaviour

Thanks for the patch. If you add a patch please mark it correctly and request review from a calendar developer.
Attachment #565484 - Attachment is patch: true
Attachment #565484 - Flags: review?(philipp)
(Assignee)

Comment 3

7 years ago
Created attachment 569615 [details] [diff] [review]
Improved routine for performance

the column only gets sorted once. Then it just gets reversed.
Attachment #565484 - Attachment is obsolete: true
Attachment #565484 - Flags: review?(philipp)
(Assignee)

Comment 4

7 years ago
Created attachment 569616 [details] [diff] [review]
Removed TODO comment line

Equal to the previous without the //TODO:
Attachment #569615 - Attachment is obsolete: true
Attachment #569616 - Flags: review?(philipp)
(Assignee)

Comment 5

7 years ago
Created attachment 569621 [details] [diff] [review]
Optimized for readability

Improved for readability, that meant adding a var and an if condition so that the flow of operations can be seen more linearly (and without repeated source code)
Attachment #569616 - Attachment is obsolete: true
Attachment #569616 - Flags: review?(philipp)
Attachment #569621 - Flags: review?(philipp)
Comment on attachment 569621 [details] [diff] [review]
Optimized for readability

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

Joel, does this patch already fix the whole bug? It looks to me as if something is missing? Maybe you could explain a bit more. Thanks!
Attachment #569621 - Flags: review?(philipp) → review-
(Assignee)

Comment 7

7 years ago
Have you tested it? Does not solve the previous behaviour? My last post was made 3 months ago, i do not have the workspace ready to test this (i'm kinda working on something else rigth now, sorry...)

AFAIK, from the diff that can be requested here on the site, the following behaviour was implemented:

//"A priori" when the cycleHeader function is called the column has not been selected yet.
let colAlreadySelected = false;

//Then we try to find out if the column was already selected, if not, the direction of sort is ascending, if the column was already selected we will set colAlreadySelected = true, this will be helpful in the next step. We also have to sort it by the reverse order: if the previous order was descending the current order is ascending and vice versa.

//The next step is to check if the column was already selected, if true, the list is already sorted, we just have to reverse the current list, if not we have to sort it by the assigned column. This way, the sort is only run when the user selects a different column to sort, after that the operation takes much less time, making it more efficient.
(Assignee)

Comment 8

7 years ago
Also note that (and maybe that's what you think it's missing) the only time the sort runs is when the order is ascending (in the first time).
(Assignee)

Comment 9

7 years ago
Did i made myself clear? If not just post any questions, or contact me directly. Thank you!
You need to log in before you can comment on or make changes to this bug.