571807 - Numerical categories are not sorted as numbers

Status: RESOLVED FIXED

(Calendar :: Tasks, defect)

Description

[Carl Menezes]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 ( .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.9) Gecko/20100317 Lightning/1.0b1 Thunderbird/3.0.4

I have the following categories setup in lightning: 1,2,3,4,5,6,7,50,99

I sort my task list by category and use this to queue tasks since there is no way to do so at present. 

When I sort by category in ascending order, this is the result:
1,2,3,4,5,50,6,7,99



Reproducible: Always

Steps to Reproduce:
1. Create the following categories : 5,6,7,50,99
2. Create a few tasks. 
3. Assign one category to each task - choose from 5,6 and 50.
4. Sort the vie in the tasks tab by category.
Actual Results:  
tasks with a category of 50 appear before tasks with a category of 6.

Expected Results:  
tasks with a category of 6 should appear before those with a category of 50 because 6 is smaller than 50

The real problem is that category was probably not intended to be used with numbers. However, Category is the only user customizable field that can be used to add information to tasks that the current lightning task management does not provide, like order of execution, which is what I am using the category field for in this case.

However, I think there is a case for recognizing numbers in the category field and sorting by them. The following categories would also cause the same problem:
"build 1.7.0.5", "build 1.7.0.55", "build 1.7.0.6"

Comment 1

[[not reading bugmail]]

That sorting would occur if the data is stored as text format.  As 50 comes after 5 and before 6.

Comment 2

[Stefan Sitter [:ssitter]]

Yes, category names are strings. This means they are sorted and stored as strings. If you want to enforce a specific order you could try to use e.g. "05", "06", "50" instead of "5", "6", "50".

Comment 3

[Carl Menezes]

I realize it sorts categories as strings. 

What it should do is recognize the numbers in a string and alter the sort order accordingly, simply because that's what a non technical user would expect. If you asked a random person to sort 5,6 and 50 in ascending order, what order would they use? 

This is a usability bug, because it is unexpected behavior. Stuff like this needs to "just work".

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Confirming, it shouldn't be too much work to use sort as described above.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Klichka is going to give this a shot.

Comment 6

[[not reading bugmail]]

(In reply to comment #3)
> I realize it sorts categories as strings. 
> 
> What it should do is recognize the numbers in a string and alter the sort order
> accordingly, simply because that's what a non technical user would expect. If
> you asked a random person to sort 5,6 and 50 in ascending order, what order
> would they use? 
> 
> This is a usability bug, because it is unexpected behavior. Stuff like this
> needs to "just work".

Making it just work has always been a problem with most computer databases / data storage / or programming languages, which usually need some kind of custom solution to deal with it.

Comment 7

[klichka]

I'm going to throw out a question: Can we have a new field for priority? It seems like the most elegant solution would be to work around this by making a new category that satisfies the criteria of the client, but does not demand a custom collator. 

I can do either, I'm just thinking of every solution before I implement anything and I feel it might be more efficient. I can ratchet up a special case custom collator if need be. I'm just wondering if the bug report is indicative of the desire for a new feature rather than a retool of the category system seeing as how the client's problem looks like there's an underlying issue of the desire for a new sortable priority field. Possibly called: Task Order or something similar. 

I will continue work on the collator replacement in the meantime however.

Comment 8

[[not reading bugmail]]

I think from my personal experience with datasets, major systems and databases, that another field is the best and easiest way to fix a problem like this before doing custom collator work, as both the inputs and outputs could then point to the new field while the old could be depreciated, but I don't know what the developers would prefer. Typically any custom work is more prone to workarounds in the future.

Comment 9

[klichka]

I agree, also the collator runs into issues since it becomes a lot more complex. When is something a number? 5.00 50Jeff 5Jeff 2morrow 5 5. 5) etc. This would need to be resolved as well before it could be finalized since a non numeric symbol could either mean the end of numeric data, or it could mean it is not numeric data. Then there's the issue of how do you sort those numbers along side other values:

5, 5jeff, 6, 7, 50, 99
or 
5, 6, 7, 50, 99, 5jeff

Comment 10

[[not reading bugmail]]

Unfortunately the solution doesn't have to follow database design fundamentals of normalization and proper use, only if that was part of the UX would it make this easier too.  In this case the bug report is specific to asking to do provide custom sorting rules/code in one field type.  

Im not sure how managing two field types would work in the UI, except as master preferences or by some other form of application, but it would provide user choice and power to always do one field type or the other if there were a more elegant UI/UX solution around this.

Comment 11

[Carl Menezes]

Regarding sorting 5.00 50Jeff 5Jeff 2morrow 5 5. 5 :

Speaking from the point of view of a user who doesn't know about normalization, I'd expect the sorting to occur the way it does on the file system - like how file names are sorted, which on the Mac is : 
2morrow, 5, 5., 5.00, 5Jeff, 50

I'm not on Windows, but I'm assuming that the sorting is similar.

In terms of custom fields, the way I imagine it would work is similar to the way Google contacts work - where you can add extra fields, but the format is known - like telephone: other telephone, address: other address. So maybe, if there was a field that sorted numerically, you have the option of adding a custom field of that type.

Comment 12

[Jim Porter (:squib)]

Should this sorting method be used for the other string columns too?

(In reply to comment #9)
> Then there's the issue of how do you sort those numbers along side other 
> values:
> 
> 5, 5jeff, 6, 7, 50, 99
> or 
> 5, 6, 7, 50, 99, 5jeff

I think I mentioned this on IRC, but I'd call parseInt() (or parseFloat()) on the field and sort by that and if the integer values are equal, fall back to string comparison. Unfortunately, this would cause problems with sorting "5 - foo" and "05 - bar", but in the absence of strtod/strtol in Javascript, this seems like the easiest way.

Comment 13

[Stefan Sitter [:ssitter]]

(In reply to comment #7)
> I'm going to throw out a question: Can we have a new field for priority?

Already exists, see <http://tools.ietf.org/html/rfc5545#section-3.8.1.9>
Lightning maps the integer value to string low/medium/high in the user interface. Maybe you can write your own extension that makes the integer values 0...9 directly visible and sortable.

Comment 14

[Andrew]

Hi, 
Can I be assigned this bug?

Comment 15

[Pavan Karthik [:matrixisreal]]

Hi,Is the bug OS Specific
I have cloned the mozilla-central in my linux machine.
If i can still work on this bug,please assign it to me, thanks

Comment 16

[Clara Moraes Dantas]

Hi! =)
I'm a new contributor and I would like to work on this bug, what should I do?

Comment 17

[[:MakeMyDay]]

Clara and Pavan, thank you for showing interest to work on this bug.

@Pavan: This bug is not OS specific. Are you still intersted in working on this bug? You would have the first shot for showing up earlier, but I have seen you started to work on two other bugs already. If you're already busy with that, I would assign it to Clara. 

Please note that calendar lives in comm-central not mozilla-central, so you would need to clone cc as well to build Thunderbird and Lightning. If you have difficulties to setup your environment, don't hesitate to aks in #maildev or #calendar on irc. 

@Clara: if Pavan still wants to work on this bug, you might want to have a look for another good first calendar bugs [1] or if nothing seems to fit just ask on #calendar so we can find a suitable first bug for you. Did you already have setup your environment?

[1] https://bugzilla.mozilla.org/buglist.cgi?order=Importance&list_id=13344217&resolution=---&status_whiteboard_type=allwordssubstr&query_format=advanced&status_whiteboard=%5Bgood%20first%20bug%5D&product=Calendar

Comment 18

[Pavan Karthik [:matrixisreal]]

Hi, I may not work on this right now, Let Clara work on this. :)

Comment 19

[Clara Moraes Dantas]

Thanks, Pavan! :)

