498941 - Partition off the icons in the location bar

Status: RESOLVED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, enhancement)

Description

[Stuart Morgan]

Dan did a really nice mockup for partitioning off the location bar right-side icons as part of the 1-click bookmark mockups:
http://wiki.caminobrowser.org/images/b/b5/Newlocation_bar.png
There was general agreement that it looks good (setting aside the contentious round-rect, and the star itself), so we should make it happen!

Comment 1

[Dan Weber]

Attachment: Patch 1

Comment 2

[Stuart Morgan]

Comment on attachment 387256 [details] [diff] [review]
Patch 1

Very pretty :)

First, some behavioral notes:
- There's a very noticeable lag before it starts to animate in, and before it vanishes when leaving. For example, on caminobrowser.org in a current build the blue search icon highlight and the RSS icon show up at the same time, but with this patch there's a pause before the RSS icon shows up.
- It feels odd that it slides in, but vanishes instead of sliding out. Was this a technical issue?
- Launching with a clean profile (which loads caminobrowser.org) then closing the window crashes in the autorelease pool every time.
- The fade effect on the left (which is very cool) currently blocks click-through to the URL.

>-static const int kFrameMargin = 1;
>+const int kFrameMargin = 1;
>+const int kPartitionLeftPadding = 3;
>+const int kPartitionTransitionWidth = 30;

These should all be static too.

Comment explaining kPartitionTransitionWidth; it took me a while to figure out what it referred to. Or maybe just change "Transition" to "Fade"? Transition made me think it was animation-related.

>+  theRect.size.width -= partitionWidth + (partitionWidth > kPartitionTransitionWidth - kPartitionLeftPadding ? kPartitionLeftPadding - kPartitionTransitionWidth : 0);

This is hard to grok. Split it up with some explanatory variables for intermediate bits maybe?

>+typedef enum _IconDisplayStatus {
>+  SecureAndFeedIconHidden = 0,
>+  SecureIconDisplayed,
>+  SecureIconHidden,
>+  FeedIconDisplayed,
>+  FeedIconHidden,
>+  SecureAndFeedIconDisplayed,
>+} IconDisplayStatus;

This enum confuses me. How are there 6 entries in an enum for something that only has four possible states? Also, an enum for flag combinations doesn't scale well; is there any reason not to just have two setFooVisible:(BOOL)visible (or something along those lines) instead, corresponding to bools internally?

>+@interface LocationBarPartitionView : NSView

Put this in a new file; it's always easier to navigate small files corresponding to individual classes.

>+  NSColor*  mTopTopColor;          // owned
>+  NSColor*  mTopBottomColor;       // owned
>+  NSColor*  mBottomTopColor;       // owned
>+  NSColor*  mBottomBottomColor;    // owned

How about m{Top,Bottom}Half{Start,End}Color; seems like it would be much more intuitively understandable.

>+  NSColor*  mLineTopColor;         // owned
>+  NSColor*  mLineBottomColor;      // owned

mTopLineColor and mBottomLineColor, so it reads more like English.

>+- (void)requestIconDisplayStatus:(IconDisplayStatus)status;
>+- (void)processStatusChangeRequests;
>+- (void)setSecureImage:(NSImage *)image;
>+- (void)setLockIconContextMenu:(NSMenu *)aMenu;
>+- (void)resize;

When you move this class to a file, make these public on private as appropriate, and document all of them.

>+    mMenuNotificationName = @"WillShowFeedMenu";

You've removed the setter, so this can presumably be a constant now.

>+  [mFeedImage dealloc];
>+  if (mSecureImage)
>+    [mSecureImage dealloc];

Never ever call dealloc except for [super dealloc].

>+  NSRect topRect;
>+  NSRect bottomRect;
>+  NSRect leftRect;
>+  NSDivideRect(rect, &leftRect, &rect, kPartitionTransitionWidth, NSMinXEdge);
>+  NSDivideRect(rect, &bottomRect, &topRect, floor(NSHeight(rect) * 0.5), NSMinYEdge);

Move the declarations immediately above each use to make it clearer what is set where.

