If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 3 alpha1

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: thunder)

Tracking

Trunk
Firefox 3 alpha1
x86
All
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

[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
Created attachment 244543 [details]
screen shot of history sidebar showing "sessions"
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?
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?
Created attachment 244629 [details]
History with labeled sessions

Comment 5

11 years ago
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.
>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.
Blocks: 355738
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.
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)
Status: NEW → ASSIGNED
Severity: normal → enhancement
re-assign to dan, so that we can get this done for 1.9a1.

Assignee: sspitzer → thunder
Status: ASSIGNED → NEW
(Assignee)

Comment 10

11 years ago
Created attachment 246875 [details] [diff] [review]
Add the ability to turn on/off the session separators.

To enable, add a hidden (bool) pref:

browser.history.showSessions = true
Attachment #246875 - Flags: review?(sspitzer)
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.
Attachment #246875 - Flags: review?(sspitzer)
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?
(Assignee)

Comment 13

11 years ago
Created attachment 246967 [details] [diff] [review]
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.
Attachment #246875 - Attachment is obsolete: true
Attachment #246967 - Flags: review?(sspitzer)
(Assignee)

Comment 14

11 years ago
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 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.
Attachment #246967 - Flags: review?(sspitzer)
Attachment #246967 - Flags: review?(gavin.sharp)
Attachment #246967 - Flags: review+
Flags: in-testsuite?
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.
Attachment #246967 - Flags: review?(gavin.sharp) → review+
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)
Status: NEW → ASSIGNED
(Assignee)

Comment 18

11 years ago
No problem.  I agree it is nicer to have the default in about:config too.
(Assignee)

Comment 19

11 years ago
Created attachment 247902 [details] [diff] [review]
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?
Attachment #246967 - Attachment is obsolete: true
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.
morphing this bug to cover what dan checked in.

will log a new RFE bug about adding sessions to the history sidebar UI.
Severity: enhancement → normal
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Summary: [rfe] when "view | by last visited" in history sidebar, show "sessions" → when "view | by last visited" in history sidebar, don't show "sessions" by default
Target Milestone: --- → Firefox 3 alpha1

Updated

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