359349 - when "view | by last visited" in history sidebar, don't show "sessions" by default

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

[rfe] when "view | by last visited" in history sidebar, show "sessions"

we actually get this RFE "for free" when we switch to using places.

this is just a heads up, to make sure that we indeed want this feature.  I'll attach a screen shot.

additionally, it's not just when we do "view | by last visited".  you'd get sessions when ever the results are visits, and we're sorted by date, and we're either not grouping, or we're grouping by date (once bug #358784 lands, that is.)

for that logic, see http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistoryResult.cpp#3852

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: screen shot of history sidebar showing "sessions"

Comment 2

[(not reading, please use [e-mail] instead)]

something else to keep in mind about sessions is that it will lead to duplicate entries when doing "View | By Last Visited".  (see the screen shot, see how "notes and links" shows up multiple times.)

this is different than Fx 2.x, so if we really want Fx style history sidebar, we should disable it.

beltzner / alex, any comments?

Comment 3

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

Overall I like the idea of detecting browsing sessions.

The current screen shot is not really self describing.  I showed it to someone and without them knowing that the topic of discussion was sessions, they were unable to explain what certain items were grouped together.

One way to make the interface self describing would be to label each session with an icon and its time range.  I've added a screen shot to show what I mean.

-------------------------

Usually time is represented using X instead of Y, but we probably want to keep the history UI consistent with earlier versions.

If you want to geek out over time based information visualization, this project is pretty cool: http://alumni.media.mit.edu/~fviegas/projects/mountain/

Here is the same general idea applied to an open source development: http://smg.media.mit.edu/projects/OpenSources/

We have all the data we need to apply this kind of view using domains (instead of email authors/developers).  Maybe a labs project?

Comment 4

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

Attachment: History with labeled sessions

Comment 5

[Brett Wilson]

Not being able to describe sessions is not an impediment to being useful. I haven't met anybody that could actually describe exactly what the line meant with no help, but many people find it useful. It tends to group things together that go together, which ends up breaking up the vertical monotony and makes it easier to find things.

In the places view with much more horizontal space, the time field shows the time at the start of each session. I think the "Labeled sessions" image is misleading because it assumes that sessions are separated in time. This is usually not the case, and is almost certainly not the case in the screen shot. 

