4731 - Sort order strange

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P3)

Description

[esther]

Seamonkey 1999040710 win32 April 7 build
Hardware: Win95
1. Launch apprunner
2. Open Messenger via Task menu
3. Select a Folder that has several messages
4. Click on the the Sender column.  The sort happens, but the order is not
alphabetical or even recognizable.  (If you can't see the problem using one of
your folders, go to this test account (qatest04) and sort the Inbox.)

Comment 1

[Scott MacGregor]

virtual phil --> re-assigning to scott putterman.

scott this is the problem we were talking about at lunch where rdf is using
strcmp against the collation keys we are giving them. I'm assuming you'll
eventually pass this on to rdf (Robert Churchill?).

Comment 2

[scottputterman]

Robert,

This is what I've written you about in the messages I've been sending.

From the message from Naoki:

I fount that the generated keys are compared by nsString.Compare whilch uses
strcasecmp.
Generated keys are supposed to be compared by a comparison funcion in the
collation interface (strcmp is very bad because it
not only compares wrong but  it will lower case the generated binary key).
The function below need to call the CompareSortKey in nsICollation.
I know that generated key for Linux may contain zero, that's probably why Linux
looks bad.
Anyway, would you file a bug for this and assign to whoever owning the code.

Comment 3

[scottputterman]

*** Bug 4735 has been marked as a duplicate of this bug. ***

Comment 4

[lchiang]

*** Bug 4898 has been marked as a duplicate of this bug. ***

Comment 5

[scottputterman]

Robert,

any ideas on how we can solve this?

Comment 6

[Ninoschka Baca]

Linux build 04-20-08 has the sorting problem.
(NT4 build 04-20-10 doesn't have the sorting problem.)

I have 52 messages in the Inbox, sorted by Subject and their order is:

  - Return Receipt
  - Earth.gif
  - Wednesday
  - Ivy.gif
  - Friday
  - Blank messages (which have Parsing Errors)

  I sorted on Subject again and it just reversed the order:

  - Blank messages (which have Parsing Errors)
  - Friday
  - Ivy.gif
  - Wednesday
  - Earth.gif
  - Return Receipt

Comment 7

[scottputterman]

It's reversing the order because when you click on it once it sorts it ascending
and then when you click on it again it sorts descending. So that appears to be
working (even if the sorting isn't).

Comment 8

[Robert John Churchill]

I believe the real problem isn't with the sorting code but with the XUL and
JavaScript in

    mozilla\mailnews\ui\messenger\resources\threadPane.xul

Take a look at

    mozilla\rdf\resources\sidebar.xul

(near the end of the file) and note how every <XUL:TREECELL> node contains a
couple of <XUL:OBSERVES> nodes which observe the "sortActive" and
"sortDirection" properties of the respective column elements.

I suspect that  threadPane.xul  needs to do the same thing for sorting to work
properly.  :^)

I'm reassigning this back to you, Scott, to try it out.

Comment 9

[scottputterman]

I can look into that, but I'm pretty sure sorting is taking place given that on
Windows sorting pretty much works correctly, and your code is calling my
datasource with "property?=sort".

I think there is a problem with the fact that we are returning collation keys
and you are using strcmp not CompareSortKey.

I think we need the ability for the datasource to provide the compare function
so that those of us who use collation keys can compare them correctly and those
datasource that don't use collation keys can just use strcmp.

Comment 10

[scottputterman]

I need to verify that it's not my fault.  I'm pretty sure it's not.  Anyway,
it's doubtful this will get fixed for M5. I'm moving it to M6.

Comment 11

[scottputterman]

I'm reassigning this back to rjc.  I'm pretty sure there's a problem with the
current RDF sorting architecture or I'm not sure how to adapt it to our needs.

First of all, I don't think the lack of observers is making any difference.  I'm
not sure what the observers are for, but I'd imagine they'd be for making the UI
show up correctly in the column headers.

Second, I went to rjc's sort code and changed his compare to use the sort key
compare in the intl code.  Sure enough message sorting worked just fine, much
better than when using the current code.

The problem is that we need to use the collation sort key compare.  But we
shouldn't have to require everyone who wants to play in RDF to use collation
keys.  We also shouldn't prevent those of us who want to from using collation
keys.  So either we need to add a way to tell RDF that we're using collation
keys or RDF has to allow us to specify our compare function so that we can use
collation keys if we want to.

Comment 12

[Robert John Churchill]

Note: I'm slowly adding support for collation keys. It might fully make it into
M6 and it might not.

Comment 13

[Robert John Churchill]

Scott, the XUL sort code has been changed to now first ask for a column
attribute with "?collation=true" appended to it.  If you change the mail/news
code to provide an answer for this request, and two collation keys are to be
sorted, the collation service will be used instead of a straight string
comparison.  I'm reassigning this back to you... let me know if you have any
problems.

Comment 14

[scottputterman]

I have changes for mail that I will check in.  But this still doesn't work.

In XULSortServiceImpl::GetNodeValue(nsIContent *node1, nsIRDFResource
*sortProperty, sortPtr sortInfo, nsString &cellVal1, PRBool &isCollationKey)


nsCOMPtr<nsIRDFResource>	res1 = do_QueryInterface(dom1);
// Note: don't check for res1 QI failure here.  It only succeeds for RDF nodes,
// but for XUL nodes it will failure; in the failure case, the code below gets
// the cell's text value straight from the DOM

Is always returning res1 = null.  Therefore I never get called with
?collation=true.

Reassigning back to rjc.

Comment 15

[scottputterman]

I changed the line I mentioned above to

	nsCOMPtr<nsIRDFResource>	res1;
	rv = dom1->GetResource(getter_AddRefs(res1));
	if(NS_FAILED(rv))
		res1 = null_nsCOMPtr();

and everything worked great.  I got the same sort order as 4.5.  Chris had
mentioned not wanting us to use GetResource from the dom node, but since we
already know it's a DOMXULNode, it should be ok.  I'm not sure setting res1 to
null is necessary, but I was trying to duplicate the other code.  Anyway, feel
free to add this or do it some other way.  When the tree opens, I'll check in my
changes, and if you can check in a fix for this, this should work.

Comment 16

[Robert John Churchill]

Scott, I checked in your change. Thanks!

Comment 17

[fenella]

RE: Win32 on Win_Nt 4.0 and win 95
In the single account, sort order does not work the first time.  But it works
the second time and thereafter.
In a multiple account, sort order also does not work the first time.  When I
switch from one account to another, it also shows the first sort does not work.
Since the second sort sorts alphabetically, I am verifying this bug and open a
new bug for the new problem.

Comment 18

[scottputterman]

This bug is fixed.  The problems that Fenella is seeing is due to 2 things.  One
is a duplicate of 6670.  Add further sorting to the list of things that get
messed up when we sort.  The other is that we are sorting by Author, not by
sender name even though we are displaying the sender's name.  So the sort order
may not look correct, even though it is, the first time.  Amusingly enough, even
though it's the 2nd time onward that looks ok, it's really the 2nd time onward
that is acting incorrectly.

Comment 19

[fenella]

I am re-opening this bug pending Scott P. 's finding.  What we (Scott and I) is:
The first sort was sorted by the From field, not the text field as it supposed
to in the Sender column. He said it could be the same bug. Scott told me not to
write a new bug report.

Comment 20

[scottputterman]

This one is fixed.  I added more info to 6670 discussing this.

Comment 21

[fenella]

I have written a new bug 7028 to address the sort by From field issue. mark this
one verified.