265282 - [Mac] Unsorted tree columns are drawn as sorted (was: Unable to get Bookmarks columns in unsorted state)

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[Marcia Knous [:marcia]]

Seen using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.3)
Gecko/20041018 Firefox/1.0:

STR:

1. Go to Manage Bookmarks
2. Observe that you can click on a column and get ascending and descending
order, but not unsorted.

I tested this on Windows and it behaves correctly - you can sort in ascending
order, descending order and if you click a third time you are in unsorted mode
with no triangle.

Comment 1

[Ben Fowler]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041107
Firefox/0.9.1+

Clicking to cycle through the states seems to work the way you want it!

More significantly, there is a menu item in the View menu that un-sorts
bookmarks.

Comment 2

[Asaf Romano (gone)]

ooh, this is a nsITheme issue... ("unsorted" does work, but the column header
isn't drawn properly). Some impl' (including ff and seamonkey bookmarks
frontend) assign "natural" to the sort-direction attribute, but
nsNativeTheme::IsSortedColumn only checks for empty/no direction attribute.

Comment 3

[Asaf Romano (gone)]

Attachment: patch

see comment 2

Comment 4

[Boris Zbarsky [:bzbarsky]]

So is "natural" actually a supported value? 

More interestingly, why are we duplicating that logic in various places (as we
clearly are)?  Can't we put it in one shared place somewhere?

Comment 5

[[e-mail]]

"natural" appears to be a valid sort direction, presumably it exists for
persistence purposes, as nsXULTreeBuilder::EnsureSortVariables only checks for
"ascending" and "descending" (is this the duplication to which you refer?)

Comment 6

[Asaf Romano (gone)]

OK, so i guess the Right Thing would be to check if it the direction attribute
value is either "ascending" or "descending"?

Comment 7

[[e-mail]]

I guess so, and while you're there you might return different enumeration values
for natural, ascending and descending which the caller then acts on.

Comment 8

[Asaf Romano (gone)]

(In reply to comment #7)
> I guess so, and while you're there you might return different enumeration values
> for natural, ascending and descending which the caller then acts on.

isn't IsSortReversed used to do this?

Comment 9

[[e-mail]]

Ah, but you don't want to have to check for sortDirection="descending" twice
(once for IsSortedColumn and once for IsSortReversed) do you?

Comment 10

[Asaf Romano (gone)]

Attachment: v2

Comment 11

[[e-mail]]

Comment on attachment 176886 [details] [diff] [review]
v2

>+      TreeSortDirection sortDirection;
>+      sortDirection = GetTreeSortDirection(aFrame);
No point having this on two lines, is there?

The rest looks ok; sr=me with this nit fixed.

Comment 12

[Asaf Romano (gone)]

(In reply to comment #11)
> (From update of attachment 176886 [details] [diff] [review] [edit])
> >+      TreeSortDirection sortDirection;
> >+      sortDirection = GetTreeSortDirection(aFrame);
> No point having this on two lines, is there?
> 
> The rest looks ok; sr=me with this nit fixed.
> 

That would cause a crosses-initialization build error (this is inside a case).

Comment 13

[jhp (no longer active)]

Oh, I see.  In that case, just put { } around each 'case' block.

Comment 14

[Asaf Romano (gone)]

with those changes...

Checking in mac/nsNativeThemeMac.cpp;
/cvsroot/mozilla/gfx/src/mac/nsNativeThemeMac.cpp,v  <--  nsNativeThemeMac.cpp
new revision: 1.38; previous revision: 1.37
done
Checking in shared/nsNativeTheme.h;
/cvsroot/mozilla/gfx/src/shared/nsNativeTheme.h,v  <--  nsNativeTheme.h
new revision: 1.13; previous revision: 1.12
done