390575 - Make Tabsposé handle mouse clicks

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Jeff Dlouhy]

Allow the user to switch to the selected tab thumbnail and have the Tabsposé view disappear.

Comment 1

[Jeff Dlouhy]

Attachment: Click Handling Patch v1

Click handling works by a ThumbnailView getting a mouseDown: event, which tells its delegate that it was selected. The delegate (TabThumbnailGridView) then changes to the selected tab and calls its delegate (BrowserContentView) which hides Tabsposé.

Comment 2

[Stuart Morgan]

Comment on attachment 274872 [details] [diff] [review]
Click Handling Patch v1

>+- (void)setDelegate:(id)del;

Change |del| to |delegate| here and in the implementation.

>+- (void)setTabViewItem:(NSTabViewItem*)tabViewItem;
>+- (NSTabViewItem*)tabViewItem;

In the spirit of keeping this class generic, lets use
(void)setRepresentedObject:(id)object;
and
(id)representedObject;

>+- (void)setDelegate:(id)del
>+{
>+mDelegate = del;
>+}

Fix indentation.

>+- (void)mouseDown:(NSEvent*)theEvent
>+{
>+  if ([mDelegate respondsToSelector:@selector(selectionMadeForThumbnailView:)])
>+    [mDelegate selectionMadeForThumbnailView:self];
>+  
>+  [[self nextResponder] mouseDown:theEvent];
>+}

Why are you pass the mouse event along after having handled it?

>+- (void)selectionMadeForGridView
>+{
>+  [self hideTabThumbnailGridView];
>+}

In thinking about this some more, I realized it's inconsistent to have the tabsposé view tied directly to BWC for selecting the tab, but simultaneously going through the delegate dance with BCV. We're going to need a toggleTabThumbnailView:(id)sender method in BWC before long anyway, so instead of doing this lets add that now, hook it up to BCV, and have TTGV just call that after it does the tab selection.

Comment 3

[Jeff Dlouhy]

Attachment: Click Handling Patch v2

Fixed issues addressed in comment #2 as well as cleaned up the toggle behavior.

Comment 4

[Stuart Morgan]

Comment on attachment 274879 [details] [diff] [review]
Click Handling Patch v2

Toss a:

@interface NSObject (ThumbnailViewDelegate)

- (void)selectionMadeForThumbnailView:(ThumbnailView*)selectedView;

@end

onto the end of ThumbnailView's header to declare the informal protocol method (to prevent missing method warnings) and r=me.

Tabsposé is much more fun when clicking works :)

Comment 5

[Jeff Dlouhy]

Attachment: Click Handling Patch v3

Added delegate method to ThumbnailView's header.

Comment 6

[Sean Murphy]

Comment on attachment 274924 [details] [diff] [review]
Click Handling Patch v3

Some more nice work Jeff; r=me with the following points fixed or considered...

