Numerical categories are not sorted as numbers

RESOLVED FIXED in 7.0

Status

defect
--
minor
RESOLVED FIXED
9 years ago
3 months ago

People

(Reporter: carleeto, Assigned: Srujana.htt121)

Tracking

({polish})

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 2 obsolete attachments)

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"
That sorting would occur if the data is stored as text format.  As 50 comes after 5 and before 6.
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".
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".
Confirming, it shouldn't be too much work to use sort as described above.
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: polish
Whiteboard: [good first bug]
Klichka is going to give this a shot.
Assignee: nobody → klichka
(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.
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.
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.
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
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.
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.
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.
(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.
Assignee: klichka → nobody
Hi, 
Can I be assigned this bug?
Assignee: nobody → vandrew88
Status: NEW → ASSIGNED
Assignee: vandrew88 → nobody
Status: ASSIGNED → NEW
OS: Windows Vista → All
Hardware: x86 → All
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
Hi! =)
I'm a new contributor and I would like to work on this bug, what should I do?
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
Flags: needinfo?(pavankarthikboddeda)
Hi, I may not work on this right now, Let Clara work on this. :)
Flags: needinfo?(pavankarthikboddeda)
Thanks, Pavan! :)

@MakeMyDay: Yes, I have set up my environment, what do I do next?
Clara, can you ping me on #calendar to help you to get traction?
Hi,
I see Clara isnt working on this, So can i take this now?
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.
Posted patch v1.patch (obsolete) — Splinter Review
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
Assignee: nobody → pavankarthikboddeda
Status: NEW → ASSIGNED
Attachment #8823440 - Flags: review?(philipp)
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
Flags: needinfo?(philipp)
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' }
Attachment #8823440 - Flags: review?(philipp) → review+
Posted patch v1.1.patch (obsolete) — Splinter Review
Fixed the nits.
Attachment #8823440 - Attachment is obsolete: true
Attachment #8828254 - Flags: review?(philipp)
Attachment #8828254 - Flags: review?(philipp) → review+
Flags: needinfo?(philipp)
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/78406bb70ae5
Numerical categories are not sorted as numbers. r=philipp
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.0
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]
Status: RESOLVED → REOPENED
Flags: needinfo?(philipp)
Flags: needinfo?(pavankarthikboddeda)
Resolution: FIXED → ---
Target Milestone: 6.0 → ---
Sorry for the hassle! Pavan, if you have time to update the patch that would be superb.
Flags: needinfo?(philipp)
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.
Flags: needinfo?(pavankarthikboddeda)
I am interested to work on this.
Is it still available?
Flags: needinfo?(philipp)
Hi Arshad, thank you for your interest in fixing this bug. Yes, it's still available.
You can rework the existing patch eliminating the use of Locale.jsm and then attach a new patch.
Flags: needinfo?(philipp)
Assignee: pavankarthikboddeda → nobody
Status: REOPENED → NEW
ok. thanks. will look into it later today
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?
Flags: needinfo?(philipp)
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.
Flags: needinfo?(philipp)
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.

Can I take up this bug?

Assignee: nobody → Srujana.htt121

Sure you can, hints in comment #33.

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

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?

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

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?

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

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

Flags: needinfo?(philipp)

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.

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

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

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

Flags: needinfo?(jorgk)

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.

Flags: needinfo?(philipp)
Flags: needinfo?(jorgk)
Keywords: checkin-needed
Posted patch D22978.patchSplinter Review

Fixed commit message.

Attachment #8828254 - Attachment is obsolete: true
Attachment #9049997 - Attachment is obsolete: true
Attachment #9052994 - Flags: review+

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

Status: NEW → RESOLVED
Closed: 2 years ago4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

First bug, thanks for your contribution!

Target Milestone: --- → 6.9

Philipp, we need Target Milestone 6.10 here.

Flags: needinfo?(philipp)

Correction: 7.0.

Flags: needinfo?(philipp)
Target Milestone: 6.9 → 7.0
Attachment #9049997 - Attachment is obsolete: false
You need to log in before you can comment on or make changes to this bug.