441750 - Clicking on column headers doesn't change the sort.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Mark Banner (:standard8)]

I just updated to 2008062403, when I click the column headers, the sort order doesn't change. If I do it from View -> Sort By, then it updates and changes fine.

The last build I was using was 2008062303, but I can't say that I would have changed the sorting order with that build.

Blocking 3.0a2 as I don't think we can ship with such an obvious feature as this being broken.

Comment 1

[Mark Banner (:standard8)]

On a debug build I get:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /Users/moztest/mozilla/hg/mozilla/mailnews/base/src/nsMsgDBView.cpp, line 3847
unsupported sort key: NaN

When clicking the column headers. If I back out attachment 326107 [details] [diff] [review] from bug 440616 (part 4 v1) then it fixes this bug.

Comment 2

[Joey Minta]

Ahh, yeah, MsgSortThreadPane had additional callers that aren't passing the right kind of argument now. We'll need to change http://mxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/threadPane.js#131 and http://mxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/threadPane.js#211

Comment 3

[David :Bienvenu]

I can fix this if Joey can't get to it right away...

Comment 4

[David :Bienvenu]

I'm just going to add a helper routine MsgSortThreadPaneByType - easier than changing all the callers...

Comment 5

[David :Bienvenu]

Attachment: fix for regression

this just makes a common helper routine for callers that already have a real sort type.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 326697 [details] [diff] [review]
fix for regression

I agree, I looked at the callers in HandleColumnClick and we'd actually loose some info (invalid or not) if we left them in text form.

r=me.

Comment 7

[David :Bienvenu]

fix checked in

Comment 8

[Joey Minta]

Attachment: jminta's fix

Sorry folks, I just got home from work.  This was the fix I had in mind when I posted the links this morning.  I think it's a bit cleaner, especially because it gets rid of another annoying case where shared code assumes that forked code will provide a particular function with a particular behavior.  If possible, I like to see this patch swapped in for bienvenu's.

Comment 9

[David :Bienvenu]

Comment on attachment 326794 [details] [diff] [review]
jminta's fix

yes, I'm ok with this...

Comment 10

[Joey Minta]

Alternative fix checked in.

Checking in mail/base/content/commandglue.js;
/cvsroot/mozilla/mail/base/content/commandglue.js,v  <--  commandglue.js
new revision: 1.97; previous revision: 1.96
done
Checking in mailnews/base/resources/content/commandglue.js;
/cvsroot/mozilla/mailnews/base/resources/content/commandglue.js,v  <--  commandglue.js
new revision: 1.286; previous revision: 1.285
done
Checking in mailnews/base/resources/content/threadPane.js;
/cvsroot/mozilla/mailnews/base/resources/content/threadPane.js,v  <--  threadPane.js
new revision: 1.96; previous revision: 1.95
done

Comment 11

[Mark Banner (:standard8)]

The alternative fix did indeed fix the selection of a different sort from one column to another, it broke the the switch from ascending to descending (see the two sections highlighted):

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/base/resources/content/threadPane.js&rev=1.96&mark=151-153,193#150

The problem is that dbView.sortType is an integer, and sortType is now a string...

Therefore, reopening.

Also why isn't the columnMap a const? Given that it never changes, it should be a const to allow the javascript compiler to optimise correctly.

Comment 12

[Joey Minta]

Attachment: regression regression fix

This fixes the regression and changes the map to a const, as Standard8 recommends.

Comment 15

[Joey Minta]

Checking in mailnews/base/resources/content/threadPane.js;
/cvsroot/mozilla/mailnews/base/resources/content/threadPane.js,v  <--  threadPane.js
new revision: 1.97; previous revision: 1.96
done