Having fewer times in the right column in the full places view makes it easier to find times because there are fewer of them (see http://wiki.mozilla.org/Browser_History ). But separating by time vertically as in the example attachment needs to key off of something different than sessions, which is based on link structure.

I wrote a demo of vertical time-based grouping here:
browser/components/places/content/demos/time.js
which you can view with your actual history (it uses the last 100 pages, or something).

It is included in places builds if you type the proper chrome URL. The jar file that includes it is here:
http://lxr.mozilla.org/seamonkey/source/browser/components/places/jar.mn
I forget exactly how to convert theses paths into chrome: URLs.

Comment 6

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

>I think the "Labeled sessions" image is misleading because
>it assumes that sessions are separated in time.

Fair enough, that's because I assumed sessions were separated in time, based off of user activity.  They are based on link structure?

Sorry, I should have figured out how the feature actually works before working on the UI.  I am reading up on it now.

Comment 7

[(not reading, please use [e-mail] instead)]

since we are (initially) going for UI parity with Fx 2.0, and since we don't have the date column in the Fx 2.0 style sidebar, I'd like to add a pref to control drawing the session lines in the history sidebar, and disable it for now.

I believe all I need to do is bail out early from nsNavHistoryResultTreeViewer::ComputeShowSessions(), so that mShowSessions is always PR_FALSE.

Comment 8

[(not reading, please use [e-mail] instead)]

note, that approach would affect history in all UI, not just the history sidebar.  (but keep in mind, that's the onl history UI we have so far.)

so another approach would be to add this to nsNavHistoryQueryOptions, following the instructions at http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistoryQuery.h#156 for how to add more options.

then, in the place: uri for "View | By Last Visited", I'd add the (serialized) option to not show do sessions (based on the pref value)

Comment 9

[(not reading, please use [e-mail] instead)]

re-assign to dan, so that we can get this done for 1.9a1.

Comment 10

[Dan Mills [:thunder]]

Attachment: Add the ability to turn on/off the session separators.

To enable, add a hidden (bool) pref:

browser.history.showSessions = true

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 246875 [details] [diff] [review]
Add the ability to turn on/off the session separators.

dan, if browser.history.showSessions  is not defined, won't getBoolPref() throw an exception?

if I'm right, you'll want to wrap the added lines in history-panel.js with a try / catch.

Comment 12

[(not reading, please use [e-mail] instead)]

alternatively, you could define a default for browser.history.showSessions in mozilla/browser/app/profile/firefox.js.

I think I prefer the try / catch.  gavin, what do you prefer for code in mozilla/browser?

Comment 13

[Dan Mills [:thunder]]

Attachment: New version of the patch, with a try/catch

You're right, behavior-wise it was ok, but the error console showed an uncaught exception.  Fixed in new patch.

Comment 14

[Dan Mills [:thunder]]

Regarding my last comment--

Seth Spitzer and I spent a little while today figuring out why things appeared to work even when an exception was being thrown.

The short answer is that things *don't* actually work properly.

The longer answer is that when the place uri of the history tree in the sidebar is null (which is what ends up happening when the exception is thrown), an empty query object is created which results in a default SQL query, which happens to be very similar to my selected view during my testing.

Hope I didn't bore anyone to death there, it's good for me to document this :)

Comment 15

[(not reading, please use [e-mail] instead)]

Comment on attachment 246967 [details] [diff] [review]
New version of the patch, with a try/catch

r=sspitzer, thanks for taking care of the pref with a try / catch, and for debugging to see why the sidebar appeared to work even when we never set place attribute on the tree.

I'd recommend getting an additional review from gavin, as he is able to review toolkit and browser.

Comment 16

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

Comment on attachment 246967 [details] [diff] [review]
New version of the patch, with a try/catch

I think I would prefer a default pref, to avoid the try/catch, and so that this is easily togglable from about:config.

Comment 17

[(not reading, please use [e-mail] instead)]

dan, per gavin's request, can you:

1) remove the try / catch
2) to mozilla/browser/app/profile/firefox.js, add:

+pref("browser.history.grouping", false);

(sorry for misleading you towards using try / catch in comment #11)

Comment 18

[Dan Mills [:thunder]]

No problem.  I agree it is nicer to have the default in about:config too.

Comment 19

[Dan Mills [:thunder]]

Attachment: New new version of the patch, without the try/catch, with a default.

Final (?) version of the patch.  Seth, I assume you meant browser.history.showSessions, right?

Comment 20

[(not reading, please use [e-mail] instead)]

tested dan's fix on the trunk (on windows), and everything looks good.

landed it:

Checking in toolkit/components/places/public/nsINavHistoryService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v  <-
-  nsINavHistoryService.idl
new revision: 1.49; previous revision: 1.48
done
Checking in toolkit/components/places/src/nsNavHistoryQuery.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp,v  <--  nsN
avHistoryQuery.cpp
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/components/places/src/nsNavHistoryQuery.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.h,v  <--  nsNav
HistoryQuery.h
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  ns
NavHistoryResult.cpp
new revision: 1.79; previous revision: 1.78
done
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.167; previous revision: 1.166
done
Checking in browser/components/places/content/history-panel.js;
/cvsroot/mozilla/browser/components/places/content/history-panel.js,v  <--  hist
ory-panel.js
new revision: 1.3; previous revision: 1.2
done

about adding sessions as an RFE, the bug should remain open, or we should log a new bug and morph this one.

Comment 21

[(not reading, please use [e-mail] instead)]

morphing this bug to cover what dan checked in.

will log a new RFE bug about adding sessions to the history sidebar UI.