379745 - Additional template sort options

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin (away Dec 12 - Dec 20)]

Templates should support some additional sorting related options:
  - sort as if the data was a certain type
  - sort case-insensitive vs. case-sensitive
  - sort with only two states instead of three
  - sort in groups, for example containers and then leaves

Comment 1

[Nick P]

This would be a great feature to have and will be a blocker for me adopting built-in templates in some cases.

Comment 2

[Neil Deakin (away Dec 12 - Dec 20)]

Attachment: implement the first three above

Implemented as:

sorthints="integer"
sorthints="matchcase"
sorthints="twostate"

Multiple hints should be separated by spaces.

Or, sortService.sort(element, sortKey, sortHints) and "ascending" and "descending" are just additional allowed hints.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Neil: In case you missed it, this is exactly what I as after in bug 557636 for the new addons manager UI.

This may be wishful thinking, but another possible improvement would be to allow passing a custom comparison function to the .sort() method.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 437895 [details] [diff] [review]
implement the first three above

>+    if (token.EqualsLiteral("matchcase"))

'matchcase' might not be the best name. I have no idea if that means case insensitive or case sensitive. How about "comparecase" or "ignorecase"?


>+XULSortServiceImpl::CompareValues(const nsAString& aLeft,
>+                                  const nsAString& aRight,
>+                                  PRUint32 aSortHints)
>+{
>+  if (aSortHints & SORT_INTEGER) {
>+    PRInt32 err;
>+    PRInt32 leftint = nsDependentString(aLeft).ToInteger(&err);
>+    if (NS_SUCCEEDED(err)) {
>+      PRInt32 rightint = nsDependentString(aRight).ToInteger(&err);
>+      if (NS_SUCCEEDED(err)) {
>+        return (leftint < rightint ? -1 : (leftint > rightint ? 1 : 0));

Can't you just return |leftint - rightint|?

>+      }
>+    }
>+    // if they aren't integers, just fall through and compare strings
>+  }
>+
>+  if (aSortHints & SORT_MATCHCASE) {
>+    return ::Compare(aLeft, aRight);
>+  }
>+
>+  nsICollation* collation = nsXULContentUtils::GetCollation();
>+  if (collation) {
>+    PRInt32 result;
>+    collation->CompareString(nsICollation::kCollationCaseInSensitive,
>+                             aLeft, aRight, &result);
>+    return result;
>+  }

If we ever compare any significant number of values then converting on every call to CompareValues will be quite suboptimal. I think especially the collation code could be very slow. Is this worth optimizing?

Or are you expecting that most of the time few values will be compared and so not worth worrying about?

Apart from that it's looking good.

Comment 6

[[e-mail]]

Comment on attachment 437895 [details] [diff] [review]
implement the first three above

>+    PRInt32 err;
[We can use nsresult these days. (Looks better when used with NS_SUCCEEDED.)]

>+    PRInt32 leftint = nsDependentString(aLeft).ToInteger(&err);
>+    if (NS_SUCCEEDED(err)) {
>+      PRInt32 rightint = nsDependentString(aRight).ToInteger(&err);
This is internal so just make aLeft and aRight const nsString& instead.

(In reply to comment #5)
>(From update of attachment 437895 [details] [diff] [review])
>>+    if (token.EqualsLiteral("matchcase"))
>'matchcase' might not be the best name. I have no idea if that means case
>insensitive or case sensitive. How about "comparecase" or "ignorecase"?
I think it means case sensitive, in which case "noignorecase" might be better.

>>+        return (leftint < rightint ? -1 : (leftint > rightint ? 1 : 0));
>Can't you just return |leftint - rightint|?
Only for small ints. Otherwise you get integer overflow :-(

>>+    collation->CompareString(nsICollation::kCollationCaseInSensitive,
>>+                             aLeft, aRight, &result);
>>+    return result;
>>+  }
>If we ever compare any significant number of values then converting on every
>call to CompareValues will be quite suboptimal. I think especially the
>collation code could be very slow. Is this worth optimizing?
I know that mailnews allocates its sort keys ahead of time (and I remember that the API for doing this used to suck, and I doubled the speed by improving it), but I don't know whether this can be done easily here. (RDF data sources have an easier time because they can provide RDF blob data that does just this.)

Comment 7

[Dave Townsend [:mossop] (he/him)]

Will the case insensitive sorting correctly handle international strings?

Comment 8

[[e-mail]]

Comment on attachment 437895 [details] [diff] [review]
implement the first three above

>+        column->GetAttr(kNameSpaceID_None, nsGkAtoms::sorthints, hints);
>+        sortdirection.AppendLiteral(" ");
>+        sortdirection += hints;
What's the desired effect if you set a sort direction in the hits?

Comment 9

[Dave Townsend [:mossop] (he/him)]

This blocks a Firefox 4 blocker. Is this ready to land? Are there answers to the last few questions here?

Comment 10

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

That would mean it should also be marked as blocking Firefox 4.

Comment 11

[Neil Deakin (away Dec 12 - Dec 20)]

(In reply to comment #8)
> (From update of attachment 437895 [details] [diff] [review])
> >+        column->GetAttr(kNameSpaceID_None, nsGkAtoms::sorthints, hints);
> >+        sortdirection.AppendLiteral(" ");
> >+        sortdirection += hints;
> What's the desired effect if you set a sort direction in the hits?

It would have the same effect if set in the direction or the hints. I don't think this possible case matters much though.

Comment 12

[Neil Deakin (away Dec 12 - Dec 20)]

http://hg.mozilla.org/mozilla-central/rev/3670872bb38a

Comment 13

[Nagy Mária]

[Tracking Requested - why for this release]: