82301 - Today History folder should be expanded by default

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P4)

Description

[Matthew T (active 1999-2002)]

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

[Alec Flett]

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

Comment 2

[Alec Flett]

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

Comment 3

[:Gavin Sharp [email: [e-mail]]]

It looks like this was fixed in SeaMonkey, but it not in Firefox, so moving there.

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Alex Limi (:limi) — Firefox UX Team]

Agreed! Today should be expanded (selected) by default when opening History.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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? :)

Comment 7

[Marco Bonardo [:mak]]

maybe not exactly trivial, but should be feasible without big headaches.

Comment 8

[Xin]

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

Comment 9

[Xin]

The patch will expand the first item of Bookmark in Library if show bookmark is selected.  I will attach a modified patch later.

Comment 10

[Xin]

Attachment: expand Today History folder and sidebar by default

Fixed expanding Bookmarks Toolbar mentioned in Comment 9

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Marco Bonardo [:mak]]

Oh, before I forgot, many thanks and congratulations for your first contribution!

Comment 13

[Xin]

Attachment: expand Today History folder by default

leave history sidebar as is

Comment 14

[Marco Bonardo [:mak]]

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

Comment 15

[Xin]

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

Comment 16

[Xin]

Several test cases failed. What can I do next? Any suggestions?

Comment 17

[Marco Bonardo [:mak]]

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.

Comment 18

[Xin]

Attachment: expand Today History folder by default

This new patch passed all tests.

Comment 19

[Marco Bonardo [:mak]]

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

Comment 20

[Xin]

Attachment: expand Today History folder by default

Merged the if condition to previous if

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f63ce344beca

Thanks for fixing this, Xin! \o/

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f63ce344beca

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Thank you and congratulations on fixing a near 12-year old bug!

Comment 24

[Bogdan Maris, Desktop QA]

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.