Clicking on column headers doesn't change the sort.

RESOLVED FIXED in Thunderbird 3

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: standard8, Assigned: Bienvenu)

Tracking

(Blocks: 1 bug, {regression})

Trunk
Thunderbird 3
regression
Bug Flags:
blocking-thunderbird3.0a2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

10 years ago
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.
Flags: blocking-thunderbird3.0a2+
(Reporter)

Comment 1

10 years ago
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.
Depends on: 440616

Comment 2

10 years ago
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
(Assignee)

Comment 3

10 years ago
I can fix this if Joey can't get to it right away...
Assignee: nobody → bienvenu
(Assignee)

Comment 4

10 years ago
I'm just going to add a helper routine MsgSortThreadPaneByType - easier than changing all the callers...
Status: NEW → ASSIGNED
(Assignee)

Comment 5

10 years ago
Created attachment 326697 [details] [diff] [review]
fix for regression

this just makes a common helper routine for callers that already have a real sort type.
Attachment #326697 - Flags: review?(bugzilla)
(Reporter)

Comment 6

10 years ago
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.
Attachment #326697 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 7

10 years ago
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
OS: Mac OS X → All
Hardware: PC → All
Target Milestone: --- → Thunderbird 3

Comment 8

10 years ago
Created attachment 326794 [details] [diff] [review]
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.
Attachment #326794 - Flags: superreview?(bienvenu)
Attachment #326794 - Flags: review?(bienvenu)
(Assignee)

Comment 9

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

yes, I'm ok with this...
Attachment #326794 - Flags: superreview?(bienvenu)
Attachment #326794 - Flags: superreview+
Attachment #326794 - Flags: review?(bienvenu)
Attachment #326794 - Flags: review+

Comment 10

10 years ago
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
Blocks: 440616
No longer depends on: 440616
(Reporter)

Comment 11

10 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 12

10 years ago
Created attachment 327092 [details] [diff] [review]
regression regression fix

This fixes the regression and changes the map to a const, as Standard8 recommends.
Attachment #327092 - Flags: superreview?(bienvenu)
Attachment #327092 - Flags: review?(bienvenu)
(Assignee)

Updated

10 years ago
Attachment #327092 - Flags: superreview?(bienvenu)
Attachment #327092 - Flags: superreview+
Attachment #327092 - Flags: review?(bienvenu)
Attachment #327092 - Flags: review+
(Reporter)

Updated

10 years ago
Duplicate of this bug: 442490
(Reporter)

Updated

10 years ago
Duplicate of this bug: 442646

Comment 15

10 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 442917
You need to log in before you can comment on or make changes to this bug.