Closed Bug 392497 Opened 16 years ago Closed 15 years ago

search in history sidebar loses sort


(Firefox :: Bookmarks & History, defect)

3.0 Branch
Not set



Firefox 3.6a1


(Reporter: tuukka.tolvanen, Assigned: tbertels+bugzilla)



(Keywords: regression, verified1.9.1)


(2 files, 2 obsolete files)

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
- visit
- 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)
Component: History → Places
QA Contact: history → places
I don't see this with 3.0b5pre 2008031504, although I see the reverse at #423296 .
My bad, I misunderstood, the bug is still there.
This keeps the sort order for the lastvisited and visited sort orders.
Attachment #309991 - Flags: review?(mano)
This is a regression from Firefox 2.
Keywords: regression
Flags: wanted1.9.0.x?
Mano, do you have time to review this?
Assignee: nobody → tbertels+bugzilla
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: wanted-firefox3.1?
nice to have, not an issue we're going to prioritize for 3.1
Flags: wanted-firefox3.1? → wanted-firefox3.1-
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.
(In reply to comment #7)
I don't see which comment you're talking about, but yes it's reproduceable everywhere.
OS: Linux → All
Hardware: PC → All
Version: Trunk → 3.0 Branch
Closed: 15 years ago
Resolution: --- → DUPLICATE
Sorry for bugspam, the culprit seems to be actually bug 389782.
Resolution: DUPLICATE → ---
Attachment #309991 - Flags: review?(mano) → review+
Comment on attachment 309991 [details] [diff] [review]
Keep sort order when searching in sidebar history

r=mano code wise, please seek for ui-review.
Attachment #309991 - Flags: ui-review?(mconnor)
Attachment #309991 - Flags: ui-review?(mconnor) → ui-review?(faaborg)
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?)
Attachment #309991 - Flags: ui-review?(faaborg) → ui-review+
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.)
Attached patch unbitrotSplinter Review
Attachment #309991 - Attachment is obsolete: true
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Comment on attachment 358378 [details] [diff] [review]

asking approval, low risk
Attachment #358378 - Flags: approval1.9.1?
Can we get a test for this, please?
Attached patch browser-chrome test (obsolete) — Splinter Review
yes, we can!
Attachment #369407 - Flags: review?(adw)
Flags: in-testsuite?
Comment on attachment 369407 [details] [diff] [review]
browser-chrome test

Only some nits:

>+var pages = [
>+  "",
>+  "",
>+  "",
>+  "",

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?

Attachment #369407 - Flags: review?(adw) → review+
Attached patch test for checkinSplinter Review
fixed Drew's comments
Attachment #369407 - Attachment is obsolete: true
test pushed
Flags: in-testsuite? → in-testsuite+
Comment on attachment 358378 [details] [diff] [review]

a191=beltzner, please make sure the test lands with it
Attachment #358378 - Flags: approval1.9.1? → approval1.9.1+
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


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
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.

Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.