390909 - Tabsposé allows clicking in bookmark bar without reflecting action

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Chris Lawson (gone)]

1) Invoke Tabsposé.
2) Click a bookmark bar button or invoke a bookmark menu item.
3) Watch Tabsposé stay the same.
4) Resume normal browser usage by de-invoking Tabsposé and note that foreground tab no longer reflects the contents shown in Tabsposé.

I'm not sure what the "right" behaviour is here -- whether to hide the bookmark bar (if visible) and largely avoid this problem (although that still doesn't fix the bookmark menu problem), or whether to update the thumbnails if a page is in the middle of loading when Tabsposé is invoked (or changes after invocation, which seems like it could be pretty easy to do accidentally, and possibly on purpose). Either way, we need to do...something different from what we have now.

It would be awesome if the thumbnails were like live browser windows (the same way animations/QuickTime movies/etc. can continue playing in the Dock) but that might be asking a little too much. I'd be happy with triggering a redraw of a tab's thumbnail any time the contents of a that tab changed, to the extent that we could know about the change.

Comment 1

[Stuart Morgan]

The plan is to disable most actions in tabsposé mode.

Comment 2

[Samuel Sidler (old account; do not CC)]

I kind of wouldn't mind the "click the an item in the bookmark bar, open new tab for it, and show it in Tabspose using some sweet animation" option. (Similar to how MobileSafari handles popups.) Anyway, just an idea that sprung to mind...

Comment 3

[Jeff Dlouhy]

Attachment: Disable Actions Patch v1

Disables mostly everything once tabsposé is opened. One thing that has not been decided yet is handling of the location and search fields.

Comment 4

[Chris Lawson (gone)]

Comment on attachment 277460 [details] [diff] [review]
Disable Actions Patch v1

>Index: src/browser/BrowserWindowController.mm
>===================================================================

>@@ -1507,16 +1507,23 @@
> - (BOOL)validateToolbarItem:(NSToolbarItem *)theItem
> {
>   // Check the action and see if it matches.
>   SEL action = [theItem action];
>   // NSLog(@"Validating toolbar item %@ with selector %s", [theItem label], action);
>   if (action == @selector(back:)) {
>     // if the bookmark manager is showing, we enable the back button so that
>     // they can click back to return to the webpage they were viewing.
>+    // if the tabThumbnailGridView is showing, we disable the back button.
>+
>+    if ([self tabThumbnailGridViewIsVisible]) {
>+      [theItem setEnabled:NO];
>+      return NO;
>+    }
>+
>     BOOL enable = [[mBrowserView getBrowserView] canGoBack];

Why not just combine the declaration of |enable| with the previous conditional? I.e.:

BOOL enable = ([[mBrowserView getBrowserView] canGoBack] || !([self tabThumbnailGridViewIsVisible]));

That saves some code and avoids the early return.

Same applies to the canGoForward hunk about 30 lines further down.


>Index: src/application/MainController.mm
>===================================================================

>@@ -1574,17 +1574,21 @@
> #pragma mark Menu Maintenance
> 
> - (BOOL)validateMenuItem:(NSMenuItem*)aMenuItem
> {
>   BrowserWindowController* browserController = [self getMainWindowBrowserController];
>   SEL action = [aMenuItem action];
> 
>   // NSLog(@"MainController validateMenuItem for %@ (%s)", [aMenuItem title], action);
>-
>+  
>+  // disable everything when TabThumbnailGridView is visible
>+  if(browserController && [browserController tabThumbnailGridViewIsVisible])
>+    return NO;
>+  

No whitespace on blank lines, please.

r=me with those questions/comments addressed.

Comment 5

[Jeff Dlouhy]

Attachment: Disable Actions Patch v2

Comment 6

[froodian (Ian Leue)]

Comment on attachment 277951 [details] [diff] [review]
Disable Actions Patch v2

>+    BOOL enable = ([[mBrowserView getBrowserView] canGoBack] || !([self tabThumbnailGridViewIsVisible]));

I believe you mean

BOOL enable = ([[mBrowserView getBrowserView] canGoBack] && ![self tabThumbnailGridViewIsVisible]);

(and the same below)

Also, we should probably keep "New Window" enabled when it's visible, since you can have other perfectly functioning windows, you just can't create them.

History menu items need to be disabled too.

The overall approach of this solution is OK, but it still leaves the problem that halfway loaded (or completely unloaded) tabs remain halfway loaded, since the thumbnails aren't live versions of the page.  Regardless though, this work should probably still be done, so please file that as a pie-in-the-sky bug (if it's not already - in case you didn't notice, I'm a little out of touch these days).

Also, there are some issues with the lazily loaded bookmarks menu not updating until you show the menu (for example, invoke thumbnails, Cmd-B, it loads bookmarks).  If I recall correctly, you want a [[NSApp delegate] delayedAdjustBookmarksMenuItemsEnabling]; when invoking or uninvoking.

Comment 7

[Jeff Dlouhy]

Attachment: Disable Actions Patch v3

Now disables bookmark actions thanks to [[NSApp delegate] delayedAdjustBookmarksMenuItemsEnabling];

'New Window' now stays enabled, while history also gets disabled.

Comment 8

[froodian (Ian Leue)]

Comment on attachment 282499 [details] [diff] [review]
Disable Actions Patch v3

>-    BOOL enable = [[mBrowserView getBrowserView] canGoBack];
>+    // if the tabThumbnailGridView is showing, we disable the back button.
>+
>+    BOOL enable = ([[mBrowserView getBrowserView] canGoBack] && ![self tabThumbnailGridViewIsVisible]);

nit: here and the other place you do this, don't leave a bare line after the comment.  it detaches the comment from the code it applies to

>   mTabThumbnailGridView = nil;
>+  [[NSApp delegate] delayedAdjustBookmarksMenuItemsEnabling];

nit: add the comment here too

>+  // Keep "New Window" active when tabThumbnailGridViewIsVisible
>+  if (browserController && [browserController tabThumbnailGridViewIsVisible] && 
>+      action == @selector(newWindow:))
>+    return YES;

Do we ever want to disable newWindow?  I think not.  If that's the case, just have if (browserController && action == @selector(newWindow:)) and change the comment

Comment 9

[Jeff Dlouhy]

Attachment: Disable Actions Patch v4

Fixes nits and comments stated above

Comment 10

[Stuart Morgan]

Comment on attachment 282503 [details] [diff] [review]
Disable Actions Patch v4

>+  [[NSApp delegate] delayedAdjustBookmarksMenuItemsEnabling];

BVC doesn't (and shouldn't) know about this method, so put this at the end of BWC's toggle method instead.

sr=smorgan with that change, but we need a follow-up bug about the location and search fields.

Comment 11

[Stuart Morgan]

Comment on attachment 282503 [details] [diff] [review]
Disable Actions Patch v4

I thought I might be able to just tweak this on checkin, but as I did so I found a couple of issues I missed in review; at this point I'm far enough through it that rather than go another round of review comments I'll just finish it up and post a modified patch in the next day or two.

Comment 12

[Stuart Morgan]

Attachment: v5

This restructures the patch to abstract the logic some, and to bring it up-to-date with the current state of the tree.

Comment 13

[Stuart Morgan]

Attachment: v5, really

Whoops, wrong version of the file.

Comment 14

[Jeff Dlouhy]

Comment on attachment 318071 [details] [diff] [review]
v5, really

Seems to be working correctly. Next we'll tackle the location bar.

r=jeff

Comment 15

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 318071 [details] [diff] [review]
v5, really

sr=pink

Comment 16

[Stuart Morgan]

Landed on trunk; we should get new bugs filed for anything left.