390576 - Add site title to Tabsposé's ThumbnailView

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Jeff Dlouhy]

Add a title that will appear below each thumbnail.

Comment 1

[Jeff Dlouhy]

Attachment: Title Patch v1

Initial Patch for adding titles.

Comment 2

[Håkan Waara]

Comment on attachment 276263 [details] [diff] [review]
Title Patch v1

mThumbnailTitle is leaked (not released in dealloc) and there it seems you are using tabs.

Comment 3

[Sean Murphy]

Comment on attachment 276263 [details] [diff] [review]
Title Patch v1

>Index: src/browser/TabThumbnailGridView.mm
> ...
>+const int kThumbnailTitleHeight = 20;

>Index: src/browser/ThumbnailView.m
> ...
>+const int kThumbnailTitleHeight = 20;

The thumbnail title height constant should definitely be declared in only one class (imagine someone wanting to change it) and ThumbnailView is the best (only) home for it.  Even more than just declaring it once, I think it should only be referenced inside ThumbnailView as well.  To me, the best approach would be for TabThumbnailGridView to simply ask ThumbnailView for its size and not have to know anything about how that size breaks down and what different elements it may contain (such as image, shadow, title).  This way, if we change/enhance something about ThumbnailView's appearance, TabThumbnailGridView's layout process would hopefully not need to specifically take that individual change into consideration when laying out the thumbs, and instead just deal with the general frame (if that is possible).

> @interface ThumbnailView : NSView {
>   NSImage* mThumbnail;
>+	NSCell* mThumbnailTitle;
> }
> 
> - (void)setThumbnail:(NSImage*)image;
>+- (void)setThumbnailTitle:(NSString*)title;

This is very optional, but maybe change the variable name |mThumbnailTitle| to |mThumbnailTitleCell| because at first glance (and coupled with the setter) I thought it was declared as a simple NSString.

>+		// Adjust the view to make room for its title
>+		NSRect thumbRect = NSMakeRect(0, kThumbnailTitleHeight, NSWidth([self bounds]), NSHeight([self bounds]) - kThumbnailTitleHeight);
>+    [mThumbnail drawInRect:NSInsetRect(thumbRect, kShadowPadding, kShadowPadding)
>                   fromRect:NSZeroRect
>                  operation:NSCompositeSourceOver
>                   fraction:1];
>   }
>+	
>+	if (mThumbnailTitle)
>+		[mThumbnailTitle drawWithFrame:NSMakeRect(0, 0, NSWidth([self bounds]), kThumbnailTitleHeight) inView:self];
> }

As far as readability is concerned, rectangle construction during drawing routines often take me a long time to look at before I can really grasp their ultimate size and location.  This one's totally up to you, and it might only be easier for me to understand because I wrote it, but here's one other possible way of slicing up the title and image rects:

NSRect thumbnailImageRect;
NSRect thumbnailTitleRect;
NSDivideRect([self bounds], &thumbnailTitleRect, &thumbnailImageRect, kThumbnailTitleHeight, NSMinYEdge);

if (mThumbnail) {
  [[NSGraphicsContext currentContext] setImageInterpolation:NSImageInterpolationHigh];
  [mThumbnail setScalesWhenResized:YES];

  [mThumbnail drawInRect:NSInsetRect(thumbnailImageRect, kShadowPadding, kShadowPadding)
                fromRect:NSZeroRect
               operation:NSCompositeSourceOver
                fraction:1];
}

if (mThumbnailTitle)
	[mThumbnailTitle drawWithFrame:thumbnailTitleRect inView:self];

Also, I did write that quickly and it seems to have the same appearance, but if used it might need a double-check.  I don't mean to totally re-write your code, but to instead shine some light on one other possible and (maybe easier to maintain) way.

> - (id)initWithFrame:(NSRect)frame {
>-  if ((self = [super initWithFrame:frame]))
>-    mThumbnail = nil;
>-
>+  if ((self = [super initWithFrame:frame])) {
>+		mThumbnail = nil;
>+		mThumbnailTitle = [[NSCell alloc] initTextCell:@""];
>+		[mThumbnailTitle setLineBreakMode:NSLineBreakByTruncatingTail];
>+		[mThumbnailTitle setAlignment:NSCenterTextAlignment];
>+	}
>   return self;
> }

Thanks to Håkan for pointing out that mThumbnailTitle will need to be released in |dealloc| since you're allocating it here.

-[NSCell setLinkBreakMode:] is 10.4+ only, so you'll either have to see what can be done with |setWraps:| or maybe utilize our own TruncatingTextFieldCell class.

One additional note: visually it seems as though there's not enough padding on the top and bottom of the grid view.  This is probably another bug (and I think TabThumbnailGridView's problem).  I'll submit a bug and screenshot for that later tonight if it's not in here somewhere already.

I love receiving Tabposé review requests in my inbox, since they mean it's moving closer to completion!

Comment 4

[Jeff Dlouhy]

Attachment: Title Patch v2

I used murph's code snippet in drawRect: and really like the results!

I kept '[mThumbnailTitleCell setLineBreakMode:NSLineBreakByTruncatingTail];' in the code for now, we have to decide if we want to target 10.3 or just go 10.4+ only. If we do choose 10.3 removing setLineBreakMode does not make a big difference in the title's appearance.

Correct me if I am wrong, but wont using 'TruncatingTextFieldCell' mean that ThumbnailView.m will have to be a .mm? A CVS headache.

Comment 5

[Stuart Morgan]

We have a couple of options:
- Split NSString+Utils into two pieces, one with the Gecko bits that require ObjC++, and one that doesn't. I'd certainly be fine with that.
- Do our usual ifdef dance to declare the AppKit constant and method, and call it only on 10.4, leaving 10.3 with the fallback.

Comment 6

[Sean Murphy]

Comment on attachment 276886 [details] [diff] [review]
Title Patch v2

Jeff, I couldn't get the patch to apply.  It seems to have been diff'd against the files before mouse click support was introduced.

Anyway, I was able to integrate it manually; r=me with a few stray tab characters removed.

Comment 7

[Jeff Dlouhy]

Attachment: Title Patch v3

I removed the 'NSLineBreakByTruncatingTail' and will roll it in with a different bug once NSString+Utils is split up in Bug 392970

Comment 8

[Stuart Morgan]

Comment on attachment 277916 [details] [diff] [review]
Title Patch v3

>+  NSCell*   mThumbnailTitleCell;
...
>+- (void)setThumbnailTitle:(NSString*)title;

The "Thumbnail" parts of these can be dropped, since being members of the ThumbnailView class already conveys the context.

> const int kShadowX = 0;
> const int kShadowY = -3;
> const int kShadowRadius = 5;
> const int kShadowPadding = 5;
>+const int kThumbnailTitleHeight = 20;

Oops, missed this before; these should all be static.

sr=smorgan with those changes. This looks really slick.

Comment 9

[Håkan Waara]

(In reply to comment #8) 
> > const int kShadowX = 0;
> > const int kShadowY = -3;
> > const int kShadowRadius = 5;
> > const int kShadowPadding = 5;
> >+const int kThumbnailTitleHeight = 20;
> 
> Oops, missed this before; these should all be static.

Someone with stronger C++ fu than me may correct me, but aren't they implicitly static?

Comment 10

[Jeff Dlouhy]

Attachment: Title Patch v4

Comment 11

[Stuart Morgan]

Landed on trunk.