Closed Bug 473157 Opened 11 years ago Closed 11 years ago

Want to sort history in container view without sorting the containers

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: neil, Assigned: neil)

References

Details

(Keywords: verified1.9.1)

Attachments

(2 files, 3 obsolete files)

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.)
Attached patch Possible patchSplinter Review
Well, this works for our use case. I can't see how to work what other side-effects it might have though, sorry.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #356513 - Flags: review?(dietrich)
Attachment #356513 - Flags: review?(dietrich) → review+
Comment on attachment 356513 [details] [diff] [review]
Possible patch

looks fine, passes all unit tests, r=me.
Pushed changeset afba4fd650aa to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 356513 [details] [diff] [review]
Possible patch

requesting approval for 1.9.1 branch
Attachment #356513 - Flags: approval1.9.1?
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 356513 [details] [diff] [review]
Possible patch

Can we get tests for this?
Attached patch WIP Test (obsolete) — Splinter Review
This is basically copy and paste... I should really do this in a loop.
Attached patch Alternative test (-w patch) (obsolete) — Splinter Review
Less copy/paste this way.
Attached patch Test (obsolete) — Splinter Review
OK, so I tweaked the first version of the test to have better comments.
Attachment #369399 - Attachment is obsolete: true
Attachment #369402 - Attachment is obsolete: true
Attachment #369492 - Flags: review?(mak77)
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.
Attachment #369492 - Flags: review?(mak77)
Attachment #369492 - Attachment is obsolete: true
Attachment #369549 - Flags: review?(mak77)
Attachment #369549 - Flags: review?(mak77) → review+
Comment on attachment 369549 [details] [diff] [review]
Updated for bitrot

looks good, thanks
Flags: in-testsuite?
test pushed by Neil:
http://hg.mozilla.org/mozilla-central/rev/16c9c6fb7d62
Flags: in-testsuite? → in-testsuite+
Attachment #369549 - Flags: approval1.9.1?
Attachment #369549 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 369549 [details] [diff] [review]
Updated for bitrot

a191=beltzner
Comment on attachment 356513 [details] [diff] [review]
Possible patch

a191=beltzner
Attachment #356513 - Flags: approval1.9.1? → approval1.9.1+
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?
Keywords: fixed1.9.1
Depends on: 488783
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
Status: RESOLVED → VERIFIED
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)
(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.
(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...)
You need to log in before you can comment on or make changes to this bug.