+- (void)selectionMadeForThumbnailView:(ThumbnailView*)selectedView
+{
+  BrowserWindowController* bwc = [[self window] windowController];
+  BrowserTabView* tabView = [bwc getTabBrowser];

I feel like we need to cast |windowController| to return the more specific |BrowserWindowController*|, but there are no compiler warnings as is :-/

+- (void)setRepresentedObject:(id)object
+{
+  [mRepresentedObject release];
+  mRepresentedObject = [object retain];
+}

ThumbnailView will need to release the retained object during its dealloc to avoid a leak.  Also, releasing in the setter without an identical object check isn't a problem with the current usage of this method, but we might want to make it more robust just in case: 

if (mRepresentedObject != object) {
  // release, retain
}

+- (void)mouseDown:(NSEvent*)theEvent
+{
+  if ([mDelegate respondsToSelector:@selector(selectionMadeForThumbnailView:)])
+    [mDelegate selectionMadeForThumbnailView:self];
+}

I was thinking here, should we trigger the selection on mouseUp: instead?  Waiting just seems more natural to me since most other UI actions are performed upon release, not instantaneously on mouseDown.  What do you guys think?

+- (void)toggleTabThumbnailGridView
+{
+  if (![mTabThumbnailGridView isDescendantOf:self])
+    [self showTabThumbnailGridView];
+
+  else if ([mTabThumbnailGridView isDescendantOf:self])
+    [self hideTabThumbnailGridView];
+}

I hope I'm not missing something when I say we can call |isDescendantOf:| only once:

if ([mTabThumbnailGridView isDescendantOf:self])
  [self hideTabThumbnailGridView];
else
  [self showTabThumbnailGridView];

Comment 7

[Jeff Dlouhy]

Attachment: Click Handling Patch v4

Fixed and agreed with all of murph's concerns. I switched the click to mouseUp:, this was something I was wondering about doing before since I would like to add dragging at some point.

Comment 8

[Håkan Waara]

Comment on attachment 275263 [details] [diff] [review]
Click Handling Patch v4

I've been looking at the Tabspose changes, and playing around with it on trunk and it's great!

> - (void)removeSubviews

I know there are no changes here in this patch, but I think this would be useful to add to our NSView category in NSView+Utils.h/m


>+// Change the tab to the selected ThumbnailView
>+//
>+- (void)selectionMadeForThumbnailView:(ThumbnailView*)selectedView
>+{
>+  BrowserWindowController* bwc = (BrowserWindowController*)[[self window] windowController];
>+  BrowserTabView* tabView = [bwc getTabBrowser];
>+
>+  [tabView selectTabViewItem:[selectedView representedObject]];
>+  [bwc toggleTabThumbnailView:self];
>+}

Maybe thumbnailViewWasSelected? I guess the placement of words here is to make sure "..ThumbnailView:" is at the end, before the arg? In my opinion, when there's only 1 argument, that's not strictly necessary - but I'd like to hear your opinions.

> @interface ThumbnailView : NSView {
>   NSImage* mThumbnail;
>+  id mRepresentedObject;
>+  id mDelegate;
> }

Maybe mRepresentedObject should be an NSObject* since you expect to be able to send retain/release messages to it?

 
>+- (void)setRepresentedObject:(id)object
>+{
>+	if (mRepresentedObject != object) {
>+		[mRepresentedObject release];
>+		mRepresentedObject = [object retain];
>+	}
>+}

There are tabs here, and in some other places. Please remove

>+
>+- (id)representedObject;
>+{
>+  return mRepresentedObject;
>+}
>+
>+- (void)setDelegate:(id)delegate
>+{
>+  mDelegate = delegate;
>+}
>+
>+- (id)delegate
>+{ 
>+  return mDelegate;
>+}
>+
> - (void)drawRect:(NSRect)rect
> {
>   NSShadow* shadow = [[[NSShadow alloc] init] autorelease];
>   [shadow setShadowOffset:NSMakeSize(kShadowX, kShadowY)];
>   [shadow setShadowBlurRadius:kShadowRadius];
>   [shadow set];
> 
>   if (mThumbnail) {
>@@ -73,15 +96,22 @@
> 
>     [mThumbnail drawInRect:NSInsetRect([self bounds], kShadowPadding, kShadowPadding)
>                   fromRect:NSZeroRect
>                  operation:NSCompositeSourceOver
>                   fraction:1];
>   }
> }
> 
>+- (void)mouseUp:(NSEvent*)theEvent
>+{
>+  if ([mDelegate respondsToSelector:@selector(selectionMadeForThumbnailView:)])
>+    [mDelegate selectionMadeForThumbnailView:self];
>+}

Hmm. Are we going to want to implement things like highlighting while clicking etc? 

If it quacks like a NSControl, and looks like NSControl ... maybe ThumbnailView should really be an NSControl? 

Then you would get a lot of clicking behaviour things for free, as well as setDelegate: and more. It probably means some changes (I don't know exactly how much work?), so maybe not something for this bug - but I think it's worth considering if you will want to implement more advanced clicking/selecting behaviour.
 
>+- (IBAction)toggleTabThumbnailView:(id)sender;

I'm not a fan of using the IBAction and sender method signature unless it is really used as an action message; it's misleading. Do we have many other such cases? :-(

Please just make it (void)toggleTabThumbnailView; and place it near other such method declarations (doesn't look like you need the sender)

Cancelling the super-review request for now, hoping that's okay.

Comment 9

[Håkan Waara]

Even better, NSView+Utils.m already has a removeAllSubviews instance method for you to use, so the duplicate can be removed.

Comment 10

[Jeff Dlouhy]

(In reply to comment #8)
> 
> >+- (IBAction)toggleTabThumbnailView:(id)sender;
> 
> I'm not a fan of using the IBAction and sender method signature unless it is
> really used as an action message; it's misleading. Do we have many other such
> cases? :-(
> 
> Please just make it (void)toggleTabThumbnailView; and place it near other such
> method declarations (doesn't look like you need the sender)

This was added as per a conversation with smorgan, we are adding the IBAction now and will later use it for when we need a tabsposé button/control in toolbar/menu. Sorry that this was not clarified before.

Comment 11

[Jeff Dlouhy]

Attachment: Click Handling Patch v5

(In reply to comment #8)

> >+// Change the tab to the selected ThumbnailView
> >+//
> >+- (void)selectionMadeForThumbnailView:(ThumbnailView*)selectedView
> >+{
> >+  BrowserWindowController* bwc = (BrowserWindowController*)[[self window] windowController];
> >+  BrowserTabView* tabView = [bwc getTabBrowser];
> >+
> >+  [tabView selectTabViewItem:[selectedView representedObject]];
> >+  [bwc toggleTabThumbnailView:self];
> >+}
> 
> Maybe thumbnailViewWasSelected? I guess the placement of words here is to make
> sure "..ThumbnailView:" is at the end, before the arg? In my opinion, when
> there's only 1 argument, that's not strictly necessary - but I'd like to hear
> your opinions.

I am fine with 'thumbnailViewWasSelected' I tried to search around on what other classes use as their naming convention for selection delegates, this one seems fine. It's also shorter. ;)


> >+
> >+- (id)representedObject;
> >+{
> >+  return mRepresentedObject;
> >+}
> >+
> >+- (void)setDelegate:(id)delegate
> >+{
> >+  mDelegate = delegate;
> >+}
> >+
> >+- (id)delegate
> >+{ 
> >+  return mDelegate;
> >+}
> >+
> > - (void)drawRect:(NSRect)rect
> > {
> >   NSShadow* shadow = [[[NSShadow alloc] init] autorelease];
> >   [shadow setShadowOffset:NSMakeSize(kShadowX, kShadowY)];
> >   [shadow setShadowBlurRadius:kShadowRadius];
> >   [shadow set];
> > 
> >   if (mThumbnail) {
> >@@ -73,15 +96,22 @@
> > 
> >     [mThumbnail drawInRect:NSInsetRect([self bounds], kShadowPadding, kShadowPadding)
> >                   fromRect:NSZeroRect
> >                  operation:NSCompositeSourceOver
> >                   fraction:1];
> >   }
> > }
> > 
> >+- (void)mouseUp:(NSEvent*)theEvent
> >+{
> >+  if ([mDelegate respondsToSelector:@selector(selectionMadeForThumbnailView:)])
> >+    [mDelegate selectionMadeForThumbnailView:self];
> >+}
> 
> Hmm. Are we going to want to implement things like highlighting while clicking
> etc? 
> 
> If it quacks like a NSControl, and looks like NSControl ... maybe ThumbnailView
> should really be an NSControl? 
> 
> Then you would get a lot of clicking behaviour things for free, as well as
> setDelegate: and more. It probably means some changes (I don't know exactly how
> much work?), so maybe not something for this bug - but I think it's worth
> considering if you will want to implement more advanced clicking/selecting
> behaviour.

I don't know if this should be included in this patch, I want to do more more investigation and get some more feedback before/if we make it a NSControl.

Comment 12

[Håkan Waara]

Comment on attachment 276956 [details] [diff] [review]
Click Handling Patch v5

Fix the small nits below, and r=me.

>+//
>+// Change the tab to the selected ThumbnailView
>+//
>+- (void)thumbnailViewWasSelected:(ThumbnailView*)selectedView

Isn't camino-style usually (ThumbnailView *)?

>+{
>+  BrowserWindowController* bwc = (BrowserWindowController*)[[self window] 
windowController];

Same here

>+- (void)setRepresentedObject:(id)object
>+{
>+  if (mRepresentedObject != object) {
>+    [mRepresentedObject release];
>+    mRepresentedObject = [object retain];
>+	}
>+}

Tabs... 
 
>+- (void)toggleTabThumbnailGridView
>+{
>+	if ([mTabThumbnailGridView isDescendantOf:self])
>+		[self hideTabThumbnailGridView];
>+  else
>+    [self showTabThumbnailGridView];
>+}

Tabs! you should check your IDE indenting prefs :)

Comment 13

[Jeff Dlouhy]

Attachment: Click Handling Patch v6

(In reply to comment #12)
> (From update of attachment 276956 [details] [diff] [review])
> Fix the small nits below, and r=me.
> 
> >+//
> >+// Change the tab to the selected ThumbnailView
> >+//
> >+- (void)thumbnailViewWasSelected:(ThumbnailView*)selectedView
> 
> Isn't camino-style usually (ThumbnailView *)?

I believe the standard practice is '(ThumbnailView*)' I was nit picked about that one already :P

> Tabs! you should check your IDE indenting prefs :)

Yeah, I forgot to change that on Leopard. Woops..

Comment 14

[Stuart Morgan]

Comment on attachment 276970 [details] [diff] [review]
Click Handling Patch v6

>+  if (mRepresentedObject != object) {
>+    [mRepresentedObject release];
>+    mRepresentedObject = [object retain];
>+	}

There's still a tab here, but that can be fixed on checkin.

sr=smorgan

Comment 15

[Stuart Morgan]

Landed on trunk.