623774 - Don't show empty History folders after the last non-empty folder in History view/manager

Status: RESOLVED FIXED

(Camino Graveyard :: History, defect)

Description

[Markus Amalthea Magnuson]

Attachment: Empty History folders from a fresh profile

When there is no History for the time periods defined by the History folders, they show up empty. See attached screenshot from a fresh profile without any history at all. I think a better experience would be to not show any such empty folders at all.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Per bug 311106 comment 3, this is by design (and I think I'd still rather see a consistent collection of folders than a random, possibly non-linear set of folders, depending on which days have history).  I'll let others weigh in, but I believe this should be WONTFIX.

Comment 2

[Chris Peterson [:cpeterson]]

My suggestion would be to show History folders for Today through the last non-empty History folder. That shows the history's date range without any gaps.

So a browser without any history would show an empty Today folder. The next day it would show Today and Yesterday folders.

Note that Camino's History menu behaves as Markus describes. The History menu has gaps if some days have no history items.

Comment 3

[Stuart Morgan]

I like that idea (comment 2); I don't see any down-side in trimming folders that predate the first piece of history.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #3)
> I like that idea (comment 2); I don't see any down-side in trimming folders
> that predate the first piece of history.

I'm OK with that, too, I guess.

Comment 5

[Chris Peterson [:cpeterson]]

Should the History menu's behavior be changed to match the proposed "Show History" window's behavior? Currently, the History menu omits all empty History folders, leaving date gaps.

I don't have a strong opinion, but adding empty folder menus sounds more confusing than date gaps. And wasteful of screen real estate.

Safari's History menu and "Show All History" page behave the same: they omit all empty History folders, leaving gaps.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I think the menu's current behavior is fine; I have less of a spatial relationship with that than with the nice view in the manager.

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: trim-empty-history-folders-v1.patch

trim-empty-history-folders-v1.patch implements my suggestion from comment #2.

Comment 8

[Stuart Morgan]

Comment on attachment 505357 [details] [diff] [review]
trim-empty-history-folders-v1.patch

While this would work, you could simplify the logic by having the loop go from (numDateCategories - 1) to 1 and do nothing but break on the first non-empty category, then using subarrayWithRange: with the final value of i to get the child array all at once.

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: trim-history-v2.patch

This patch simplifies the loop logic and removes an NSMutableArray ref-count bug.

To avoid making a new NSMutableArray copy, I modify HistoryByDateTreeBuilder's mDateCategories. I believe this is safe because buildTree rebuilds mDateCategories when the history changes. Also, I don't expect new history items to be dynamically added to old empty categories that have been trimmed from the current history view.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #9)
> Also, I don't expect new history
> items to be dynamically added to old empty categories that have been trimmed
> from the current history view.

What happens when the date rolls over while Camino is running (e.g., there's history for Today and Yesterday, the clock strikes 12, and Today's items move to Yesterday, and Yesterday's original items move to $DayBeforeYesterday, which now must be created), or is this something else entirely?

Comment 11

[Chris Peterson [:cpeterson]]

When the date rolls over, the history data is rebuilt and the view updates itself (including any new date categories necessary). I confirmed that my patch doesn't change this behavior.

Comment 12

[Stuart Morgan]

Comment on attachment 505714 [details] [diff] [review]
trim-history-v2.patch

Not what I had in mind, but this works too :) You just need to remove the comment at the end of categoryItemForDate:

r=smorgan with that change.

Comment 13

[Chris Peterson [:cpeterson]]

I was not able to use subarrayWithRange: because it would return a (non-mutable) NSArray, which was not really compatible with HistoryByDateTreeBuilder's other mDateCategories logic. I will implement the categoryItemForDate: change your requested.

Comment 14

[Chris Peterson [:cpeterson]]

The comment in categoryItemForDate: says "in theory we should never get here, because the last item has a date of 'distant past'". But AFAIK this comment is still correct.

My change removes date categories from HistoryByDateTreeBuilder's mDateCategories. I can't reproduce any cases where new history items (from browsing in another tab) can be added to an existing History view for which that History view does not already have a date category (even after my change trims old empty categories).

When the clock strikes midnight, the HistoryTimerProxy timer creates a *new* HistoryByDateTreeBuilder instance to replace yesterday's (trimmed) HistoryByDateTreeBuilder.

HistoryByDateTreeBuilder's setUpDateCategories has code to repopulate a new or existing mDateCategories, but setUpDateCategories is only called by initWithDataSource (during HistoryByDateTreeBuilder init). So AFAICT mDateCategories is never repopulated after init.

Comment 15

[Stuart Morgan]

(In reply to comment #13)
> I was not able to use subarrayWithRange: because it would return a
> (non-mutable) NSArray

There's always mutableCopy.

(In reply to comment #14)
> The comment in categoryItemForDate: says "in theory we should never get here,
> because the last item has a date of 'distant past'". But AFAIK this comment is
> still correct.

Your change removes items from mDateCategories, which means that it's no longer a given that the last item has a date of 'distant past'. The internal implementation comment of a single method shouldn't be conditional on the call graph leading to that method never changing--comments that fragile do more harm than good.

Comment 16

[Chris Peterson [:cpeterson]]

Attachment: trim-history-v3.patch

As per smorgan, I changed the comment in categoryItemForDate: to "Category not found. The category for this date must have already been trimmed."

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Chris, what's the expected result when you have

* Today (items)
* Yesterday (items)
* Sunday (0)
* Saturday (items)
* Friday (0)
* Thursday (0)
* Wednesday (0)
* More than a week ago (items)

and then you delete "More than a week ago"?

Should the now-empty "More than a week ago" and the empty days between it and Saturday hang around until you close History (or until the date rolls over, when comment 11 happens), or should they be trimmed right away?

Right now the patch does the former, and if that's the right behavior, I'll go ahead and land this (per smorgan, his r+ is now sr+).

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/361df9693fe3

Stuart said as-is is fine, and is probably less jarring for the user.

Comment 19

[Chris Peterson [:cpeterson]]

Sorry for the late response. If you decide the "trim right away" behavior is preferable, just let me know.