Today History folder should be expanded by default

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Bookmarks & History
P4
enhancement
RESOLVED FIXED
16 years ago
4 years ago

People

(Reporter: Matthew Paul Thomas, Assigned: Xin)

Tracking

Trunk
Firefox 22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=mak][lang=js])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

16 years ago
Build: 2001052115, Mac OS 9.1

To reproduce:
*   Open the History window.
*   Look at the `Today' folder.

What you should see:
*   The folder should be expanded.

What you actually see:
*   The folder is collapsed.

Comment 1

16 years ago
this probably wouldn't be too hard, but it might require an entry in the
profile's localstore.rdf

Updated

16 years ago
Status: NEW → ASSIGNED
Keywords: helpwanted
Priority: -- → P3
Target Milestone: --- → Future

Comment 2

16 years ago
reassigning history bugs to new owner - send this bug back to me if it looks
like something I should fix (such as embedding-related architecture issues),
rather than the actual history owner...
Assignee: alecf → blakeross
Status: ASSIGNED → NEW
Target Milestone: Future → ---

Updated

16 years ago
Target Milestone: --- → mozilla1.0.1

Updated

16 years ago
Target Milestone: mozilla1.0.1 → Future

Updated

15 years ago
Severity: normal → enhancement
OS: Mac System 9.x → All
Priority: P3 → --
Hardware: Macintosh → All
Summary: `Today' folder should be expanded by default → Today History folder should be expanded by default
Assignee: bross2 → nobody
QA Contact: claudius → history.global
It looks like this was fixed in SeaMonkey, but it not in Firefox, so moving there.
Component: History: Global → Bookmarks & History
Keywords: helpwanted
Product: Core → Firefox
QA Contact: history.global → bookmarks
Target Milestone: Future → ---
actually i think this would make more sense now that history is splitted by default also in the Library.
When you're in History menu and you choose "Show me all History", you are still 1 click away from your visits (indeed you have to click/double-click Today to see visits).
Since History menu shows last 10 visits, Show me all History should go into Today folder, showing all the other visits for today.

Updated

8 years ago
Priority: -- → P4
Target Milestone: --- → Future
Keywords: uiwanted
Agreed! Today should be expanded (selected) by default when opening History.
Keywords: uiwanted
Doing this in the sidebar and library window should be relatively straightforward, I hope?

Marco, can you mentor this one, assuming it is a good first bug? :)
maybe not exactly trivial, but should be feasible without big headaches.
Whiteboard: [mentor=mak][lang=js]
(Assignee)

Comment 8

4 years ago
Created attachment 724312 [details] [diff] [review]
expand Today History folder and sidebar by default

Submitting a patch.

Today History in Library and sidebar will be expanded.  If there is no Today History, next available history item will be expanded, yesterday for example.  If Clear Recent History is done, then nothing will be expanded.
(Assignee)

Comment 9

4 years ago
The patch will expand the first item of Bookmark in Library if show bookmark is selected.  I will attach a modified patch later.
(Assignee)

Comment 10

4 years ago
Created attachment 724587 [details] [diff] [review]
expand Today History folder and sidebar by default

Fixed expanding Bookmarks Toolbar mentioned in Comment 9
Attachment #724312 - Attachment is obsolete: true
Attachment #724587 - Flags: review?(mak77)
Assignee: nobody → magicxinzhang
Status: NEW → ASSIGNED
Target Milestone: Future → ---
Comment on attachment 724587 [details] [diff] [review]
expand Today History folder and sidebar by default

Review of attachment 724587 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure I like the idea to open Today in the sidebar, in the Library makes more sense since I arrive there selecting "show all history" and I expect to have history in the right pane.
The sidebar behavior is different and may be grouped differently, that means it's also easier to cause regressions there... I'd leave it alone for now.

::: browser/components/places/content/places.js
@@ +46,5 @@
> +                            .QueryInterface(Ci.nsINavHistoryContainerResultNode);
> +      var todayNode = historyNode.getChild(0);
> +      if (todayNode)
> +        this._places.selectNode(todayNode);
> +    }

selectLeftPaneQuery has already some special code to forcefully open AllBookmarks, I think you may just move your code there
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#28
That should simplify your code.

