116329 - XUL sort service should grok nsIRDFBlobs for collation keys

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Chris Waterson]

The XUL sort service should handle nsIRDFBlob objects for collation keys.

Comment 1

[Chase Tingley]

waterson, i'll take this bug if you don't object.

random note:

Presumably, if we encounter blob targets for property arcs that don't have
"?collation=true" on them, we should still treat them as binary blobs and use
CompareRawSortKey() (the alternative is attempting to do a collation compare on
arbitrary binary data...).  

This means that theoretically the dependence on "?collation=true" for fast
sorting is lessened.

Comment 2

[Chris Waterson]

All yours...thanks a ton!

Comment 3

[Chase Tingley]

Attachment: patch

Updates nsXULSortService::CompareNodes() so that it will use a raw compare for
Blobs.	It preserves the old (broken) behavior of doing a raw comparison on
literals for which "?collation=true" was specified for the time being so as not
to break existing uses.

A couple notes about this patch.

1) It deviates slightly from the current code in behavior during type
mismatches -- when node 1 is an integer but node 2 is a string.  The current
code will assign a preferred sortOrder based on which node is of the correct
type.  When rewriting this, I opted instead for the behavior that the
nsXULOutlinerBuilder uses -- just return a sortOrder of 0 in the case of the
mismatch.  This will cause a mismatch on the first property to fall through to
a comparison based on the values of the second property (assuming it exists).

2) It's sort of a shame that the Blob GetValue() doesn't return a const. 
Should I fix this, or is there some reason it should return a raw |PRUint8 *|?

Comment 4

[Chase Tingley]

pushing out.

Comment 5

[Chris Waterson]

Comment on attachment 65842 [details] [diff] [review]
patch

sr=waterson

Comment 6

[Navin Gupta]

We need this for bug 115071

Comment 7

[Navin Gupta]

I had to make minor change to compile this patch but sorting in menus is 
still wrong, doesn't sort the same way as in folder-pane. surely, 
something else is broken because both OutlinerBuilder and SortService
are doing the same thing. Note I have a patch for the client (mail folders)
to use RDFBlob. 

+const PRUint8 *lkey, *rkey;
-PRUint8 *lkey, *rkey;

Comment 8

[Chase Tingley]

Comment on attachment 65842 [details] [diff] [review]
patch

Argh, I forgot to fix that -- this patch has been stale since waterson changed
nsIRDFBlob::GetValue to use const.

Comment 9

[Navin Gupta]

Attachment: same patch w/ minor changes

The problem was we were comparing cellNode1 w/ cellNode1 instead of comparing
cellNode1 w/ cellNode2. this patch works but do we still need isCollationKey1,
isCollationKey2. It seems to me QI for blob would do that.

Comment 10

[Navin Gupta]

We need to check in this one also. 

Comment 11

[Chase Tingley]

nice catch on the cellnode1 stuff.  this patch still needs review from someone
other than me, though.  (rjc?)

You're right, the blob QI should make isCollationKey obsolete (and remove a
bunch of other logic from the file, including a lot of RDF queries).  My plan
was to wait until I was sure that "?collation=true" wasn't being used anywhere
else in the tree, and then rip all that stuff out (see related thread waterson
started on n.p.m.rdf).  If this already the case (i.e. we won't cause any
regressions by removing functionality people depend on), then we could do that now.

Comment 12

[Robert John Churchill]

Comment on attachment 66483 [details] [diff] [review]
same patch w/ minor changes

Not that it matters much, but might want to bound this calculation:
    sortOrder = (lint - rint);
to [-1,1]

r=rjc

Comment 13

[Chase Tingley]

Over to Navin for final cleanup (rjc's comment, indentation) and checkin.

Comment 14

[Navin Gupta]

fix checked in ( as per tingley's last comment)