History creates extra folders it doesn't need

RESOLVED FIXED in Camino1.0

Status

P3
minor
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: bugzilla-graveyard, Assigned: englabenny)

Tracking

({fixed1.8})

unspecified
Camino1.0
PowerPC
macOS
fixed1.8

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
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
(Assignee)

Comment 2

14 years ago
I am working on this one

Comment 3

14 years ago
(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.
Assignee: mikepinkerton → sfraser_bugs
(Assignee)

Comment 4

14 years ago
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.
(Reporter)

Comment 5

14 years ago
(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
Assignee: sfraser_bugs → englabenny

Comment 6

14 years ago
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.
Attachment #198534 - Flags: review-
(Assignee)

Comment 7

14 years ago
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
Attachment #198534 - Attachment is obsolete: true
Attachment #198663 - Flags: review?(sfraser_bugs)
(Assignee)

Comment 8

14 years ago
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.
Attachment #198663 - Attachment is obsolete: true
Attachment #198664 - Flags: review?(sfraser_bugs)
(Assignee)

Updated

14 years ago
Attachment #198663 - Flags: review?(sfraser_bugs)

Comment 9

14 years ago
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.
(Assignee)

Comment 10

14 years ago
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

14 years ago
Yep, that looks right.
(Assignee)

Comment 12

14 years ago
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
Attachment #198664 - Attachment is obsolete: true
Attachment #199010 - Flags: review?(sfraser_bugs)
(Assignee)

Updated

14 years ago
Attachment #198664 - Flags: review?(sfraser_bugs)
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Depends on: 311793

Updated

14 years ago
Attachment #199010 - Flags: review?(sfraser_bugs) → review+

Comment 13

14 years ago
Fixed on trunk and branch. Thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed1.8
Priority: -- → P3
Resolution: --- → FIXED
Target Milestone: --- → Camino1.0

Comment 14

14 years ago
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.
(Assignee)

Comment 15

14 years ago
Great, thanks for your work on this.

Thank you for the code review I got, too. I'm always learning something.
You need to log in before you can comment on or make changes to this bug.