Closed Bug 379745 Opened 13 years ago Closed 10 years ago

Additional template sort options

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b1
Tracking Status
blocking2.0 --- beta2+

People

(Reporter: enndeakin, Assigned: enndeakin)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

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
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
This would be a great feature to have and will be a blocker for me adopting built-in templates in some cases.
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Duplicate of this bug: 557636
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #437895 - Flags: superreview?(jonas)
Attachment #437895 - Flags: review?(neil)
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.
Attachment #437895 - Flags: superreview?(jonas) → superreview+
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.)
Will the case insensitive sorting correctly handle international strings?
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?
Attachment #437895 - Flags: review?(neil) → review+
This blocks a Firefox 4 blocker. Is this ready to land? Are there answers to the last few questions here?
That would mean it should also be marked as blocking Firefox 4.
blocking2.0: --- → ?
(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.
http://hg.mozilla.org/mozilla-central/rev/3670872bb38a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
blocking-b2g: 2.6? → ---
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.