311106 - History creates extra folders it doesn't need

Status: RESOLVED FIXED

(Camino Graveyard :: History, defect, P3)

Description

[Chris Lawson (gone)]

With the new ("new" being a relative term, as this has been in effect for at
least a few months now) Go menu, we create unnecessary folders when the user's
History is set to less than seven (7) days.

For example, if History is set to three (3) days, we still create folders in the
Go menu for four, five, six, and seven days ago. We should be smart enough to
create only the folders we'll need, and not the folders we don't.

Comment 1

[Chris Lawson (gone)]

I should probably mention that these folders are being created in the History
itself, rather than only in the Go menu (which, IIRC, is building from History
anyway).

cl

Comment 2

[Ulrik Sverdrup]

I am working on this one

Comment 3

[Simon Fraser [no longer active]]

(In reply to comment #0)
> With the new ("new" being a relative term, as this has been in effect for at
> least a few months now) Go menu, we create unnecessary folders when the user's
> History is set to less than seven (7) days.
> 
> For example, if History is set to three (3) days, we still create folders in the
> Go menu for four, five, six, and seven days ago. We should be smart enough to
> create only the folders we'll need, and not the folders we don't.

Agreed, but I still think we should make folders for "empty" days within the
history limit.

Comment 4

[Ulrik Sverdrup]

Attachment: Patch to only create as many folders as needed

A patch that remodels HistoryBySiteTreeBuilder's categoryItemForDate: to create
items that are needed.

Changes done:
Put the HistoryCategoryItems in a dictionary as it's probably much more
efficient
Put off creation to category items til when they were asked for 
Put a method we needed in NSDate+Utils 

Changes not done:
Does not create empty folders for the days that are skipped
I will probably fix this and post a new patch, but posting this here because I
tested the patch in this form extensively. I also prefer not to have empty
folders, even if they are in the sensible range.

Comment 5

[Chris Lawson (gone)]

(In reply to comment #3)
> Agreed, but I still think we should make folders for "empty" days within the
> history limit.

So in other words, if I have my history set to five days, and I browse a bunch
today but then I go on vacation for the next two days, I should have (after
doing a little browsing when I return from vacation on Friday) the following:

Earlier Today
Yesterday (which would be empty, since I was on vacation)
Wednesday, October 05 (also empty)
Tuesday, October 04 (with all the browsing I did today)
Monday, October 03 (with that day's browsing in it)

I should no longer have last Sunday or Saturday, or the "More than a week ago"
folder, since all those are outside my history limit of five days.

Is that basically what you're saying, Simon? If so, I agree that we should
create the folders, but we should avoid populating them with anything so as to
avoid the submenu widget in the menu item. (I haven't actually tried setting my
history this low, so I don't know if empty history folders automatically lose
the submenu triangle or not.)

Also, assigning the bug to Ulrik.

cl

Comment 6

[Simon Fraser [no longer active]]

Comment on attachment 198534 [details] [diff] [review]
Patch to only create as many folders as needed

>Index: extensions/NSDate+Utils.h
>===================================================================

>+- (int)daysSinceNow;

I think this needs renaming to clarify it a little, since it's really
"days before now". Perhaps "ageInDays"?

>Index: extensions/NSDate+Utils.m
>===================================================================

>+- (int)daysSinceNow
>+{
>+  NSCalendarDate* nowDate = [NSCalendarDate calendarDate];
>+  static const NSTimeInterval dayLength = 86400.0; // in seconds
>+  NSCalendarDate* lastMidnight = [[NSCalendarDate alloc] initWithYear:[nowDate yearOfCommonEra]
>+                                                    month:[nowDate monthOfYear]
>+                                                      day:[nowDate dayOfMonth]
>+                                                     hour:0
>+                                                   minute:0
>+                                                   second:0
>+                                                 timeZone:[nowDate timeZone]];
>+  int daysAgo = (int)(1 + [lastMidnight timeIntervalSinceDate:self]/dayLength);

This takes some thought to figure out. I wonder if there's a simpler way.

>Index: history/HistoryDataSource.mm
>===================================================================

> @interface HistoryByDateTreeBuilder : HistoryTreeBuilder
> {
>-  NSMutableArray*         mDateCategories;        // array of HistoryCategoryItems ordered recent -> old
>+  NSMutableDictionary*         mDateCategories;        // Dictionary of date categories

As we discussed, I think a NSArray that it just used as a look-up
table is preferable to a dictionary (and avoids the ordering problems).

>+- (NSDate*)dateForDaysAgo:(int)daysAgo;   // For private use

No need for comment (it's in the private category).


>+- (NSDate*)dateForDaysAgo:(int)daysAgo
...
>+  NSCalendarDate* resultDate = [nowDate dateByAddingYears:0
>+                                                     days:(-1)*daysAgo
>+                                                    hours:(-1)*[nowDate hourOfDay]
>+                                                  minutes:(-1)*[nowDate minuteOfHour]
>+                                                  seconds:(-1)*[nowDate secondOfMinute]];                                      
>+  return resultDate;

I don't really like all those subtractions. If NSDate keeps track of time
intervals with a resolution of < 1 second, then the resulting date will
still have the fractional seconds. I'd prefer to make a new date with 
-initWithYear:month:... etc. and then use -dateByAddingYears:0 months:0
days:-1...

>+  static const int kOlderThanAWeek = 7;
>+  int daysAgo = [date daysSinceNow];  // this is a method in our ChimeraDateUtils category
>+  if(daysAgo >= kOlderThanAWeek)

if<space>( please.

>+  // Try to find the date category in the dictionary
>+  id categoryKey = [NSNumber numberWithInt:daysAgo];

Please strongly type when you can, so id -> NSNumber.


>+  [mRootItem addChildren:[mDateCategories allValues]];

This is the main problem: -allValues doesn't guarantee ordering.

Comment 7

[Ulrik Sverdrup]

Attachment: Patch to create only those folders that are needed v2

This patch addresses several concerns but leave some untouched.
-The code uses an Array for lookup of the categories, using indexes from 0 to
(at most) 7.
-Only creates folders until the oldest bookmark, but creates empty folders
inside that range. (So it does not check for the history cutoff)

The - (int)daysSinceNow method.
Is still named like that, but now returns negative if the reciever is earlier
than Now.
Why: It is modeled in name and return value after the already existing 
- (NSTimeInterval)timeIntervalSinceNow
"Returns the interval between the receiver and the current date and time. If
the receiver is earlier than the current date and time, the return value is
negative."

-Also, changed the implementation of dateForDaysAgo: as you suggested

Comment 8

[Ulrik Sverdrup]

Attachment: Patch to create only those folders that are needed v3

Obviously made a mistake in the last attachment; forgot one of the two files!

This is the patch I'm talking about in the previous comment.

Comment 9

[Simon Fraser [no longer active]]

Comment on attachment 198664 [details] [diff] [review]
Patch to create only those folders that are needed v3

I think we should just make the date category items up-front as we did before.
The saving from doing it lazily are minute, and it makes the logic more
complex.

Comment 10

[Ulrik Sverdrup]

Okay, I'll get there.

Is it okay to use a snippet like this:

BOOL gotPref;
int expireDays = [[PreferenceManager sharedInstance]
getIntPref:"browser.history_expire_days" withSuccess:&gotPref];
  if (!gotPref)
    expireDays = 9; // or anything

Comment 11

[Simon Fraser [no longer active]]

Yep, that looks right.

Comment 12

[Ulrik Sverdrup]

Attachment: Create only folders up to the history cutoff

This patch reads the preference for "Remember visited pages for X days"

Creates one more folder than the preference says, since 1 day means the last 24
hours, and most often this spans both our Today and Yesterday history folders
(for example).

This patch reorganises the setupDateCategories method, but touches nothing
else. The method now uses a loop, as that is the most logical way to exit when
we created enough folders.

This patch depends on bug 311793

Comment 13

[Simon Fraser [no longer active]]

Fixed on trunk and branch. Thanks!

Comment 14

[Simon Fraser [no longer active]]

I also fixed nsSimpleGlobalHistory to expire old entries on the expire_days pref
changed callback, so that as you change the pref in the prefs window, the
History view (and menu) will update appropriately.

Comment 15

[Ulrik Sverdrup]

Great, thanks for your work on this.

Thank you for the code review I got, too. I'm always learning something.