473157 - Want to sort history in container view without sorting the containers

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[[e-mail]]

When you for example have a view that is grouped by day and site, then changing the result's sortingMode also reorders the day and site containers. (Note that setting the query's options does not currently sort a grouped view.)

Comment 1

[[e-mail]]

Attachment: Possible patch

Well, this works for our use case. I can't see how to work what other side-effects it might have though, sorry.

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 356513 [details] [diff] [review]
Possible patch

looks fine, passes all unit tests, r=me.

Comment 3

[[e-mail]]

Pushed changeset afba4fd650aa to mozilla-central.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 356513 [details] [diff] [review]
Possible patch

requesting approval for 1.9.1 branch

Comment 5

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

Comment on attachment 356513 [details] [diff] [review]
Possible patch

Can we get tests for this?

Comment 6

[[e-mail]]

Attachment: WIP Test

This is basically copy and paste... I should really do this in a loop.

Comment 7

[[e-mail]]

Attachment: Alternative test (-w patch)

Less copy/paste this way.

Comment 8

[[e-mail]]

Attachment: Test

OK, so I tweaked the first version of the test to have better comments.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 369492 [details] [diff] [review]
Test

>diff --git a/toolkit/components/places/tests/unit/test_history_sidebar.js b/toolkit/components/places/tests/unit/test_history_sidebar.js
>+  // Bug 473157: changing sorting mode should not affect the containers
>+  result.sortingMode = options.SORT_BY_TITLE_DESCENDING;
>+  for (var i=0; i<dayLabels.length; i++)
>+  {
>+    var node = root.getChild(i);
>+    do_check_eq(node.title, dayLabels[i]);
>+  }

use this style please, so it's common to Places code:
  for (var i = 0; i < dayLabels.length; i++) {


btw, this patch does not apply to m-c, so you'll need to provide a new one, test has changed recently and changes are most likely going into 1.9.1.

Comment 10

[[e-mail]]

Attachment: Updated for bitrot

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 369549 [details] [diff] [review]
Updated for bitrot

looks good, thanks

Comment 12

[Marco Bonardo [:mak]]

test pushed by Neil:
http://hg.mozilla.org/mozilla-central/rev/16c9c6fb7d62

Comment 13

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

Comment on attachment 369549 [details] [diff] [review]
Updated for bitrot

a191=beltzner

Comment 14

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

Comment on attachment 356513 [details] [diff] [review]
Possible patch

a191=beltzner

Comment 15

[[e-mail]]

Pushed changeset 4113063ae13f to releases/mozilla-1.9.1

(In reply to comment #9)
> test has changed recently and changes are most likely going into 1.9.1.
Please can you push the test with your other test changes for 1.9.1?

Comment 16

[Marco Bonardo [:mak]]

pushed test
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/21a56d7f1497

Comment 17

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090505 Minefield/3.6a1pre ID:20090505030940

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090505 Shiretoko/3.5b5pre ID:20090505030932

Comment 18

[Asaf Romano (gone)]

I'm not sure why SeaMonkey needed this change, but it's plan wrong API-wise, and it broke the Places Organizer UI a little. In particular, see the Tags case in bug 526430.

You could implement the same behavior, I think, by moving sortingMode from the result interface to the container interface[1], which is something I was planning to do long ago.

--
[1] Like this:
readonly attribute sortingMode
setSortingMode(aRecursive)

Comment 19

[[e-mail]]

(In reply to comment #18)
> readonly attribute sortingMode
> setSortingMode(aRecursive)

Ideally I'd like to be able to read the result's sorting mode, even though I'm only sorting item in containers. Maybe an aIgnoreContainers parameter?

Ideally I'd also like to the sorting mode to apply to closed containers too, so that I don't have to manually verify the sorting mode before opening them.

Comment 20

[[e-mail]]

(In reply to comment #19)
> Ideally I'd like to be able to read the result's sorting mode, even though I'm
> only sorting item in containers. Maybe an aIgnoreContainers parameter?
Oh, and this needs to be settable before creating the result, so that we can switch from flat to day view without the days being sorted in (say) name order (obviously this assumes that you're also going to fix the result to generate containers in the defined sorting order...)