392497 - search in history sidebar loses sort

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Tuukka Tolvanen (sp3000)]

trunk firefox 20070812, linux. me being lazy, here's a copy of bug 364325 comment 0:

Steps to reproduce:

- start a new profile with blank history
- open the sidebar, set "View" to "By Last Visited"
- visit https://bugzilla.mozilla.org/show_bug.cgi?id=257334
- visit https://bugzilla.mozilla.org/show_bug.cgi?id=362003
- verify that bug 362003 is on top in the history sidebar (that's correct)
- now do a history search for "history.dat" in the sidebar

Result: both entries change position: the last visited address is no longer on
top (like in Firefox 2.0)

Comment 1

[Gummi]

I don't see this with 3.0b5pre 2008031504, although I see the reverse at #423296 .

Comment 2

[Gummi]

My bad, I misunderstood, the bug is still there.

Comment 3

[Thomas Bertels]

Attachment: Keep sort order when searching in sidebar history

This keeps the sort order for the lastvisited and visited sort orders.

Comment 4

[Thomas Bertels]

This is a regression from Firefox 2.

Comment 5

[Samuel Sidler (old account; do not CC)]

Mano, do you have time to review this?

Comment 6

[Mike Connor [:mconnor]]

nice to have, not an issue we're going to prioritize for 3.1

Comment 7

[David Balažic]

Why is this a separate bug ?
This is a bug on all platforms. Even if bug 364325 is currently marked as fixed. It is not.
See my comment there.

Comment 8

[Thomas Bertels]

(In reply to comment #7)
I don't see which comment you're talking about, but yes it's reproduceable everywhere.

Comment 10

[Thomas Bertels]

Sorry for bugspam, the culprit seems to be actually bug 389782.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 309991 [details] [diff] [review]
Keep sort order when searching in sidebar history

r=mano code wise, please seek for ui-review.

Comment 14

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

I'm a little confused on this, wouldn't relevance be the most important sort order when searching? (or is our search really just more of a filter at the moment?)

Comment 15

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

Comment on attachment 309991 [details] [diff] [review]
Keep sort order when searching in sidebar history

We can disgrard my previous comment, ranking and sort are two different things, so if we eventually rank based on
frecency (like the location bar does) that is an independent decision from
maintaining sort, which we should do either way.  (The exception being "most
visited" and "last visited" which would of course still effect ranking.)

Comment 16

[Marco Bonardo [:mak]]

Attachment: unbitrot

Comment 17

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/14997a124841

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 358378 [details] [diff] [review]
unbitrot

asking approval, low risk

Comment 20

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

Can we get a test for this, please?

Comment 21

[Marco Bonardo [:mak]]

Attachment: browser-chrome test

yes, we can!

Comment 22

[Drew Willcoxon :adw]

Comment on attachment 369407 [details] [diff] [review]
browser-chrome test

Only some nits:

>+var pages = [
>+  "http://sidebar.mozilla.org/a",
>+  "http://sidebar.mozilla.org/b",
>+  "http://sidebar.mozilla.org/c",
>+  "http://www.mozilla.org/d",
>+];

A comment here explaining that these are listed by descending visit date would be useful.

>+const FILTERED_COUNT = 1;

A comment about how this is used.

>+      sidebar.contentDocument.getElementById("bylastvisited").doCommand();

A comment here like, "sort by visit date descending."

>+      //sidebar.contentDocument.contentWindow.searchHistory(this.value);

Did you mean to leave this in?

>+  is(rc, aExpectedRows, "Rows found: " + rc);
>+  var columns = tree.columns;
>+  is(columns.count, 1, "Columns found: " + columns.count);
>+  for (var r = 0; r < rc; r++) {
>+    var node = treeView.nodeForTreeIndex(r);
>+    is(node.uri, pages[r], "node is correct: " + node.uri);
>+  }

I think the strings in is() should be a description of what you expect, like, "there should be only one column in tree", "position of node in tree should be the same as the order it was visited in".  On failure the harness prints out the first argument to is() anyway right?

r=adw

Comment 23

[Marco Bonardo [:mak]]

Attachment: test for checkin

fixed Drew's comments

Comment 24

[Marco Bonardo [:mak]]

test pushed
http://hg.mozilla.org/mozilla-central/rev/ead10c322220

Comment 26

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

Comment on attachment 358378 [details] [diff] [review]
unbitrot

a191=beltzner, please make sure the test lands with it

Comment 27

[Marco Bonardo [:mak]]

landed patch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c51d6084775c
and test:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/df52eb41dbcc

Comment 28

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre ID:20090608122057

and

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

as well as Fx3.5RC1 on win XP

Comment 29

[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