@MakeMyDay: Yes, I have set up my environment, what do I do next?

Comment 20

[[:MakeMyDay]]

Clara, can you ping me on #calendar to help you to get traction?

Comment 21

[Pavan Karthik [:matrixisreal]]

Hi,
I see Clara isnt working on this, So can i take this now?

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Whoever fixes it first gets the prize :-P

I see you've asked to be assigned to a few other bugs, but if this is your favorite please go ahead. I don't recall hearing from Clara, but if she wants to work on this bug I'm sure she will comment. Otherwise I'm sure I can find any amount of other good first bugs to work on! 

Please find me on irc.mozilla.org #calendar with nickname Fallen. You can also email me or post in the mozilla.dev.apps.calendar newsgroup.

Comment 23

[Pavan Karthik [:matrixisreal]]

Attachment: v1.patch

Hi Philipp,
Hope this looks Okay.
The fix turned out simpler than I thought. :)
Having said that where is my prize that you have mentioned. :P

Comment 24

[Pavan Karthik [:matrixisreal]]

Hey fallen,
Just found some time after college.
Do check with the patch, and let me know If you have any other interesting bugs for me.
Anyway ill ping you on IRC later.

pavan

Comment 25

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8823440 [details] [diff] [review]
v1.patch

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

This looks great, sorry for the delay! Can you upload a new patch with the following minor nit fixed?

