377248 - [SoC] Camino : Tabosé (or Tabsposé or Tabposé)

Status: VERIFIED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[chris hofmann]

Tracking bug for progress on summer of code project

Comment 2

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

Pulling to 1.6 per roadmap.

Comment 3

[Jeff Dlouhy]

Attachment: Add TabposeView.h

Comment 4

[Jeff Dlouhy]

Attachment: Add TabposeView.mm

Comment 5

[Jeff Dlouhy]

Attachment: Initial Patch

Comment 6

[Jeff Dlouhy]

Attachment: Add TabposeView to BrowserWindow

Comment 7

[Nick Kreeger]

Comment on attachment 270285 [details] [diff] [review]
Initial Patch

>+@class TabposeViewItem;
>+@interface TabposeView : NSView {
>+  int vertPadding, horzPadding;
>+  int columns, rows;
>+}

Please put a blank line between the @class and the @interface declarations.
Also, could we use the 'm' prefix for these members and declare each int on a new line. Like:
  int mNumCols;
  int mNumRows;

Finally, vertPadding and horzPadding should be constants in your current version of your patch. See my comment below about naming and assignment.

+- (void)drawViews:(NSArray *)views asNewSubviews:(BOOL)add;

I'm not a big fan of the name of the in-param 'add', maybe something like 'inShouldAddSubviews' or 'shouldAddSubviews'.

+- (NSImage *)imageWithScreenShotInView:(NSView *)myView;

Same here (in-param name), is it really 'myView'?

>+//Core Graphics & Shading
>+void drawGradientBackground(CGContextRef context, NSRect rect);
>+static void VerticalGrayGradient(void* inInfo, float const* inData, float* outData);
>+void shadeBackground(CGContextRef context, NSRect rect);
>+CGColorSpaceRef getTheGreyColorSpace(void);
>+@end
>\ No newline at end of file

Should these really be declared in the TabposeView class? It appears that all of these functions are only used in TabposeView, and should be declared at the top of TabposeView.m.

>+// View handling
>+- (void)removeSubviews;
>+
>+// Grid Calculations and drawing
>+- (void)enumerateTabsToViews;
>+- (void)updateGridSizeFor:(int)num;
>+- (void)drawViews:(NSArray *)views asNewSubviews:(BOOL)add;
>+- (NSImage *)imageWithScreenShotInView:(NSView *)myView;
>+
>+//Core Graphics & Shading
>+void drawGradientBackground(CGContextRef context, NSRect rect);
>+static void VerticalGrayGradient(void* inInfo, float const* inData, float* outData);
>+void shadeBackground(CGContextRef context, NSRect rect);
>+CGColorSpaceRef getTheGreyColorSpace(void);

All of these functions are used privately, and should be declared in a private category in the .m file. In your current iteration, you only trigger drawing the view by using NSView classes. Your private declaration will look like:

@interface TabposeView (Private)
// declare methods here.
@end

Also (style nit), let's be consistent with the position of the pointer (your obj-c declarations use a different style than the CG stuff). Most of the Camino code already uses |foo* aFoo|.

>+#pragma mark -
>+#pragma mark NSView

+#pragma mark -
+#pragma mark Grid Calculations

First, let's only use one #pragma mark instead of two.


>+- (id)initWithFrame:(NSRect)frame
>+{
>+	self = [super initWithFrame:frame];
>+	if (self) {
>+		vertPadding = 25;
>+    horzPadding = 25;
>+    [self setAutoresizesSubviews:NO];
>+	}
>+	return self;
>+}

Looks like some tabs snuck in here, I know that Xcode can be tricky when working with Tabs. You can try TextWrangler for removing tabs from a massive piece of code.

