Closed Bug 589521 Opened 14 years ago Closed 14 years ago

[PATCH] Remove extra separators between "Show History" and "Clear History" menu items when history is empty

Categories

(Camino Graveyard :: Toolbars & Menus, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.2.8pre) Gecko/20100712 Camino/2.1a1pre (like Firefox/3.6.8pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.2.8pre) Gecko/20100712 Camino/2.1a1pre (like Firefox/3.6.8pre)

This patch does three things:

1. Currently, when the History menu had no history items, the menu has two separators in a row between the "Show History" and "Clear History" menu items. This patch ensures the extra separator is not added. Safari does this, too.

2. Remove some unused code from HistoryMenu.h and .mm

3. Refactor the HistorySubmenu and TopLevelHistoryMenu classes to simplify rebuildHistoryMenu() and remove some intimate code commingling.


Reproducible: Always
Attached patch refactor-history-menu-v1.patch (obsolete) — Splinter Review
Attachment #468111 - Flags: review?(stuart.morgan+bugzilla)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 468111 [details] [diff] [review]
refactor-history-menu-v1.patch

>+  return [historyItem respondsToSelector:@selector(isTodayCategory)]
>+          && [(HistoryDateCategoryItem*)historyItem isTodayCategory];

Our style is to put operators like && at the end of the previous line.

>+  BOOL isThisTodayCategory = isTodayCategory(mRootItem);

You've added knowledge of the Today category handling, which is specific to the top-level menu, to the generic history menu class. Only TopLevelHistoryMenu should know about this, so this is increasing coupling between the two classes, not decreasing it.

>-  HistoryItem* curChild;
>-  while (((curChild = [childEnum nextObject])) && remainingEntriesToShow > 0) {
>-    NSMenuItem* newItem = nil;
>+  for (;;) {
>+    HistoryItem* curChild = [childEnum nextObject];
>+    if (!curChild)
>+      break; // no more history items

Please don't use a completely different idiom for enumeration than the whole rest of our codebase. This applies throughout the patch.

>-    if ([curChild isKindOfClass:[HistorySiteItem class]]) {
...
>+    NS_ASSERTION([curChild isKindOfClass:[HistorySiteItem class]], "not a HistorySiteItem?!");

Why have you changed from a check to an assertion? This isn't an unrecoverable error--and if it did happen in a Release bulid this form is actually less safe, because it will then run code assuming it's the wrong class.

Also, there's no Core code in this file, so NS_ASSERTION shouldn't be used.

(This also applies throughout the patch.)

>-- (BOOL)validateMenuItem:(NSMenuItem*)aMenuItem

Why did you remove this method?

>+  if (isTodayCategory(inItem))
>   {

|{| should be on the same line for a single-line condition.

>+  // now iterate through the history categories

Comments should be full sentences with caps and a period. This comment is a misleading though, since you don't actually iterate until much later in the method.

>+  NSEnumerator* childEnum = [[mRootItem children] objectEnumerator];

Needs a more descriptive title (e.g., categoryEnum).

>+  if ([todayCategory numberOfChildren] > (int) kMaxTodayItems) {

Cast numberOfChilden to unsigned int, not the max to int. Also, no space between a cast and the thing being casted.

>+      if (isTodayCategory(historyItem))
>+      {

Same line.
Attachment #468111 - Flags: review?(stuart.morgan+bugzilla) → review-
Thanks for your constructive feedback, Stuart. Here is patch v2. It is _much_ simpler and only includes these changes:

1. HistoryMenu.h: Remove unused mAdditionalItemsParent variable from HistorySubmenu class.

2. HistoryMenu.h Fix a comment typo misspelling "heirarchy".

3. HistoryMenu.mm: Fix TopLevelHistoryMenu's addLastItems() function to only check *non-empty* History categories when determining whether an extra separatorItem is needed before the "Clear History" menu item. The previous check would always return true because the number of History categories (empty or not) was always 8.

4. HistoryMenu.mm: Fix a old comment's reference to "Go" menu with "History" menu.
Attachment #468111 - Attachment is obsolete: true
Attachment #468939 - Flags: review?(stuart.morgan+bugzilla)
Comment on attachment 468939 [details] [diff] [review]
Incorporate Stuart's feedback into a much simpler patch.

r=smorgan, thanks!
Attachment #468939 - Flags: superreview?(mikepinkerton)
Attachment #468939 - Flags: review?(stuart.morgan+bugzilla)
Attachment #468939 - Flags: review+
+  NSEnumerator* categoryEnum = [[mRootItem children] objectEnumerator];

Can we use fast enumeration here, or is that 10.5+ only?

sr=pink
Attachment #468939 - Flags: superreview?(mikepinkerton) → superreview+
This web page states the fast enumeration is officially 10.5+ only, but can be made to work on 10.4 for non-Cocoa user-defined classes. Sounds risky to me.

http://cocoawithlove.com/2008/05/fast-enumeration-clarifications.html
checkin-needed, please.
Keywords: checkin-needed
http://hg.mozilla.org/camino/rev/6042d960288c

Thanks so much for fixing this, Chris!  (I hope there are some other bugs we can interest you in fixing, too ;) )
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: