All users were logged out of Bugzilla on October 13th, 2018

MsgNavigation does not follow top-down order in some cases.

VERIFIED FIXED in mozilla0.9.8

Status

P3
normal
VERIFIED FIXED
17 years ago
14 years ago

People

(Reporter: naving, Assigned: naving)

Tracking

Trunk
mozilla0.9.8
x86
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.64 KB, patch
nhottanscp
: review+
sspitzer
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

17 years ago
I have seen some issues here, will file new bug about this. looks
like we might need to do CompareRawSortKey when comparing folder keys 
rather than relying on js (key1 <key2)

fix in hand, I will attach a patch.
(Assignee)

Updated

17 years ago
Priority: -- → P3
Summary: MsgNavigation does not follow top-down order in some cases. → MsgNavigation does not follow top-down order in some cases.
Target Milestone: --- → mozilla0.9.8
(Assignee)

Comment 1

17 years ago
Created attachment 64401 [details] [diff] [review]
proposed fix

the fix as i said is to use CompareRawSortKey rather than key1 <key2. 

seth, please review.
didn't we switch over to ascii safe collation keys?

Did we switch back because of the macos x problems, or did we never switch, and 
I'm just confused?

If we did switch to ascii safe keys, the js check would have worked.  

but since we're doing raw sort keys, we have to use the raw sort key compare.

this patch basically treats the sort keys are PRUnichar *s, and then you cast 
to PRUint8 *.

I thought that was bad, or I remember some comments in the code (from nhotta?) 
about why it was bad.

nhotta, can you clarify?
(Assignee)

Comment 3

17 years ago
It was bad to use PRUnichar*, but we were having problems because rdf
wanted them to be PRUnichar*, so we decided to go this way. I believe
there are lots of bug filed to make all this work correctly. 

This is how OutlinerBuilder does sorting in the folder pane ie casts
from PRUnichar* to PRUint8*




Comment 4

17 years ago
I did not check in the ASCII encoded key support. Providing more than one key
format could cause confusion because the client code has to use different
comparison functions and they are easily be mixed up and causes wrong results. 
Also the current implementation in XUL is fixed to one comparison code and
cannot be changed by the caller, so if one client of the XUL sort switch to
ASCII key then all the ther clients has to switch to ASCII keys. And ASCII keys
solution needs encode/decode, encode might be not significant only once per key,
but decode has to be done per comparison. The current plan is to change XUL sort
to support binary key which is bug 115926.

Updated

17 years ago
QA Contact: esther → laurel

Comment 5

17 years ago
Comment on attachment 64401 [details] [diff] [review]
proposed fix

If you're abandoning .sortKey you could remove that from the .idl and
associated files.
(Assignee)

Comment 6

17 years ago
we still need that because we are asking for sortKey from msgFolderDataSource.
i think we should let it be scriptable. 
I agree with neil, don't make it scriptable.

Casting the raw sort keys from  PRUnichar * to PRUint8 * is evil, as we know.

let's not expose that through XPConnect.

When crossing XPConnect, we copy the strings, and probably call strlen() during 
that process.  That means null bytes in the raw sort key will mess us up.

Let's not expose this evil to JS if we don't have too.

can you amend the patch to make it nonscript?

nhotta, can you provide the r=?

Comment 8

17 years ago
So the patch has a comparison code in the mail code, does this mean this part
does not use RDF? Is this for a folder tree sorting?
(Assignee)

Comment 9

17 years ago
This is just for msgNavigation so that it follows the order in which
the folders appear in the folder-pane. 

Comment 10

17 years ago
So msgNavigation is that the list I can see when I move/copy message?
(Assignee)

Comment 11

17 years ago
Created attachment 64574 [details] [diff] [review]
proposed fix

same patch with sortKey non-scriptable.
Attachment #64401 - Attachment is obsolete: true
(Assignee)

Comment 12

17 years ago
msgNavigation is "Next" in the toolbar. so when you click next it 
should follow top-down order in the folder-pane, as it navigates
into folders with unread messages. 

Comment 13

17 years ago
Comment on attachment 64574 [details] [diff] [review]
proposed fix

r=nhotta
Attachment #64574 - Flags: review+
Comment on attachment 64574 [details] [diff] [review]
proposed fix

looks good.

one suggestion (and brendan would be so proud of me)

+ nsresult rv=NS_OK;

no need to initialize this value.
Attachment #64574 - Flags: superreview+
(Assignee)

Comment 15

17 years ago
fixed
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

17 years ago
To verify this create two local folders '34' and 'aaa' and have unread messages 
in both and see the difference before and after the fix. 

Comment 17

17 years ago
OK using feb04 commercial trunk build: win98, mac OS 10.1, linux rh6.2
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.