>+	self = [super initWithFrame:frame];
>+	if (self) {

This can be reduced to:
if ((self = [super initWithFrame:frame])) { ... 

Let's also define |vertPadding| and |horzPadding| as constants, using the 'k' prefix. You might also be a bit more specific as to what we are padding in the variable names.

>+- (void)setHidden:(BOOL)flag
>+{
>+  [super setHidden:flag];
>+  if(!flag){
>+    [self removeSubviews];
>+    [self enumerateTabsToViews];
>+  }
>+}

Again spacing at the if statement. See my comment below about your view organization (below the drawRect: comments)

>+- (void)drawRect:(NSRect)rect 
>+{
>+	//Set the BG to white
>+	[[NSColor whiteColor] set];
>+	NSRectFill(rect);
>+	
>+	//Get the graphics context
>+	NSGraphicsContext *gctx = [NSGraphicsContext currentContext];
>+	CGContextRef context = (CGContextRef)[gctx graphicsPort];
>+	
>+	drawGradientBackground(context,rect);
>+  
>+  [self drawViews:[self subviews] asNewSubviews:NO];
>+}  

More tabs here. Probably don't really need the comment about setting the background color, or getting the graphics context.

Also the graphics context line can be condensed into one line:
CGContextRef context = (CGContextRef)[[NSGraphicsContext currentContext] graphicsPort];

>+  [self drawViews:[self subviews] asNewSubviews:NO];

This is the beginning of my suggestions for restructuring your code ;-) 
While this works probably just fine, I would prefer that you track your subview classes in a NSMutableArray. That would make the class write cleaner and probably make the currently named 'enumerateTabsToViews' method a bit nicer.

I'm also not sure that firing off the individual tab view drawing code every time is the best idea. I could be wrong, but I could see some performance issues here. 
Here is why:
I think that we should not keep the tabpose view in the BrowserContentView at all times. The view shouldn't rely on |isHidden| to draw itself. Here is my suggestion for the current setup you have:

1. The user does something to invoke the tabpose process
2. The BrowserContentView adds the TabposeView as a child view, and assigns the frame dimensions.
3. BrowserContentView calls a function to tell the TabposeView to draw its mini-tab views.

Basically I'm proposing that you add a member boolean in your TabposeView class, init it to NO, and add a setter method that your owning class can set at will.

Please add a space after the comma:
>+	drawGradientBackground(context,rect);

+//
+// Removes the all the subviews of TabposeView
+//
+- (void)removeSubviews
+{

Since the method name tells us what is happening, the comment probably is unnecessary.

+//
+// Gets the current BrowserWindowController for the frontmost window, then
+// the tabs and proceeds to take thumbnails and draw them to the screen
+//
+- (void)enumerateTabsToViews
+{
+  //Get the tab view out of the main window controller
+  BrowserWindowController* bwc = [(MainController *)[NSApp delegate] getMainWindowBrowserController];
+	BrowserTabView *tabView = [bwc getTabBrowser];
+  NSArray *openTabs = [tabView tabViewItems];

Looks like another hidden tab and more random pointer placements.

I don't really like the name 'enumerateTabsToView', perhaps something like 'drawTabsToViews'. Plus that would eliminate the need to have the above comment.

+  if([tabView numberOfTabViewItems] > 0){
...
+    while ((tabViewItem = [tabEnum nextObject])){
...
+      if(tView){

Spacing here in between the if/left-paren and the right-paren/curly.

>+  if([tabView numberOfTabViewItems] > 0){
>+    NSEnumerator *tabEnum = [openTabs objectEnumerator];
>+    BrowserTabViewItem *tabViewItem;
>+    while ((tabViewItem = [tabEnum nextObject])){
>+      BrowserWrapper *bView = [tabViewItem view];               //Current site
>+      
>+      NSImage *thumb = [self imageWithScreenShotInView:bView];  //Take a picture
>+      [tabViewItem setView:bView];                              //Set the view back
>+      
>+      TabposeViewItem *tView = [[TabposeViewItem alloc] init];
>+      if(tView){
>+        [tView setImage:thumb];                                 //Set the site image
>+        [tabposeViews addObject:tView];                         //Add the view to the array
>+        
>+        [tView release];
>+      }
>+    }
>+    
>+    [self drawViews:tabposeViews asNewSubviews:YES];           //Draw the views as new subviews
>+    [tabposeViews release];
>+  }
>+}

Rather than using local names like 'tView' and 'bView', use something like 'curBrowserWrapper' and 'curTabposeView'.


>+      BrowserWrapper *bView = [tabViewItem view];               //Current site
>+      
>+      NSImage *thumb = [self imageWithScreenShotInView:bView];  //Take a picture
>+      [tabViewItem setView:bView];      

If the reason you get and set the same views was the stealing issue we discussed in IRC, please make that a clear comment as to why this behavior is needed.


>+- (void)updateGridSizeFor:(int)num
>+{
>+  NSSize cSize = [self frame].size;
>+  
>+  float x = num / (ceilf(sqrtf(num))); //Get the rough estimate on # of col
>+  
>+  float deci = x - floorf(x);          //Are we in the safe deci >.5 zone?
>+  
>+  if(deci > 0.0 && deci < 0.5){        //Since the decimal is less than .5 we need to add one more column
>+    columns = ceilf(sqrtf(num)) + 1;
>+    rows = ceilf((float)num/columns);
>+  }
>+  else{                                //Do not need to add 1 since above or equal to 0.5 or 0.0
>+    columns = ceilf(sqrtf(num));
>+    rows = ceilf((float)num/columns);
>+  }
>+}

Could we have a more specific name for the in-param?

>+  NSSize cSize = [self frame].size

This isn't used.

For whatever reason, this just seems harder than it needs to be. If you want to stick by this logic, please indicate what the algorithm is here. Could just be my stupidity, but it took me a good deal of time to understand this method. I took your method, and implemented it (I think, basing this on not much sleep and Starbucks) in two lines (plus comments):

  // For this algorithm, we first take the ceil'd square-root value
  // of the passed in number. This value will give us the number of 
  // columns that we want to use. After the number of columns has been
  // asssigned, we then figure out how many rows are needed.
  int columns = ceilf(sqrtf(num));  
  int rows = ceilf((float) num / numOfCols);


>+- (void)drawViews:(NSArray *)views asNewSubviews:(BOOL)add
>+{
>+  [self updateGridSizeFor:[views count]];  
>+  NSSize cSize = [self frame].size;                           //Whole view size
>+  float ratio = cSize.width/cSize.height;                     //Browser Ratio
>+  float vWidth = (cSize.width / columns) - (2 * horzPadding); //Subview Width
>+  float vHeight = vWidth / ratio;                             //Subview Height

Let's start here... ;-)
I'm not sure what a 'cSize' is, so I would recommend something like 'viewSize' or 'fullViewSize'. Same goes for 'vWidth' and 'vHeight', how about 'subviewWidth' and 'subviewHeight'. That would also do away with those comments.

Keep your spacing consistent, for example, keep a space in your operations here:
  >+  float ratio = cSize.width/cSize.height;
Because you spaced out your other operations and it's just plain easier to read.

I see your using your horizontal padding, but what about your vertical padding?

>+  //This will allow us to center the last row if # of views is odd
>+  unsigned firstItemInLastRow = ((rows - 1) * columns);

Please designate unsigned int.

>+  for(unsigned i = 0; i < [views count]; i++){
...
>+    if(edge <= cSize.width){
...
>+    //The newX will be centered if i is the first item in the last row
>+    else{
>+      if(i == firstItemInLastRow)

Spacing...

>+    TabposeViewItem *viewItem = [views objectAtIndex:i];
>+    float edge = newX + vWidth + (float)horzPadding - 1;      //Right edge of the view

Nit here, but I would call 'viewItem', 'curViewItem'.

>+  NSWindow* offscreenWindow = [[NSWindow alloc]
>+                initWithContentRect:offscreenRect
>+                          styleMask:NSBorderlessWindowMask 
>+                            backing:NSBackingStoreRetained
>+                              defer:NO];

Style nit here, but please move the 'initWithContentRect' to the same line as the alloc call, and aline from there:

NSWindow* offscreenWindow = [[NSWindow alloc] initWithContentRect:offscreenRect
                                                        styleMask:NSBorderlessWindowMask 
                                                          backing:NSBackingStoreRetained
                                                            defer:NO];

>+	// Create an image with the representation
>+	NSImage *image = [[[NSImage alloc] initWithSize:[rep size]] autorelease];
>+	[image addRepresentation:rep];

I think I made a comment earlier about not autoreleasing the object, since you are handling the retain/release in your view subclass.

>+	shadeBackground(context,rect);
...
>+	shading = CGShadingCreateAxial(getTheGreyColorSpace(),
>+                                 startPoint,endPoint,axialFunction,extendStart,extendEnd);
...
>+	if(shading == NULL){
>+		fprintf(stderr, "Couldn't create the shading!\n");
>+		return;
>+	}
...
>+  if(genericGreyColorSpace == NULL){
...
>+    if(&kCGColorSpaceGenericGray != NULL){
...
>+      if(genericGreyColorSpace == NULL){

Spacing nits...

>+	//The shading needs a function with 1 in and 4 outs
>+	if(axialFunction == NULL){
>+		return;
>+	}
...
>+      //If it's still NULL use the device grey
>+      if(genericGreyColorSpace == NULL){
>+        genericGreyColorSpace = CGColorSpaceCreateDeviceGray();
>+      }

No need for the curlies here and here.

>+#import <Cocoa/Cocoa.h>
>+
>+
>+@interface TabposeViewItem : NSView {
>+  NSImage *siteThumbnail;
>+}

Please remove the extra new line. Also, let's use the 'm' prefix for your NSImage member.

>+- (id)initWithFrame:(NSRect)frame{
>+    self = [super initWithFrame:frame];
>+    if (self) {
>+			siteThumbnail = nil;
>+    }
>+    return self;
>+}

Spacing concerns here as well. Your declaration could look cleaner without tabs and like this:

-(id)initWithFrame:(NSRect)aFrame
{
  if ((self = [super initWithFrame:aFrame]))
    mSiteThumbnail = nil;

  return self;
}

You're also missing a dealloc method to release your retained NSImage member.

>+- (void)setImage:(NSImage *)image;
...
>+- (void)setImage:(NSImage *)image
>+{
>+  [image retain];
>+  [siteThumbnail release];
>+  siteThumbnail = image;
>+}

First, the image that is passed in is autoreleased from the |imageWithScreenShotInView:| call. I would remove the autorelease call and deal with the release/retain call like you have it.

Perhaps their is a better name than just 'setImage', something to the tune of 'setSiteThumbnail:' would work.

Lastly, the retain assignment can look like:
  siteThumbnail = [image retain];

>+  if(siteThumbnail){

Spacing..

> @interface BrowserContainerView : NSView
> {
>   IBOutlet BrowserTabView *mTabView;
>   IBOutlet BrowserTabBarView *mTabBarView;
>+  IBOutlet TaposeView        *mTaposeView;

Not you, but if you are aligning the member declarations, please also line up 'mTabView'.

>+// Temporary, For testing purposes only
>+//
>+- (BOOL)performKeyEquivalent:(NSEvent *)theEvent
>+{
>+    //Control+Command+T envokes tabpose
>+    if ([theEvent modifierFlags] & NSControlKeyMask && NSCommandKeyMask) {
>+        NSString *keystroke = [theEvent charactersIgnoringModifiers];
>+        if([keystroke isEqualToString:@"t"])
>+        {
>+            if([mTaposeView isHidden])
>+            {
>+                [mTabView setHidden:YES];
>+                [mTaposeView setHidden:NO];
>+            }
>+            else if(![mTaposeView isHidden])
>+            {
>+                [mTabView setHidden:NO];
>+                [mTaposeView setHidden:YES];
>+                [mTabView setNeedsDisplay:YES];
>+            }
>+            return YES;
>+        }
>+    }
>+    return NO;
>+}
> @end

Although this is temporary, you have tabs here as well, they could get hidden in a future patch.

Also, unless you are using any C++ class in your ObjC, use .m instead of .mm in the file extension.

If TabposeViewItem remains a small class, I think you should stick it in TabposeView.h/Tabpose.m, since that seems like the only place that it is getting used.

Comment 8

[Jeff Dlouhy]

Attachment: Initial Patch (revised)

Made suggested changes as well as added shadows and removed the dependency of needing to modify the NIB.

For testing purposes only "Control+Command+T" will bring up Tabpose.

Comment 9

[Nick Kreeger]

Comment on attachment 270850 [details] [diff] [review]
Initial Patch (revised)


>+- (void)drawRect:(NSRect)rect 
>+{
>+  [[NSColor whiteColor] set];
>+  NSRectFill(rect);
>+  
>+  CGContextRef context = (CGContextRef)[[NSGraphicsContext currentContext] graphicsPort];
>+  drawGradientBackground(context, rect);
>+  
>+  [self drawViews:[self subviews] shouldAddSubviews:NO];

The reason I suggested the |mShouldDrawSubviews| member boolean is so that you could suppress drawing the subviews at your 'drawViews:' method. So it should look like this:

  if (mShouldDrawSubviews)
    [self drawViews:[self subviews] shouldAddSubviews:NO]; 
    
There are a couple of reasons why I suggest this. First, it's more elegant, so bonus points. Next, your 'drawViews:' method does a lot of floating point calculations. Plus you call 'upgradeGridSizeFor:' in 'drawViews:', which contains more floating calculations. Finally, the stack allocated variables, while not super-expensive (size wise they're somewhat cheap) they never get used because your for loop will never get executed when there are no subviews to draw. The 'drawRect:' method of NSView will get called several times during the views span, so it makes sense to make that overridden method as efficient as possible (even when hidden).

>+- (NSImage*)imageWithScreenShotInView:(NSView*)browserView
>+{
>+  NSRect bounds = [browserView bounds];
>+  NSSize windowSize = bounds.size;
>+  
>+  NSRect offscreenRect = NSMakeRect(0, 0, windowSize.width, windowSize.height);
>+
...
>+  // Create the NSBItmapImageRep
>+  [[offscreenWindow contentView] lockFocus];
>+  NSBitmapImageRep* rep = [[NSBitmapImageRep alloc] initWithFocusedViewRect:
>+    NSMakeRect(0, 0, windowSize.width, windowSize.height)];
>+

You declared 'windowSize' at the beginning of the function, so you should use it in your NSBitmapImageRep initialization, rather than declaring the same thing. And a chance for more bonus points (or is it brownie points?) because you can put the call on one line rather than spanning it out over two.

>+CGColorSpaceRef getTheGreyColorSpace(void)
>+{
>+  static CGColorSpaceRef genericGreyColorSpace = NULL;
>+  
>+  if (genericGreyColorSpace == NULL) {
>+    
>+    //If the kCGColorSpaceUserGray is available, use it
>+    if (&kCGColorSpaceGenericGray != NULL) {
>+      genericGreyColorSpace = CGColorSpaceCreateWithName(kCGColorSpaceGenericGray);
>+      
>+      //If it's still NULL use the device grey
>+      if (genericGreyColorSpace == NULL)
>+        genericGreyColorSpace = CGColorSpaceCreateDeviceGray();
>+    }
>+  }
>+  return genericGreyColorSpace;
>+}

I'm sorry, I must of have missed this the first go-round. If something is static, let's move it out of the C function and somewhere close to where you declared your const ints. Since this is a singleton ref, please use the 's' prefix. 

>+  axialFunction = CGFunctionCreate(grays, 1, NULL, 2, NULL,
>+                                   &callbacks);
>+  
>+  //The shading needs a function with 1 in and 4 outs
>+  if (axialFunction == NULL) {
>+    return;
>+  }

Please remove the extra curlies, and while you're there, please move the '&callbacks' declaration to the same line.

+  //axialFuncion be gone!
+  CGFunctionRelease(axialFunction);

Minor spelling mistake here.

+//
+// Callback function that calculates a grey gradient
+// Taken from BookmarkToolbar.mm
+//
+static void VerticalGrayGradient(void* inInfo, float const* inData, float* outData)
+{
+  float* grays = NS_STATIC_CAST(float*, inInfo);
+  outData[0] = (1.0-inData[0])*grays[0] + inData[0]*grays[1];
+  outData[1] = 1.0;
+}

If we are reusing code here, especially a static function, then we need to find a universal place where your class and BookmarkToolbar can find it. So either make the function public in BookmarkToolbar.h, or find a new home for it.

>+  return genericGreyColorSpace;
>+}
>+
>+@end
>\ No newline at end of file
...
>+
>+@end
>\ No newline at end of file

Please add newlines at the end of your files.

>//Whole view size
..
>+//
>+// Takes an array of views and whether or not to add them as subviews
>+// this is so we can update the sizes later without having to re-add them
>+//

Style nit, let's keep spacing on your comments consistent. If I were to vote, I would choose one space after the //.

>+- (id)initWithFrame:(NSRect)frame{

Spacing at the curly.

>+	[shadow setShadowOffset:NSMakeSize(0,-3)];
>+	[shadow setShadowBlurRadius:5];
>+	[shadow set];
>+  
>+  if (siteThumbnail) {
>+    [siteThumbnail drawInRect:NSInsetRect([self bounds],7,7)

It would be nice to have constants for the magic numbers here like 7, 0, 5, and -3. Not critical, but they would be nice to have.

>+- (void)showTabpose
>+{
>+  if (!mTabposeView) {
>+    NSRect browserRect = [self bounds];
>+    float bookmarkBarHeight = NSHeight([mBookmarksToolbar frame]);
>+    NSRect newRect = NSMakeRect(0, 0, browserRect.size.width,browserRect.size.height - bookmarkBarHeight);
>+    mTabposeView = [[TabposeView alloc] initWithFrame:newRect];

Make sure you are releasing 'mTabposeView' at dealloc.

>+@interface TabposeViewItem : NSView {
>+  NSImage *siteThumbnail;
>+}

I'm just thinking a little ahead of you here maybe. Are you going to hold the URL string of the view item in this class? In other words, how did you plan on dispatching input events? If this is discussion is out of the scope of this patch, I apologize in advance.

This patch is much better than the previous one, especially the consistency in style. I'm not sure of the timetable or landing schedules that you and/or smorgan have worked out. But this first step of the feature looks good to me with the above issues that I have pointed out dealt with.

r=me with the changes above taken care of.

Comment 10

[Stuart Morgan]

> If we are reusing code here, especially a static function, then we need to find
> a universal place where your class and BookmarkToolbar can find it. So either
> make the function public in BookmarkToolbar.h, or find a new home for it.

It absolutely shouldn't go in BookmarkToolbar.h. The fact that both classes happen to have a vertical grey gradient is coincidental, so creating dependencies between files in unrelated parts of the code isn't desirable. If we had a file to use as dumping ground for random graphics functions, that would be fine, but I think making one is something we can do in a later bug if there's enough code to warrant it. For now, the duplication is fine.

Comment 11

[Jeff Dlouhy]

(In reply to comment #9)

> I'm just thinking a little ahead of you here maybe. Are you going to hold the
> URL string of the view item in this class? In other words, how did you plan on
> dispatching input events? If this is discussion is out of the scope of this
> patch, I apologize in advance.

In my most recent builds I added a method 'setTabViewItem:' that stores a pointer to the tab it is representing. Then once the user selects one of the TabposeViewItems 'selectTabViewItem:' gets called on the current BrowserTabView with the item's set BrowserTabViewItem. This changes what tab is currently selected in the browser window.

As for events, I have a delegate method 'selectionMadeForView:' which, will alert the delegate when a subview was selected. This however is not final and might not be needed.

Comment 12

[Jeff Dlouhy]

(In reply to comment #9)
> (From update of attachment 270850 [details] [diff] [review])
> 
> >+- (void)drawRect:(NSRect)rect 
> >+{
> >+  [[NSColor whiteColor] set];
> >+  NSRectFill(rect);
> >+  
> >+  CGContextRef context = (CGContextRef)[[NSGraphicsContext currentContext] graphicsPort];
> >+  drawGradientBackground(context, rect);
> >+  
> >+  [self drawViews:[self subviews] shouldAddSubviews:NO];
> 
> The reason I suggested the |mShouldDrawSubviews| member boolean is so that you
> could suppress drawing the subviews at your 'drawViews:' method. So it should
> look like this:
> 
>   if (mShouldDrawSubviews)
>     [self drawViews:[self subviews] shouldAddSubviews:NO]; 
> 
> There are a couple of reasons why I suggest this. First, it's more elegant, so
> bonus points. Next, your 'drawViews:' method does a lot of floating point
> calculations. Plus you call 'upgradeGridSizeFor:' in 'drawViews:', which
> contains more floating calculations. Finally, the stack allocated variables,
> while not super-expensive (size wise they're somewhat cheap) they never get
> used because your for loop will never get executed when there are no subviews
> to draw. The 'drawRect:' method of NSView will get called several times during
> the views span, so it makes sense to make that overridden method as efficient
> as possible (even when hidden).

Upon further review I think 'mShouldDrawSubviews' might not be needed at all. 'TabposeView' is not getting drawn when it not in the view. Right now 'BrowserContentViews' adds then removes 'TabposeView' as a subview depending if its enabled. So this does not cause it to redraw when hidden, because it is not attached to a view. The only time something like 'mShouldDrawSubviews' could be useful is if a subview is animating. The line [self drawViews:[self subviews] shouldAddSubviews:NO] must be called on every redraw otherwise the grid will not resize.

Comment 13

[Jeff Dlouhy]

Attachment: Initial Patch (revised v2)

Changes made in response to Comment #9.

> 
> >+	[shadow setShadowOffset:NSMakeSize(0,-3)];
> >+	[shadow setShadowBlurRadius:5];
> >+	[shadow set];
> >+  
> >+  if (siteThumbnail) {
> >+    [siteThumbnail drawInRect:NSInsetRect([self bounds],7,7)
> 
> It would be nice to have constants for the magic numbers here like 7, 0, 5, and
> -3. Not critical, but they would be nice to have.

I am going to keep the magical numbers in there for now and hold of on adding constants for the next major patch. This patch does not include any of the tab switching features as demonstrated in my screencast.

Comment 14

[Stuart Morgan]

Before getting into a full review, there's an architectural issue with the current patch in that it conflates drawing and layout (drawViews:shouldAddSubviews:). There are three conceptually different operations, which should be handled by different methods and triggered by different actions:
1) adding subviews to the view, which should be done once, when the tabpose layer is shown.
2) computing and setting the subview frames, which should be triggered after 1) and when the frame/bounds of the tabpose view change.
3) drawing, which should be triggered by drawRect:.

Two small notes for other sections of the code that won't be impacted by the above:
- The status bar now uses setHidden: to handle being hidden (e.g., by popup windows, but also potentially by a user default and eventually dynamically by a menu item) so you can't unconditionally call setHidden:NO on it when hiding the tabpose view anymore.
- Nit, but just for consistency please use the more standard form for your thumbnail setter:
if (image != mSiteThumbnail) {
  [mSiteThumbnail release];
  mSiteThumbnail = [image retain];
}

Please do add constants for the magic numbers mentioned above when you respin.

Comment 15

[Jeff Dlouhy]

Attachment: Addresses smorgan's comments

Comment 16

[Stuart Morgan]

Comment on attachment 273006 [details] [diff] [review]
Addresses smorgan's comments

Lots of small nits below, but structurally it looks great, so this is very close.

+@class TabposeViewItem;

This can be removed.

>+  BOOL mShouldDrawSubviews;

This appears to be unused, so it should be removed.

>+- (void)drawTabsToViews;

Rather than making clients of the view do this after adding it, just trigger things to happen in viewDidMoveToSuperview:

Also, to prevent the heavy part of this view (all the site snapshots) sticking around longer than needed, have it remove all subviews on viewDidMoveToSuperview:nil.

+const int kVertPadding = 25;
+const int kHorzPadding = 25;

Favor readability over compression, so kVerticalPadding and kHorizontalPadding.

>+void drawGradientBackground(CGContextRef context, NSRect rect);
>+static void VerticalGrayGradient(void* inInfo, float const* inData, float* outData);
>+void shadeBackground(CGContextRef context, NSRect rect);
>+CGColorSpaceRef getTheGreyColorSpace(void);

All your helper functions should be static, but is there a reason drawGradientBackground and shadeBackground are functions instead of methods?

>+  mSettingFrame = YES;

Please add the explanatory comment that was in the other class's implementation, since the reason for this construction isn't necessarily obvious.

>+// Gets the current BrowserWindowController for the frontmost window, then
>+// the tabs and proceeds to take thumbnails and draw them to the screen
>+- (void)drawTabsToViews

The method name and comment aren't really accurate anymore. How about something like createThumbnailViews? Also, mentioning BrowserWindowController is more implementation details than you need in a method-level comment.

>+  // Figure out the # of mNumCols we'll need
>+  [self updateGridSizeFor:[openTabs count]];

This shouldn't be necessary here; it's something that only matters later in layoutThumbnails.

>+  NSMutableArray* tabposeViewItems = [[NSMutableArray alloc] init];

There's no need for this array or the addSubviews: method if each new view is added to your subviews array as it's created.

>+  if ([openTabs count] > 0) {

This can be removed, since nothing here requires that the number of tabs be > 0. If it's a concern for the layout method, it can be an early return there.

>+      BrowserWrapper* curBrowserWrapper = [tabViewItem view];               // Current site

We generally don't like to do these kind of line-by-line comments, since if the methods are reasonably named they don't usually add any information that the call itself doesn't provide. The preferred commenting is a block explaining the overall flow of a block; in this case something before the |while| like:

// Walk through the open tabs, taking a snapshot of each
// one and creating a thumbnail view for it.

>+      [tabViewItem setView:curBrowserWrapper];                              // Set the view back

I'm not clear on what this is doing; is it fixing something that was changed in imageWithScreenShotInView:? If so it needs to be in imageWithScreenShotInView:.

>+      TabposeViewItem* curTabposeView = [[TabposeViewItem alloc] init];

We prefer autoreleasing at the time of alloc in general in the Camino code base, to reduce accidental leaks.

>+// Takes an array of views and whether or not to add them as subviews
>+// this is so we can update the sizes later without having to re-add them
>+- (void)layoutThumbnails

This comment is out of date now as well.

>+  NSSize viewSize = [self bounds].size;                                   // Whole view size
>+  float ratio = viewSize.width / viewSize.height;                         // Browser Ratio
>+  float subviewWidth = (viewSize.width / mNumCols) - (2 * kHorzPadding);  // Subview Width
>+  float subviewHeight = subviewWidth / ratio;                             // Subview Height

Again, no need for these comments since the variable names should be sufficient. I'd suggest s/ratio/aspectRatio/ though.

>+  float newX = kHorzPadding;
>+  // Centers the grid vertically
>+  float newY = kVertPadding + ((viewSize.height - (mNumRows * (subviewHeight + (2 * kVertPadding)))) / 2);

I'll leave this as a judgment call for you since I haven't actually tried the other way (and maybe you already have), but it seems like it might be cleaner to compute the position of each item directly based on its grid position, rather than incrementally based on the last item, since you have pre-computed all the grid info you need.

+  // This will allow us to center the last row if # of views is odd

s/odd/< mNumCols

>+    // Is the next view going to fix within the bounds?
>+    if (edge <= viewSize.width) {
>+      NSRect frame = NSMakeRect(newX, newY, subviewWidth, subviewHeight);
>+      [curViewItem setFrame:frame];
>+    }

Why would this be necessary? You computed the widths explicitly to fit based on the number of columns.

>+      newY += (2 * kVertPadding) + subviewHeight;
...
>+    newX += (2 * kHorzPadding) + subviewWidth;

Why 2*padding? Generally grid views have the same inter-item padding as they do overall edge padding, so it seems like the padding should be applied just once per item (and the other related math changed appropriately).

>+  NSRect bounds = [browserView bounds];
>+  NSSize windowSize = bounds.size;

  NSSize windowSize = [browserView bounds].size
(No need for the intermediate variable you don't use)

>+  NSWindow* offscreenWindow = [[NSWindow alloc] initWithContentRect:offscreenRect

This is a case where you should disregard my comment about autoreleasing, and keep it the way you have it ;) We don't want to build up a whole lot of window objects over the course of the loop this is being called from. Same with the bitmap below.

>+  // Create the NSBItmapImageRep

s/BI/Bi/

>+  // Create an image with the representation
>+  NSImage* image = [[NSImage alloc] initWithSize:[rep size]];

This image should be autoreleased when you create it, since it should be returned autoreleased (right now it's leaking).

>+@interface TabposeViewItem : NSView {

This is actually a very generic class that we could theoretically use for other reasons, so how about just ThumbnailView.

>+  NSImage* mSiteThumbnail;

... and then this is just mThumbnail.

>+- (void)setSiteThumbnail:(NSImage*)image;

and setThumbnail:

>+- (void)showTabpose

Go ahead and make a corresponding hideTabpose (and use that in your temporary pKE), since we'll need it or something like it with the final toggle UI too. It should probably release and nil mTabpose view as well, since you are using on-demand creation for it and there's minimal overhead to creating the main view itself.

Comment 17

[Jeff Dlouhy]

Attachment: Initial Patch (revised v4)

Changed class names to something a little more generic so we are not tied down to the *posé name. I also fixed some of the issues raised in comment #16.

Comment 18

[Stuart Morgan]

Comment on attachment 273447 [details] [diff] [review]
Initial Patch (revised v4)

>+    //Puts the tab's view back where it came from
>+    //then selects it so we don't get a blank view
>+    [tabViewItem setView:curBrowserWrapper];
>+    [[tabViewItem tabView] selectTabViewItem:tabViewItem];

Per IRC discussion, the setView: needs to happen as part of the thumbnailing method.

I haven't had time to try to figure out an alternative to the selectTabViewItem: hack, so for now lets go with pulling that out of the loop and have it just jump to another tab and then back to the current tab. It'll broken in the one-tab case, but we can mark it with a TODO:, get the code landed to unblock your other work, then come back to it.

>+  if([self superview])

|if (|

>+  float newX = kHorizontalPadding / 2;

The point of removing the doubling was so that the border around the grid should be the same width as the spacing between elements in the grid, but this undoes that.

>+  NSBitmapImageRep* rep = [[[NSBitmapImageRep alloc] initWithFocusedViewRect:offscreenRect] autorelease];

Sorry, this one you should also not autorelease, since again we don't want to build up a lot of heavy objects inside the view creation loop.

>+      if (![mTabThumbnailView isDescendantOf:self]) {
>+        [self showTabThumbnailView];
>+        [mStatusBar setHidden:YES];
>+      }
>+      
>+      else if ([mTabThumbnailView isDescendantOf:self]) {
>+        [self hideTabThumbnailView];
>+        if (!mStatusBarWasHidden)
>+          [mStatusBar setHidden:NO];
>+      }

Handling the status bar should be part of the show/hideTabThumbnailView methods.


A bigger issue is that in testing and playing around a bit, I could consistently crash by bringing up Tabsposé with plugin content in a tab. We'll have to talk about that today, and how to proceed in the short term; it's possible that longer term that this will require dropping down to doing the thumbnailing at the Gecko level.

Comment 19

[Jeff Dlouhy]

Attachment: Initial Patch (revised v5)

Changes made as requested in comment #18.

No solution yet for the 'selectTabViewItem' hack. This will be addressed in a later patch along with Gecko thumbnailing code.

'TabThumbnailView' (the class formerly know as 'TabposeView' formerly known as 'Prince') is now known as 'TabThumbnailGridView.'

The modified project file is also included in this patch. Whoo .xcodeproj!

Comment 20

[Jeff Dlouhy]

Attachment: Initial Patch (revised v6)

Fixed file path as per IRC conversation with Smokey.

(Note to self: have fewer revisions)

Comment 21

[Stuart Morgan]

Comment on attachment 273737 [details] [diff] [review]
Initial Patch (revised v6)

>+  //Puts the tab's view back where it came from
>+  //then selects it so we don't get a blank view
>+  [tabViewItem setView:browserWrapper];

The second line of this comment shouldn't be here anymore, and lets move this call up to between getting the bitmap and removing the window, since it seems like luck that the view hasn't actually be released at this point.

And, after playing around a bit, I found that you can (as far as I could tell) eliminate the console warning and the tab-selection hack by doing the following instead of just adding the content view directly to the offscreen window:
 - retain and autorelease the browser view (so it won't be dealloc'd)
 - create a blank dummy NSView the same size as the browser view
 - set the view of the tab to the dummy view
 - set the content of the offscreen window to the browser view
 - get the screenshot
 - set the view of the tab back to the browser view
 - dealloc the window, etc.

>   IBOutlet NSView           *mStatusBar;
>+  TabThumbnailGridView          *mTabThumbnailGridView;
>+  BOOL                       mStatusBarWasHidden;

Alignment needs fixing.

> #import "BookmarkToolbar.h"
> #import "BrowserTabView.h"
> #import "BrowserTabBarView.h"
> 
>+#import "TabThumbnailGridView.h"

No need for the blank line above.


The Xcode project change needs to add the two new files to CaminoStatic as well as Camino.

The only other general note is to scrub the whitespace from your blank lines when you respin.

With those changes, r=me :)

Comment 22

[Jeff Dlouhy]

Attachment: Initial Patch (revised v7)

Made final changes, removed spaces from whitespace, and fixed the 'selectTabViewItem' hack with smorgan's hack.

Comment 23

[Mike Pinkerton (not reading bugmail)]

+@interface TabThumbnailGridView : NSView {

can you put a file-level or class-level comment as to what this grid view is used for and maybe a summary about how it works?

+- (void)setFrame:(NSRect)inFrame
+{

maybe a bigger comment on these set of functions as to why you have to do this. It seems dubious, but I'm sure there's a good reason.

+  //Puts the tab's view back where it came from

fix spacing

+// Callback function that calculates a grey gradient
+// Taken from BookmarkToolbar.mm

any way we can share this between the two so we don't have to change it if we change color schemes?

+    fprintf(stderr, "Couldn't create the shading!\n");

not NSLog? Why specifically stderr?

+CGColorSpaceRef getTheGreyColorSpace(void)
+{
+

get rid of extra blank line

+@interface ThumbnailView : NSView {
+  NSImage* mThumbnail;
+}

file-level or class comment on why this is used and how it works.

sr=pink with those changes.

Comment 24

[Stuart Morgan]

(In reply to comment #23)
> +// Callback function that calculates a grey gradient
> +// Taken from BookmarkToolbar.mm
> 
> any way we can share this between the two so we don't have to change it if we
> change color schemes?

We talked about this above; the fact that they are the same is really just coincidental (we'll likely change the bookmark bar for Leopard, but not necessarily this). The second line of the comment should just be removed.

Comment 25

[Jeff Dlouhy]

Attachment: Initial Patch (revised v8)

Fixed problems in comment #23 and comment #24.

Also switched from using setFrame: to resizeSubviewsWithOldSize: as per IRC conversation with smorgan.

Comment 26

[Jeff Dlouhy]

Attachment: Initial Patch (revised v9)

Removed setBounds: initWithFrame: and changed dates from 2002 to 2007.

Comment 27

[Stuart Morgan]

Comment on attachment 274702 [details] [diff] [review]
Initial Patch (revised v9)

(Adding pink's sr+ flag)

Comment 28

[Stuart Morgan]

Landed on trunk!

All future work will be done on trunk, to keep things sane; leaving this open only as a tracker.

Comment 29

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

2.0 per status meeting.

Comment 30

[Jeff Dlouhy]

Code landed on trunk

Comment 31

[Stuart Morgan]

Erm, we were leaving this open as a tracker (per comment 28). Is there some SoC bookkeeping reason this needs to be closed? If so, we need a replacement tracker for all the bugs blocking this one.

Comment 32

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

Yes, the 2007 SoC is over, and we have working (if unpolished) Tabsposé code on the trunk. See bug 410866 on exposing the feature in the UI (and other bugs to fix before doing so).