> - (NSMenu*)menuForEvent:(NSEvent *)theEvent
> {
>-  // If we are the a feed icon, post this notification so the menu can get built.
>-  if (mMenuNotificationName) {
>-    [[NSNotificationCenter defaultCenter] postNotificationName:mMenuNotificationName 
>+  [self setMenu:nil];
>+  NSPoint clickPoint = [self convertPoint:[theEvent locationInWindow] fromView:nil];
>+  if (NSPointInRect(clickPoint, mFeedIconRect)) {
>+    [[NSNotificationCenter defaultCenter] postNotificationName:@"WillShowFeedMenu" 
>                                                         object:self];
>+  } else if (NSPointInRect(clickPoint, mSecureIconRect)) {
>+    [self setMenu:mLockIconContextMenu];
>   }
>   return [self menu];

Is [self setMenu:] actually doing anything here?

>+- (void)resize

Needs a more descriptive name. sizeToFitIcons?

>+  const int kLeftHorizontalPadding = 2; // The inside padding between the left partition frame and the left edge.

I think the variable is descriptive enough that the comment isn't necessary. If you leave it though, put in on the line above since it's pretty long for an end-of-line comment.

>+  const int kSecureIconMinY = 2.0;
>+  const int kFeedIconMinY = 3.0;

If they are ints, init them in with integer constants. I assume you got these by experimenting and seeing what made them line up well? If so, add a comment saying that (which will help make it clear that these are linked to the actual icons, rather than being independent values you chose like all the others).

>+  NSRect frame;
>+  frame.size.width = partitionFrameCount * kPartitionFrameWidth;
>+  frame.size.height = kPartitionHeight;
>+  frame.origin.x = NSWidth(textFieldRect) - NSWidth(frame) - kFrameMargin;
>+  frame.origin.y = kPartitionMinY;

NSMakeRect? Your call if you think this is easier to read though.

>+  // If the width is 0, there is no need to do any other work.
>+  // Skip to the end and set the partition frame.
>+  if (NSWidth(frame) > 0) {

Since NSWidth is a float, how about something like > 0.0001? I think it doesn't matter for zero, but it's a good habit to get into.

>+        mSecureIconRect = NSInsetRect(leftRect, 0.5 * (kPartitionFrameWidth - [mSecureImage size].width), kSecureIconMinY);

This seems wrong. Why inset the top and bottom by something called MinY? It sounds like it should be an offset rather than a shrink.

>+  // cache the bg color for secure pages.

Since you are modifying the comment anyway, s/cache/Cache/ and s/bg/background/.

>   [mSecureBackgroundColor release];
>   mSecureBackgroundColor = nil;
>   
>+  [mPartitionView release];
>+  mPartitionView = nil;
>   [mLock release]; 
>   mLock = nil;
>   [mFeedIcon release];
>   mFeedIcon = nil;

mLock and mFeedIcon are gone; I had to remove them to compile. Also, none of this UI stuff ever really belonged in cleanup; move the background color and your view release to dealloc (and kill the = nil, since it's not necessary once it's in dealloc).

Comment 3

[Dan Weber]

Attachment: Patch 2

Comment 4

[Stuart Morgan]

Comment on attachment 396342 [details] [diff] [review]
Patch 2

Sorry for the delay. This patch looks really good, just some small tweaks and then we'll be good to go!

This patch is missing the Xcode project changes.

>+  if (inShouldDisplay)
>+    [mPartitionView setDisplaySecureIcon:YES];
>+  else
>+    [mPartitionView setDisplaySecureIcon:NO];

No need for the if/else; just pass in inShouldDisplay as the argument.

>+// Called when BrowserWindowController wants us to show the RSS icon.

Don't reference the caller here (since that can get out of date easily).

>+  if (inDisplay)
>+    [mPartitionView setDisplayFeedIcon:YES];
>   else
>+    [mPartitionView setDisplayFeedIcon:NO];

You can get rid of the if/else structure here too now that it's a simple passthrough.

>+ * Portions created by the Initial Developer are Copyright (C) 2008

2009 (both files)

>+extern const int kPartitionViewLeftPadding;
>+extern const int kPartitionViewFadeWidth;

It feels a bit odd to have these exposed; since it looks like all you need them for externally is to figure out how much of the width opaque, make a public opaqueWidth method instead and spare clients the details.

>+@interface LocationBarPartitionView : NSView

Needs a class-leel comment

>+  NSColor*  mTopHalfStartColor;     // owned
>+  NSColor*  mTopHalfEndColor;       // owned
>+  NSColor*  mBottomHalfStartColor;  // owned
>+  NSColor*  mBottomHalfEndColor;    // owned
>+  NSColor*  mTopLineColor;          // owned
>+  NSColor*  mBottomLineColor;       // owned
>+  NSColor*  mClearColor;            // owned

Since these are just used to build gradients, is there any reason not to cache the gradients, rather than the colors?

>+  NSMenu*   mSecureIconContextMenu;

Why isn't this an owning ref? Seems really dangerous. If it's absolutely necessary, comment it *very* explicitly on the setter.

>+// Toggles the visibility of the feed icon.
>...
>+// Toggles the visibility of the secure icon.

Change "Toggles" to "Sets"; toggling would flip the existing state rather than taking an argument.

>\ No newline at end of file

Needs a newline, it looks like ;)

>+  [self setMenu:nil];
>+  NSPoint clickPoint = [self convertPoint:[theEvent locationInWindow] fromView:nil];
>+  if (NSPointInRect(clickPoint, mFeedIconRect)) {
>+    [[NSNotificationCenter defaultCenter] postNotificationName:kWillShowFeedMenuNotification object:self];
>+  } else if (NSPointInRect(clickPoint, mSecureIconRect)) {
>+    [self setMenu:mSecureIconContextMenu];
>+  }

Looks like that first [self setMenu:] would make more sense inside the |if|.

Comment 5

[Stuart Morgan]

It would be a shame not to ship this since we all like it, and Dan went to the trouble of implementing it; I'll take this over.

Comment 6

[Stuart Morgan]

Attachment: v3

Addresses my review comments.

Comment 7

[Eiichi]

(In reply to comment #6)
> Created attachment 500872 [details] [diff] [review]

I made a test build with this patch.
It seems that the positions of a secure icon and a feed icon are reversed.

Comment 8

[Eiichi]

Attachment: Screen shot

Screen shot of a test build.

Comment 9

[Stuart Morgan]

Attachment: v4

I'm not sure it really matters either way, but I guess having the lock in a consistent location is better. This swaps them back to the current order.

Comment 10

[Stuart Morgan]

Landed as http://hg.mozilla.org/camino/rev/06f1c2736962

Comment 11

[Stuart Morgan]

And then landed http://hg.mozilla.org/camino/rev/731ba87eb3e1 to fix the crash from a bonehead setter mistake I introduced :(