444179 - Library>Views>Sort>Sort by Tags does nothing

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Tracy Walker [:tracy]]

Seen with 3.0.1 build on Linux and 3.0 build on Mac

STR:
1) Have at least a few various tagged bookmarks in a folder.
2) Go to Library and select that folder
3) Click Views>Sort>Sort by Tags

tested results:
nothing happens

expected results:
The view pane should show tagged bookmarks in that folder alpha sorted (see bug 443745 for issue about correct order). The tagged bookmarks bubble to the top of the list, leaving untagged bookmarks at the bottom of the list.

Comment 1

[Dietrich Ayala (:dietrich)]

Would like to see this fixed, but not holding the 3.1 release for it.

Comment 2

[Tracy Walker [:tracy]]

Would it make sense to suppress that selection until it is fixed?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

bug 452000 is adding a unit test that could be used to test this in future too.

Comment 4

[Shawn Wilsher :sdwilsh]

I'm getting this in the error console when trying this too:
Error: uncaught exception: Invalid Column

Comment 5

[Drew Willcoxon :adw]

Attachment: patch and test case

This patch and test also cover Bug 443745; these two bugs are similar and touch the same code.

I anticipate a couple of potential issues with the patch:

1) Using colLookupTable instead of a switch statement as in the original.  I claim this is better than a switch because: A) it's more concise, B) there's no duplication of code as there was previously, C) it's easier to modify should we add a new column or want to change a default sort, and D) IMHO it's easier to read.

2) Throwing if the sort direction is invalid.  The code currently just picks "descending".  But since we're already throwing if the column is invalid, I think it's more consistent to throw if the direction is, too.  It does not appear that any Mozilla code is passing in invalid values.

If I'm being too pretentious in either case, just tell me, I'll change it. :)

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #5)
> This patch and test also cover Bug 443745; these two bugs are similar and touch
> the same code.

i still have to look into it, hwv for testing sort of tags column please add also to queries/test_sorting.js

also, please name your test with a meaningful name, like test_sort_in_library.js, i know we didn't do that, we should start doing it though.

> 2) Throwing if the sort direction is invalid.  The code currently just picks
> "descending".  But since we're already throwing if the column is invalid, I
> think it's more consistent to throw if the direction is, too.  It does not
> appear that any Mozilla code is passing in invalid values.

columns can have three states, sorted asc, sorted desc, unsorted, i guess this is going to throw when moving from sorted asc to unsorted, we call setSortColumn with a null direction in places.xul.
In your case if aDirection is not set you assign a default from your table, so i don't see the need to throw unless the table is wrong.

Comment 7

[Drew Willcoxon :adw]

Attachment: patch and test case

(In reply to comment #6)
> i still have to look into it, hwv for testing sort of tags column please add
> also to queries/test_sorting.js

Done, but it looks like queries/head_queries.js is not very useful at the moment: its compareArrayToResult function that the tests in queries/test_sorting.js rely on only checks if two query results are the same size.  A couple of lines that do equality testing are commented out.  I can make the tags test pass with wrong data.

> columns can have three states, sorted asc, sorted desc, unsorted, i guess this
> is going to throw when moving from sorted asc to unsorted, we call
> setSortColumn with a null direction in places.xul.
> In your case if aDirection is not set you assign a default from your table, so
> i don't see the need to throw unless the table is wrong.

I reverted to what the code is doing currently -- force descending -- if aDirection is not valid.

Nothing has changed from the current code for the unsorted case (that is, if both aColumn and aDirection are null); the first conditional still catches that, and the sort becomes SORT_BY_NONE.  If only aDirection is null, then we use the current sort direction.  If there is no current sort, then the table is used to select a default sort based on the column.  The current code just selects descending in that case.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #7)
> (In reply to comment #6)
> > i still have to look into it, hwv for testing sort of tags column please add
> > also to queries/test_sorting.js
> 
> Done, but it looks like queries/head_queries.js is not very useful at the
> moment: its compareArrayToResult function that the tests in
> queries/test_sorting.js rely on only checks if two query results are the same
> size.  A couple of lines that do equality testing are commented out. 

wtf, that's not the right way to make tests pass! I'm taking a look, but definately we should fix the function and see why those have been commented out, i see some test fails uncommenting, so we have to fix the tests or eventually the associated bugs :\
Clearly test_sorting.js is actually useless due to that!

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2.0

i've simply added somee fix for test_sorting.js and fixed head_queries. While there i've also found a bug in SORT_BY_ANNOTATION with integers!
So, i'm re-attaching the updated patch, and moving toward review of your code.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 359032 [details] [diff] [review]
patch v2.0

>diff --git a/browser/Components/places/tests/browser/browser_sort_in_library.js b/browser/Components/places/tests/browser/browser_sort_in_library.js
>
>+ * Basically, fully tests sorting the placeContent tree in the Places window.
>

nit: in the Places Library window.

>
>+// Two properties of nsINavHistoryResult control the sort of the tree:
>
>+// sortingMode and sortingAnnotation.  sortingMode's value is one of the
>
>+// nsINavHistoryQueryOptions.SORT_BY_* constants.  sortingAnnotation is just
>
>+// the name of an annotation.

nit: sortingAnnotation is the annotation used to sort for SORT_BY_ANNOTATION_* mode.

>
>+      testSortByDir(win, tree, false);
>
>+      testInvalid(win, tree);
>
>+      finish();

when ending a test you must ensure to clear every change you made, to avoid next tests runs possible fails.
So you should set sorting mode to SORT_BY_NONE (for sanity) and close the Library window before finishing.

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>
>+    // This maps the possible values of columnId (i.e., anonid's of treecols in
>
>+    // placeContent) to the following values, which determine the columns'
>
>+    // sortingMode and sortingAnnotation:

nit: This maps ... to the default sortingMode and sortingAnnotation values for each column.

>+    // Make sure we have a valid sort direction.
>
>+    aDirection = (aDirection || colLookupTable[columnId].dir).toUpperCase();
>
>+    if (aDirection !== "ASCENDING" && aDirection !== "DESCENDING")
>
>+      aDirection = "DESCENDING";

At this point, the only case where this would kick in is if a caller passes a wrong value, i would get rid of the check, null is handled above.
We could also avoid the toUpperCase() call directly setting values to uppercase in all calls and in the table, do you see any reason to not do that?

>diff --git a/toolkit/components/places/tests/queries/test_sorting.js b/toolkit/components/places/tests/queries/test_sorting.js

if you have the time and will, would be cool to check for these tests completeness, at least adding a check for various annotation types, we should also test the fallbacks (eg sorting by keywords items without a keyword should fallback to sort by title). Could be a followup though.

Looks good otherwise, i'll review the next iteration, so please also check my changes.

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> when ending a test you must ensure to clear every change you made, to avoid
> next tests runs possible fails.
> So you should set sorting mode to SORT_BY_NONE (for sanity) and close the
> Library window before finishing.
Closing the window should be sufficient, no?

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

it is actually, but in future we could support remembering the sorting mode

Comment 13

[Drew Willcoxon :adw]

(In reply to comment #10)
> >+    // Make sure we have a valid sort direction.
> >
> >+    aDirection = (aDirection || colLookupTable[columnId].dir).toUpperCase();
> >
> >+    if (aDirection !== "ASCENDING" && aDirection !== "DESCENDING")
> >
> >+      aDirection = "DESCENDING";
> 
> At this point, the only case where this would kick in is if a caller passes a
> wrong value, i would get rid of the check, null is handled above.
> We could also avoid the toUpperCase() call directly setting values to uppercase
> in all calls and in the table, do you see any reason to not do that?

Well, it would mean changing the interface of the function because of an implementation detail, which makes me squeamish.  If you're worried about a performance penalty because of the toUpperCase, perhaps I should revert back to a switch statement?

> >diff --git a/toolkit/components/places/tests/queries/test_sorting.js b/toolkit/components/places/tests/queries/test_sorting.js
> 
> if you have the time and will, would be cool to check for these tests
> completeness, at least adding a check for various annotation types, we should
> also test the fallbacks (eg sorting by keywords items without a keyword should
> fallback to sort by title). Could be a followup though.

I'll take a look.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #13)
> (In reply to comment #10)
> Well, it would mean changing the interface of the function because of an
> implementation detail, which makes me squeamish.  If you're worried about a
> performance penalty because of the toUpperCase, perhaps I should revert back to
> a switch statement?

no, i'm not worried, i was asking myself (and you) the usefulness of that path, clearly that call does not bring us a performance hit (not a noticeable one at least), i only wanted to ensure we are not doing more work than needed. Since we are late in the cycle it's ok to retain the current implementation, the gain would not be noticeable. thanks!

Comment 15

[Drew Willcoxon :adw]

Attachment: patch v3.0

test_sorting.js was a little bit more work than I thought. :)

(In reply to comment #14)
> no, i'm not worried, i was asking myself (and you) the usefulness of that path,
> clearly that call does not bring us a performance hit (not a noticeable one at
> least), i only wanted to ensure we are not doing more work than needed.

As I tried to argue in comment 5, the table approach is IMHO easier on humans than the big switch with duplicated code, and it outweighs whatever small hit is caused by the toUpperCase and two string concats.  But this code is a very small point in the grand scheme of things, so I was saying it's no big deal to me to change it back.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 359208 [details] [diff] [review]
patch v3.0

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>+    //   key:  Sort key in the name of one of the
>
>+    //         nsINavHistoryQueryOptions.SORT_BY_* constants
>
>+    //   dir:  Default sort direction to use if none has been specified
>
>+    //   anno: An annotation if key is "ANNOTATION"

nit: The annotation to sort by, if key is "ANNOTATION"

>diff --git a/toolkit/components/places/tests/queries/head_queries.js b/toolk
>+      if (aArray[i].hasOwnProperty("lastVisit"))
>
>+        do_check_eq(aArray[i].lastVisit, child.time);
>
>+      if (aArray[i].hasOwnProperty("index") && aArray[i].index >= 0)

aArray[i].index != bmsvc.DEFAULT_INDEX would be more clear

>diff --git a/toolkit/components/places/tests/queries/test_sorting.js b/toolkit/components/places/tests/queries/test_sorting.js

the only smal drawback about changes is that having uris and titles mimic themselves makes hard to guess the ordering, for example:
uri: example.com/a title: a
uri: example.com/b title: b
can you tell me if i've sorted by uri or by title?

otherwise, changes are good, please execute a final make check and browser-chrome pass before attaching final patch

Comment 17

[Drew Willcoxon :adw]

Attachment: patch v4.0

Changes re: comment 16

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/5b9a34d22d0c

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 359342 [details] [diff] [review]
patch v4.0

asking approval, minor polish with good test coverage

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this timed out on unit boxes (probably due to interaction with previous tests, focus issues?), i pushed an additional changeset to move using window watcher like other tests (works more reliably locally):
http://hg.mozilla.org/mozilla-central/rev/f1493cf102b9

Actually tinderboxes are under maintenance for a couple hours, if this won't work or badly interact with other tests i'll probably disable tests till we find/fix the cause.

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the previous fixed the timeout, but we also need to unregister window watcher observer:
http://hg.mozilla.org/mozilla-central/rev/441918b004d3

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 359342 [details] [diff] [review]
patch v4.0

a191=beltzner

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6f4d2a016e9d

Comment 24

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090312 Shiretoko/3.1b4pre and  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090312 Shiretoko/3.1b4pre

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090312 Minefield/3.2a1pre and  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090312 Minefield/3.2a1pre

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

pushed some missing bit of the test:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/451da1dd30fc

Comment 26

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.

Comment 27

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv