358784 - implement "View | By Date" and "View | By Date and Site" for places based history sidebar (GROUP_BY_DAY)

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

As far as I can tell, places does not already have a way to group results by date.

need GROUP_BY_DATE and GROUP_BY_DATE_AND_SITE for history sidebar, for View | By Date and View | By Date and Site (to achieve parity with Fx 2)

see http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/public/nsINavHistoryService.idl#934

Comment 1

[Brett Wilson]

This is correct. It used to be in there but I removed it in a big refactor when I broke it and realized we didn't need it. This should be easy to add to the grouper since other grouping is more complicated. Once you write a day grouper, you can magically do group by day the by host, for example.

Comment 2

[Brett Wilson]

I should clarify, don't write a GROUP_BY_DAY_AND_SITE flag. The grouping mode is an array of grouping flags. Specify [GROUP_BY_DAY, GROUP_BY_DOMAIN] to get this. HOST would be exactly what the old system uses, but DOMAIN is much more useful, modulo the incomplete TLD list which has to be finished anyway.

Comment 3

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

brettw, thanks for the infomation.

Perhaps for backwards compatibility, we should use GROUP_BY_HOST for Fx 2 style group "by date and site" and "by site", but add two more:  "by date and domain" and "by domain" to take advantage of the more useful GROUP_BY_DOMAIN.  Or is that just bloat?  I'll spin off a new bug about that, and cc you.

> modulo the incomplete TLD list which has to be finished anyway

are you referring to bug #319643?

Comment 4

[Brett Wilson]

I think the current dropdown already has too many things and I have to think for a surprisingly long time about what each one does before I select it. For example, what's the difference between "by date" and "by last visited" without trying it? I don't see how people are supposed to decide they want host or domain grouping. If anything, grouping modes should be removed, not added.

Comment 5

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

Attachment: patch for group by date, stiill a couple todo items

Comment 6

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

Attachment: screen shot of "View | By Date and Site", with folder icons and sorted properly

Comment 7

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

whoops, instead of this hackery:

+// XXX todo, add this for winstripe
+// XXX todo, what about open / closed state for winstripe (Fx 2.0 on pinstripe doesn't do it.)
+#define FOLDER_ICON_URI "chrome://browser/skin/places/folder.png"
+

what I want to do is add:

<?xml-stylesheet href="chrome://browser/skin/places/places.css"?>

to history-panel.xul, which has the css to style containers, and fix spacing issues, etc.

new patch coming.

Comment 8

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

Attachment: updated patch (still a couple todo items)

Comment 9

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

Attachment: screen shot of "View | By Date and Site" using places.css instead of FAVICON for folder.

Comment 10

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

Attachment: screen shot from windows

Comment 11

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

it turns out, I don't need to pass around the time, because nsNavHistoryContainerNode::FillStats() will populate the container (ex: "Today") with the time of the node, if the node's time is > than the container time.

I'll remove that part of my patch, and then add a comment to nsNavHistoryContainerNode::FillStats() to explain that.

Comment 12

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

Attachment: updated patch

Comment 13

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

Attachment: add comment about bug #359332, and fix the "sessions" code

Comment 14

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

Attachment: add comment about bug #359346

Comment 15

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

note, once the fix for bug #356487 lands, the additional patch needed for this will be to remove the comments around "group" line of code in history-panel.js

+    else if (gHistoryGrouping == "dayandsite") {
+      /* placeURI += "&group=" + NHQO.GROUP_BY_DAY; */
+      placeURI += "&group=" + NHQO.GROUP_BY_HOST; 
+      placeURI += "&sort=" + NHQO.SORT_BY_TITLE_ASCENDING;
+    }
+    else {
+      /* placeURI += "&group=" + NHQO.GROUP_BY_DAY; */
+      placeURI += "&sort=" + NHQO.SORT_BY_TITLE_ASCENDING;
+    }

Comment 16

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

from another bug, brettw says:

I realized you should be doing this grouping differently than you are, as it
will give poor performance grouping every single item in history (though
possibly not worse than the Mork one).

You should have some root thing (probably this is hardcoded somewhere for now,
in the original design it would be a bookmark folder) that contains queries.
These queries are for "Today", "Yesterday", etc...

Querying by time is *very* fast. This prevents you from having to schlep *all*
of history into memory and sort it and group it. Only those folder that the
user has opened will be populated and kept up-to-date, which will happen on
demand. Initial population will be instant.

Comment 17

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

Comment on attachment 244544 [details] [diff] [review]
add comment about bug #359346

clearing r= request, due to brettw's comment.

Comment 18

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

note, the last patch really implements GROUP_BY_DAY, not GROUP_BY_DATE (although if we want to do IE 7 style grouping, or even generic date grouping, GROUP_BY_DAY won't be accurate).

additionally, see brettw's comment #16

Comment 19

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

Attachment: complete group by date patch for the trunk

this just the old patch (updated to the trunk), and is not brettw's suggestion.

Comment 20

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

brett also writes:

> Your group by date implementation is probably fine for now, since it gives
> parity. You can worry about performance later. Just realize that this really
> needs to be changed before shipping.

based on that comment, I'll seek reviews on my implementation, and log a spin off bug about his suggested optimization.

Comment 21

[Brett Wilson]

Can you please rewrite GetAgeInDays using "regular" math. I am pretty sure it is legal to use 64-bit math nowadays because all our compilers support it, so we don't have to have these difficult to read LL_ macros. Also, this code is very confusing (I actually rewrote it once, but I can't find it). There are errors in it that happen to cancel each other out to give the correct answer. The author was confused and thought that PRTime was in msecs when it is in usecs.

For example, LL_DIV(ageInDays, ageInSeconds, msecPerDay); but days != seconds / (msecs/day)!

In GetAgeInDays, I think the hashtable stuff is unnecessary. You first compute the age in days, clamp it to your range of 0-8, then convert it to a string which you look up in a hashtable. Also, NUM_DAYS should be a const int which is scoped and typed correctly, and will be optimized the same.

Then you have:
  const int numDays = 8;
  nsNavHistoryContainerResultNode* dates[numDays];
  for (int i = 0; i < numDays; i++)
    dates[i] = NULL;

Then you can do the same thing you always did with lazily creating the categories without all those string copies and hash computations.

Comment 22

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

thanks for the review, brett.  I'll work on a new patch and seek another review.

Comment 23

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

Attachment: updated patch, per brettw's comments

Comment 24

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

> Can you please rewrite GetAgeInDays using "regular" math.

the new version (in my last patch) is based on the simplified version of code from mozilla/xpfe/components/history/src/nsGlobalHistory.cpp

> In GetAgeInDays, I think the hashtable stuff is unnecessary.

I've re-written it per your suggestion.  My previous attempt was based on how GroupByHost() works, but with a fixed number of days, it was overkill.

Comment 25

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

Attachment: updated patch, per brettw's comments

Comment 26

[Brett Wilson]

Comment on attachment 244966 [details] [diff] [review]
updated patch, per brettw's comments

Looks good.

Comment 27

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

Comment on attachment 244966 [details] [diff] [review]
updated patch, per brettw's comments

seeking browser / toolkit peer review from gavin.

Comment 28

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

from irc, gavin spots two issues:

1) can't you just use PRInt64 types directly?

(following up on brettw's original comment, about removing the LL_ macros)

and,

2) int -> PRInt32 and attach a new patch.

Comment 29

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

Attachment: updated patch, per gavin's comments

still need LL_INIT(), but other LL_ removed, and GetAgeInDays() now returns PRInt64

Comment 30

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

to be specific, if I do:

static const PRInt64 USECS_PER_DAY = PR_USEC_PER_SEC * 60 * 60 * 24;

we'd get the following warning on windows:

c:/builds/trunk/mozilla/toolkit/components/places/src/nsNavHistory.cpp(3302) : w
arning C4307: '*' : integral constant overflow

this is why I'm using the LL_INIT() macro

Comment 31

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

Comment on attachment 245179 [details] [diff] [review]
updated patch, per gavin's comments

Just a few little nits, feel free to ignore them as you deem necessary.

>+  for (PRInt32 i = 0; i < numDays; i++)
>+    dates[i] = NULL;

s/NULL/nsnull/ ?

>+  GetAgeInDaysString(numDays-2, 
>+                    NS_LITERAL_STRING("finduri-AgeInDays-isgreater").get(),
>+                    dateNames[numDays-1]);

nit: fix indent?

>+    if (ageInDays >= (numDays - 1))
>+      ageInDays = numDays - 1;

Looks like that should be just > and not >=.

>@@ -693,16 +696,26 @@ PRInt32 PR_CALLBACK nsNavHistoryContaine
>   PRUint32 aType, bType;
>   a->GetType(&aType);
>   b->GetType(&bType);
> 
>   if (aType != bType) {
>     return bType - aType;
>   }
> 
>+  if (aType == nsINavHistoryResultNode::RESULT_TYPE_DAY) {
>+    // for the history sidebar, when we do "View | By Date" or 
>+    // "View | By Date and Site" we sort by SORT_BY_TITLE_ASCENDING, 
>+    // so to make the day container show up in
>+    // desire order, we need to compare by time, instead of title
>+    // hard coding this isn't ideal, but we can't currently have
>+    // one sort per grouping.  see bug #359332 on that issue.
>+    return -ComparePRTime(a->mTime, b->mTime);

This comment is a little hard to read. I think it needs a period after "title", and "desire" needs a "d".

Comment 32

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

> s/NULL/nsnull/ ?

fixed, thanks g#.

> nit: fix indent?

fixed, thanks g#.

> Looks like that should be just > and not >=.

good point, since if ageInDays == numDays - 1, then assigning ageInDays to numDays - 1 is a noop.  fixed, thanks g#.

> This comment is a little hard to read. I think it needs a period after "title", and "desire" needs a "d".

fixed the typos, and tried to make it a little more readable.  thanks again, g#.

I'll attach the final patch, and check in.

Comment 33

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

Attachment: updated patch, per g#'s last comment

carrying over r=g#,brettw

Comment 34

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

fixed.  thanks again gavin and brettw for the reviews and suggestions.

Checking in browser/components/places/content/history-panel.js;
/cvsroot/mozilla/browser/components/places/content/history-panel.js,v  <--  history-panel.js
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/components/places/public/nsINavHistoryService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v  <--  nsINavHistoryService.idl
new revision: 1.48; previous revision: 1.47
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.102; previous revision: 1.101
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.68; previous revision: 1.67
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.78; previous revision: 1.77
done
Checking in toolkit/components/places/src/nsNavHistoryResult.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.h,v  <--  nsNavHistoryResult.h
new revision: 1.27; previous revision: 1.26
done

Comment 35

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv