Last Comment Bug 82301 - Today History folder should be expanded by default
: Today History folder should be expanded by default
Status: RESOLVED FIXED
[mentor=mak][lang=js]
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: P4 enhancement with 2 votes (vote)
: Firefox 22
Assigned To: Xin
:
: Marco Bonardo [::mak]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-05-22 22:32 PDT by Matthew Paul Thomas
Modified: 2013-04-08 09:31 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
expand Today History folder and sidebar by default (2.05 KB, patch)
2013-03-13 01:24 PDT, Xin
no flags Details | Diff | Splinter Review
expand Today History folder and sidebar by default (2.13 KB, patch)
2013-03-13 13:39 PDT, Xin
no flags Details | Diff | Splinter Review
expand Today History folder by default (1.15 KB, patch)
2013-03-15 18:21 PDT, Xin
mak77: review+
Details | Diff | Splinter Review
expand Today History folder by default (1.75 KB, patch)
2013-03-25 19:50 PDT, Xin
mak77: review+
Details | Diff | Splinter Review
expand Today History folder by default (1.82 KB, patch)
2013-03-26 18:07 PDT, Xin
no flags Details | Diff | Splinter Review

Description Matthew Paul Thomas 2001-05-22 22:32:19 PDT
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 2001-05-23 10:56:44 PDT
this probably wouldn't be too hard, but it might require an entry in the
profile's localstore.rdf
Comment 2 Alec Flett 2001-09-17 09:15:34 PDT
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: gavin@gavinsharp.com] 2009-06-02 09:22:48 PDT
It looks like this was fixed in SeaMonkey, but it not in Firefox, so moving there.
Comment 4 Marco Bonardo [::mak] 2009-06-22 13:23:12 PDT
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 2012-02-03 16:23:14 PST
Agreed! Today should be expanded (selected) by default when opening History.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-27 15:48:57 PST
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] 2013-02-27 15:51:09 PST
maybe not exactly trivial, but should be feasible without big headaches.
Comment 8 Xin 2013-03-13 01:24:45 PDT
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.
Comment 9 Xin 2013-03-13 02:21:18 PDT
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 2013-03-13 13:39:41 PDT
Created attachment 724587 [details] [diff] [review]
expand Today History folder and sidebar by default

Fixed expanding Bookmarks Toolbar mentioned in Comment 9
Comment 11 Marco Bonardo [::mak] 2013-03-14 06:33:37 PDT
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] 2013-03-14 06:34:28 PDT
Oh, before I forgot, many thanks and congratulations for your first contribution!
Comment 13 Xin 2013-03-15 18:21:09 PDT
Created attachment 725691 [details] [diff] [review]
expand Today History folder by default

leave history sidebar as is
Comment 14 Marco Bonardo [::mak] 2013-03-22 07:50:09 PDT
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 2013-03-24 16:26:37 PDT
(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 2013-03-24 21:17:43 PDT
Several test cases failed. What can I do next? Any suggestions?
Comment 17 Marco Bonardo [::mak] 2013-03-25 05:54:59 PDT
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 2013-03-25 19:50:25 PDT
Created attachment 729383 [details] [diff] [review]
expand Today History folder by default

This new patch passed all tests.
Comment 19 Marco Bonardo [::mak] 2013-03-26 11:24:44 PDT
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 2013-03-26 18:07:55 PDT
Created attachment 729907 [details] [diff] [review]
expand Today History folder by default

Merged the if condition to previous if
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-03-27 08:36:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f63ce344beca

Thanks for fixing this, Xin! \o/
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-03-27 14:12:30 PDT
https://hg.mozilla.org/mozilla-central/rev/f63ce344beca
Comment 23 Jared Wein [:jaws] (please needinfo? me) 2013-03-27 14:17:34 PDT
Thank you and congratulations on fixing a near 12-year old bug!
Comment 24 Bogdan Maris, QA [:bogdan_maris] 2013-04-08 09:31:14 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.