you should also open the History container, in your case it may already be open cause we store opened containers in localStore.rdf, but in the most common case it wouldn't be open so you should open it.
Attachment #724587 - Flags: review?(mak77)
Oh, before I forgot, many thanks and congratulations for your first contribution!
(Assignee)

Comment 13

4 years ago
Created attachment 725691 [details] [diff] [review]
expand Today History folder by default

leave history sidebar as is
Attachment #724587 - Attachment is obsolete: true
Attachment #725691 - Flags: review?(mak77)
Comment on attachment 725691 [details] [diff] [review]
expand Today History folder by default

Review of attachment 725691 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks fine, now we must ensure the tests are still passing properly, I don't remember if any of the tests is checking the Library.
You can run the tests with "./mach mochitest-browser browser/components/places/tests/" command.
If you have tryserver access you can push to it to get a tryserver run and check tests remotely, or you may ask me to do that for you if you don't yet have access.


Finally, please update the commit message to "Bug 82301 - Show all history should expand Today history folder by default. r=mak"

r=me with a green tryserver run and the message fixed
Attachment #725691 - Flags: review?(mak77) → review+
(Assignee)

Comment 15

4 years ago
(In reply to Marco Bonardo [:mak] from comment #14)
> The patch looks fine, now we must ensure the tests are still passing
> properly, I don't remember if any of the tests is checking the Library.
> You can run the tests with "./mach mochitest-browser
> browser/components/places/tests/" command.
> If you have tryserver access you can push to it to get a tryserver run and
> check tests remotely, or you may ask me to do that for you if you don't yet
> have access.

I ran the test on my local machine and unfortunately the test failed. I went through test code roughly and found that modified code this._places.selectNode(historyNode.getChild(0) will change selectedNode which breaks the test.
In tests/browser/browser_library_infoBox.js, selectedNode(now first child of history node) is used and still treated as history node.
(Assignee)

Comment 16

4 years ago
Several test cases failed. What can I do next? Any suggestions?
In this case, we usually fix the tests accordingly to the UI change, I suggest doing those changes in a part 2 patch.

for the browser_library_infoBox.js case I think we may want to change the patch slightly though, keep the containerOpen = true part in selectLeftPaneQuery (as it is doing for AllBookmarks) but move the selectNode code to init(), since calling selectLeftPaneQuery otherwise causes us to select something else, that is a bit unexpected from a caller point of view.
(Assignee)

Comment 18

4 years ago
Created attachment 729383 [details] [diff] [review]
expand Today History folder by default

This new patch passed all tests.
Attachment #725691 - Attachment is obsolete: true
Attachment #729383 - Flags: review?(mak77)
Comment on attachment 729383 [details] [diff] [review]
expand Today History folder by default

Review of attachment 729383 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you very much, looks ok, just a small cleanup you may still do.

When you have a new patch, please set checkin-needed keyword asking for it to be pushed to try to verify all tests are fine.
Then it can be pushed to inbound.
I will be away until next week, but I'm pretty sure our tree sheriffs will be kind enough to help :)

::: browser/components/places/content/places.js
@@ +29,5 @@
>      if (aQueryName == "AllBookmarks")
>        PlacesUtils.asContainer(this._places.selectedNode).containerOpen = true;
> +    else if (aQueryName == "History") {
> +      let historyNode = this._places.selectedNode;
> +      PlacesUtils.asContainer(historyNode).containerOpen = true;

now you can merge the if condition into the previous if like 
if (aQueryName == "AllBookmarks" || aQueryName == "History")
and just reuse the existing code since it's doing the same
Attachment #729383 - Flags: review?(mak77) → review+
(Assignee)

Comment 20

4 years ago
Created attachment 729907 [details] [diff] [review]
expand Today History folder by default

Merged the if condition to previous if
Attachment #729383 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f63ce344beca

Thanks for fixing this, Xin! \o/
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f63ce344beca
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Thank you and congratulations on fixing a near 12-year old bug!
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130408 Firefox/22.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20130408 Firefox/22.0

Verified as fixed on Firefox 21 beta 1 (buildID: 20130401192816) and latest Aurora (buildID: 20130408004015). Opening History/Show All History expands Today's history.
You need to log in before you can comment on or make changes to this bug.