Make Tabsposé handle mouse clicks

RESOLVED FIXED

Status

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Jeff.Dlouhy, Assigned: Jeff.Dlouhy)

Tracking

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

12 years ago
Allow the user to switch to the selected tab thumbnail and have the Tabsposé view disappear.
(Assignee)

Comment 1

12 years ago
Posted patch Click Handling Patch v1 (obsolete) — Splinter Review
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é.
Attachment #274872 - Flags: review?(stuart.morgan)

Comment 2

12 years ago
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.
Attachment #274872 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 3

12 years ago
Posted patch Click Handling Patch v2 (obsolete) — Splinter Review
Fixed issues addressed in comment #2 as well as cleaned up the toggle behavior.
Attachment #274872 - Attachment is obsolete: true
Attachment #274879 - Flags: review?(stuart.morgan)

Comment 4

12 years ago
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 :)
Attachment #274879 - Flags: review?(stuart.morgan) → review+
(Assignee)

Comment 5

12 years ago
Posted patch Click Handling Patch v3 (obsolete) — Splinter Review
Added delegate method to ThumbnailView's header.
Attachment #274879 - Attachment is obsolete: true
Attachment #274924 - Flags: review?(murph)

Comment 6

12 years ago
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];
Attachment #274924 - Flags: review?(murph) → review+
(Assignee)

Comment 7

12 years ago
Posted patch Click Handling Patch v4 (obsolete) — Splinter Review
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.
Attachment #274924 - Attachment is obsolete: true
Attachment #275263 - Flags: superreview?(mikepinkerton)
(Assignee)

Updated

12 years ago
Attachment #275263 - Flags: review?(hwaara)

Comment 8

12 years ago
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.
Attachment #275263 - Flags: superreview?(mikepinkerton)
Attachment #275263 - Flags: review?(hwaara)
Attachment #275263 - Flags: review-

Comment 9

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

Comment 10

12 years ago
(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.
(Assignee)

Comment 11

12 years ago
Posted patch Click Handling Patch v5 (obsolete) — Splinter Review
(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.
Attachment #275263 - Attachment is obsolete: true
Attachment #276956 - Flags: review?(hwaara)

Comment 12

12 years ago
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 :)
Attachment #276956 - Flags: review?(hwaara) → review+
(Assignee)

Comment 13

12 years ago
(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..
Attachment #276956 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #276970 - Attachment description: Click Handling Patch v5 → Click Handling Patch v6

Comment 14

12 years ago
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
Attachment #276970 - Flags: superreview+

Comment 15

12 years ago
Landed on trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.