::: calendar/base/modules/calUtils.jsm
@@ +585,5 @@
> +                        return seA.localeCompare(seB, locale,
> +                                                 {numeric: true, sensitivity: 'base'});
> +                    } else {
> +                      return seB.localeCompare(seA, locale,
> +                                               {numeric: true, sensitivity: 'base'});

Please add spaces around in brackets, i.e. { numeric: true, sensitivity: 'base' }

Comment 26

[Pavan Karthik [:matrixisreal]]

Attachment: v1.1.patch

Fixed the nits.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/78406bb70ae5
Numerical categories are not sorted as numbers. r=philipp

Comment 28

[Jorg K (CEST = GMT+2)]

Backout:
https://hg.mozilla.org/comm-central/rev/434e56e5ffc732a536ea8e3d426449a9231b85eb

That caused massive test failures since Locale.jsm does not exist any more:

06:40:21     INFO -  JavaScript error: resource://calendar/modules/calUtils.jsm, line 10: NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]

Comment 29

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Sorry for the hassle! Pavan, if you have time to update the patch that would be superb.

Comment 30

[Pavan Karthik [:matrixisreal]]

I'm sorry I cannot work on this until two months from now, as I have lost the local build. Meanwhile, anyone could feel free to fix this.

Comment 31

[Arshad Kazmi [:arshadkazmi42]]

I am interested to work on this.
Is it still available?

Comment 32

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

Hi Arshad, thank you for your interest in fixing this bug. Yes, it's still available.

Comment 33

[Jorg K (CEST = GMT+2)]

You can rework the existing patch eliminating the use of Locale.jsm and then attach a new patch.

Comment 34

[Arshad Kazmi [:arshadkazmi42]]

ok. thanks. will look into it later today

Comment 35

[Arshad Kazmi [:arshadkazmi42]]

where can I find the code base of this product

I have Mozilla Central setup in my local. Is it not the part of mozilla centra?

Comment 36

[Jorg K (CEST = GMT+2)]

It's not. It's comm-central code for Thunderbird or Calendar/Lightning.
https://dxr.mozilla.org/comm-central/source/
https://hg.mozilla.org/comm-central
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Thunderbird_build

To build Thunderbird and Calendar/Lightning you need to check out two repositories. If you already have M-C, getting C-C as per the instructions I linked is easy.

Comment 37

[Arshad Kazmi [:arshadkazmi42]]

I am focusing in WebExtensions bugs for now. Its a bit difficult for me to build two projects in my laptop. So anyone who is interested in this can take this up.

Comment 38

[Srujana Peddinti [:srujana121]]

Can I take up this bug?

Comment 39

[Jorg K (CEST = GMT+2)]

Sure you can, hints in comment #33.

Comment 40

[Srujana Peddinti [:srujana121]]

what's the expected output for this? 5 ,5abc ,6 ,7 ,56abc ,apple ,bat

Comment 41

[Jorg K (CEST = GMT+2)]

Hi, did you just ask on IRC?

Doesn't "regular" compare just work? There's an approved patch here which uses localeCompare(). The problem the patch had is that Locale.jsm doesn't exist any more but localeCompare() still does. Sure, when you sort 1, 10, 2, 20 as strings you get: 1, 10, 2, 20. But that's always been the case. Am I missing something?

Comment 42

[Srujana Peddinti [:srujana121]]

(In reply to Jorg K (GMT+1) from comment #41)

Hi, did you just ask on IRC?

Doesn't "regular" compare just work? There's an approved patch here which uses localeCompare(). The problem the patch had is that Locale.jsm doesn't exist any more but localeCompare() still does. Sure, when you sort 1, 10, 2, 20 as strings you get: 1, 10, 2, 20. But that's always been the case. Am I missing something?

Yes, I just asked it on IRC. I think the OP feels that in case of only numbers it would be more intuitive to sort it in ascending order of numbers ie 1,2,10,20.

So if we were to implement that feature, what happens when there is a mixture of numbers and characters? Should I
a. put all the numbers first and then strings or
b. should I sort it as strings. Sort as numbers only when ever element is a number ?

Comment 43

[Jorg K (CEST = GMT+2)]

Sorry, my comment #41 was pretty much nonsense :-( - Now I read the hole bug. Yes, this is about sorting the numbers correctly. Some suggestions were already made in comment #12.

However, the approved (and non-functional) patch uses localeCompare(). Does that sort numbers correctly?

Comment 44

[Srujana Peddinti [:srujana121]]

(In reply to Jorg K (GMT+1) from comment #43)

Sorry, my comment #41 was pretty much nonsense :-( - Now I read the hole bug. Yes, this is about sorting the numbers correctly. Some suggestions were already made in comment #12.

However, the approved (and non-functional) patch uses localeCompare(). Does that sort numbers correctly?

Yes it did then. The code seems to make sense. I also wanted to aks something, should I override the collator or can I write a new function entirely in the same file?

Comment 45

[Jorg K (CEST = GMT+2)]

That's a Calendar question best directed to a Calendar peer.

Comment 46

[Srujana Peddinti [:srujana121]]

Attachment: Bug 571807 - Numerical categories are sorted as numbers.

Localecollator sorts the numbers as strings. So localecollator is replaced by localecompare function with numeric option set true.

Also implemented another function alphanumericCompare( ) which sorts all the numeric values before text values.

Comment 47

[Richard Marti (:Paenglab)]

Srujana, you should ask Fallen for review. Without, this patch gets no attention.

Comment 48

[Srujana Peddinti [:srujana121]]

Fallen, Can you please review my patch?
Thanks,
Srujana

Comment 49

[Srujana Peddinti [:srujana121]]

Can you please review my patch? I am applying for outreachy and the deadline is fast approaching. Thank you!

Comment 50

[Jorg K (CEST = GMT+2)]

Looks like the patch was already reviewed by our (inofficial?) Calendar peer Geoff (:darktrojan). So no further reviews required, this is ready to go.

I'll merge it into the code base tonight.

Comment 51

[Jorg K (CEST = GMT+2)]

Attachment: D22978.patch

Fixed commit message.

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3e648008d386
Use localeCompare() with numeric option to sort categories. r=darktrojan

Comment 53

[Jorg K (CEST = GMT+2)]

First bug, thanks for your contribution!

Comment 54

[Jorg K (CEST = GMT+2)]

Philipp, we need Target Milestone 6.10 here.

Comment 55

[Jorg K (CEST = GMT+2)]

Correction: 7.0.