228840 - [patch] Bookmarks toolbar - Middle-click should open bookmark in new tab

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, enhancement)

Description

[Jonathan Dance]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6b) Gecko/20031213 Camino/0.7+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6b) Gecko/20031213 Camino/0.7+

This is part of a suite of bookmark toolbar enhancements to bring full
middle-click and cmd-click support for Camino's bookmarks toolbar, similar to
Firebird. I will post all the bug numbers in a comment once they are up.

This request is for middle-click support for regular bookmarks on the bookmarks
toolbar. It should behave exactly as a Cmd-click on the bookmark. When
middle-clicking, the bookmark should be opened in a new tab and, depending on
your "Load opened windows or tabs in the background" preference, that tab should
be given focus if that preference is not checked.

Reproducible: Always

Steps to Reproduce:
1. Middle-click on a bookmark in your bookmarks toolbar.
Actual Results:  
Nothing.

Expected Results:  
The bookmark is opened in a new tab, and depending on the "Load opened windows
or tabs in the background", the tab should be given focus if preference not checked.

Comment 1

[Jonathan Dance]

The bug numbers are:
bug 228478 - Cmd-click support for bookmarks in folders
bug 228839 - Middle-click support for bookmarks in folders
bug 228840 - Middle-click support for bookmarks not in folders

Comment 2

[louis bennett]

*** Bug 257848 has been marked as a duplicate of this bug. ***

Comment 3

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

Again, is this something likely to get fixed for 1.0?

Comment 4

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

*** Bug 315842 has been marked as a duplicate of this bug. ***

Comment 5

[Håkan Waara]

Would this override default OS behavior? For instance, I have a Mighty Mouse that is set up to do Exposé on middle-click.

Comment 6

[Stuart Morgan]

It shouldn't. If you customize your mouse to trigger Exposé on middle-click, then no middle-click event is going to be generated. Presumably you aren't getting conflict between that setting and middle clicking on content area links; this would be no different.

Comment 7

[Håkan Waara]

Attachment: Proposed fix

Ok, so this patch fixes both this implements both "Middle click on bookmark item" (this bug) and "Middle click on bookmark folders" (bug 228839).

It generalizes the mouse event loop in DraggableImageAndTextCell to support NSOtherMouseButton.

It also implements an otherMouseButton: handler on BookmarkButton. It adheres to the pref whether to open a new window or new tab.

I've been testing it and it works nicely. Feel free to review!

Comment 8

[Stuart Morgan]

A few notes:
1) It doesn't actually fix bug 228839, since that's about middle-clicking on bookmark items in the menu you get from left-clicking on a bookmark bar.  It's not clear whether that's even possible to fix though.
2) Currently when command-clicking on bookmarks or a tab group the presence of shift to reverse the background/foreground default is ignored. That's therefore true of middle-click on bookmarks, tab groups, and folders with this patch.
3) Currently, when command-clicking a tab group and prefs set to open in new tabs, the new tabs are *always* focused, no matter what the background pref.

1 is a non-issue. 2 and 3 are problematic though... I'm not going to r- the patch, since they are pre-existing problems, but I think there should definitely be some thought given to fixing them as part of fixing this bug since it will make the feature more discoverable, and therefore the inconsistant handling higher profile. I would think that 2, at least, would be simple to fix in the central bookmark-opening logic.

(By the way, I don't think the patch needs to keep referring to the Mighty Mouse as an example--I think developers will know what the middle button is.)

Comment 9

[Håkan Waara]

I've thrown in a fix for #2 (reverse "focus new tab/window"-pref when shift is down) in this upcoming patch.

Investigating #3 (we ignore current "focus new tab/window"-pref for bookmark folders and bookmark groups), it's a regression from bug 294019. I have a proposed fix for that as well, and it's filed as spin off bug 320660 since it will need some additional QA testing.

New patch coming up.

Comment 10

[Håkan Waara]

Attachment: Patch v2

Ok, so same patch but this also makes us listen for shift before deciding whether to focus a new tab/window. 

If shift is held down, we should do the reverse of what is set in the pref.

Comment 11

[Håkan Waara]

Comment on attachment 206184 [details] [diff] [review]
Patch v2

Stuart Morgan, you interested in reviewing this?

Comment 12

[Stuart Morgan]

Comment on attachment 206184 [details] [diff] [review]
Patch v2

Some nits:

+  // These variables are by default setup to track the left mouse button. However, if another mouse button
+  // (for example, the middle button) is clicked, we need to track that instead.
+  unsigned int eventMask      = NSLeftMouseDraggedMask | NSLeftMouseUpMask;
+  BOOL isMiddleClick          = NO;
+  NSEventType mouseUp         = NSLeftMouseUp;
 
   if (![self startTrackingAt:curWindowLocation inView:controlView])
     return NO;
+    
+  // Override the default and track the third mouse button.
+  if ([theEvent type] == NSOtherMouseDown) 
+  {
+      eventMask     = NSOtherMouseDraggedMask | NSOtherMouseUpMask;
+      isMiddleClick = YES;
+      mouseUp       = NSOtherMouseUp;
+  }

First, don't interrupt this logic with the early return; put the return first. Second, why assume one set of values and then immediately change them for a middle click, rather than just setting them up correctly once?

Also, watch your indentation.
   
+    NSEvent* event = [NSApp nextEventMatchingMask:eventMask
+                            untilDate:clickHoldBailTime
+                            inMode:NSEventTrackingRunLoopMode
+                            dequeue:YES];

Obj-C style is to align at the ':'

+    if (([event type] == (isMiddleClick ? NSOtherMouseDragged : NSLeftMouseDragged)) &&


This isn't consistent with the your other events/masks (why not have a mouseDragged set at the beginning, and eliminate isMiddleClick?). More importantly though, this causes middle-click tracking to die if you move the mouse too much--since this is specifically for dragging, it should only apply to left-button.


Other than that, it works fine it my tests.  Mouse event tracking isn't my strong suit though, so I'd link smfr or pink to take a look at the overall approach. For example, I'm wondering if there are issues I missed in testing with the hold-timeout stuff, which like drag shouldn't applying to middle-click.

Comment 13

[Wevah (Nate Weaver)]

*** Bug 325858 has been marked as a duplicate of this bug. ***

Comment 14

[Håkan Waara]

Attachment: Patch v3

Better patch addressing Stuart's comments.

Comment 15

[Stuart Morgan]

Comment on attachment 212351 [details] [diff] [review]
Patch v3

Isn't this patch incomplete?  I assume the BookmarkButton.mm changes are still necessary.

-  else if (mLastClickHoldTimedOut && mClickHoldAction) {
+  else if (mLastClickHoldTimedOut && mClickHoldAction) 
+  {

While Camino is not even remotely consistent, I believe that the preferred style is, as with the rest of Mozilla, to put { on the same line as the if (unless there is a multi-line condition); unless I'm remembering wrong it would be better to change the other occurences to the same-line style, instead of doing this.

Comment 16

[Håkan Waara]

(In reply to comment #15)
> (From update of attachment 212351 [details] [diff] [review] [edit])
> Isn't this patch incomplete?  I assume the BookmarkButton.mm changes are still
> necessary.
> 
> -  else if (mLastClickHoldTimedOut && mClickHoldAction) {
> +  else if (mLastClickHoldTimedOut && mClickHoldAction) 
> +  {

Good catch - thanks, the other changes are equivalent of the last patch.

> 
> While Camino is not even remotely consistent, I believe that the preferred
> style is, as with the rest of Mozilla, to put { on the same line as the if
> (unless there is a multi-line condition); unless I'm remembering wrong it would
> be better to change the other occurences to the same-line style, instead of
> doing this.
> 

Actually, in Camino code the preferred style seems to almost always be:
if (...)
{
...
}

Anyway, this is consistent (at least in this file), and the only discrepancy was this one...

Comment 17

[Håkan Waara]

Comment on attachment 212351 [details] [diff] [review]
Patch v3

I've had this lingering in my tree forever, would be nice to check it in soon. :-)  Reviews? Pretty please?

Comment 18

[Stuart Morgan]

I'll try to get to this over the weekend.  It would help if you could respin the patch, though, so that I don't have to cobble it together from two files. Also, I checked with pink, and |if (...) {| is the preferred style.  Given that the file is already inconsistent, I'd much rather see a) the whole file fixed or b) just this function fixed than to see code changed away from the correct style.

Comment 19

[Håkan Waara]

Hm, ok, Smfr told me on IRC that all new code he wrote was:
if (...)
{
}

You want me to go ahead and remove all of that style anyway?

Comment 20

[Håkan Waara]

Attachment: Patch without the bracing change

Here's a patch without the bracing change. If wanted I can change the file to either style before checking in.

Comment 21

[Chris Lawson (gone)]

*** Bug 328501 has been marked as a duplicate of this bug. ***

Comment 22

[Stuart Morgan]

Comment on attachment 212351 [details] [diff] [review]
Patch v3

It sounds like pink and smfr need to have a throw-down about the bracing, so I'll stop worrying about that ;)

The patch still has behavioral issues though:
- If I middle-click-and-hold on a bookmark, then move the mouse off the bookmark, it doesn't unhighlight as is the normal behavior for buttons.  Worse, if I release the button while not over the bookmark, it still fires.
- If I middle-click-and-hold on a tab group, the menu comes up, but selecting an item opens it in the current tab. Either the menu shouldn't come up or (preferably) it should open the bookmark in a new tab/window.

Comment 23

[Stuart Morgan]

Comment on attachment 212351 [details] [diff] [review]
Patch v3

Whoops, wrong flag.

Comment 24

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

Håkan, are you still looking at this?

Comment 25

[Håkan Waara]

(In reply to comment #24)
> Håkan, are you still looking at this?

Not for a long time. Feel free to take if if you wish.

Comment 26

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

I'm going to put this back to "nobody" for now, since Håkan isn't working on it. Anyone who's interested should feel free to take it.

Comment 27

[Håkan Waara]

Attachment: WIP patch

This patch is almost done, but has one major issue left. Sometimes if I drag around a bookmark item, "ghost" drag image may stick on the toolbar (or in the content area, often if it's empty).

The patch does this:

* Implements middle-click on the bm toolbar and in DraggableImageCell
* Rewrite all tracking in DraggableImageCell to be simpler, and fix lots of bugs with it.

I also attempt to fix "toggling" (while the mousebutton is down) of cells. This works for middle-clicking items, but not for the back/forward button yet.

Comment 28

[Håkan Waara]

Mento looked at this with me, and the main problem seems to be that we're running nextEventMatchingMask: for only mouse events inside a while-loop.

This chokes gecko, and also suppresses native events which are probably the cause of weird drawing glitches.

Does anyone have any idea how to run a modal mouse tracking loop while letting the system (and gecko) still breathe? I wonder what Apple's default trackMouse: implementation does to solve this?

Comment 29

[Simon Fraser [no longer active]]

Is there a reason to remove the standard NSCell tracking methods (-startTrackingAt:inView: etc) and roll your own (buggy) event tracking loop?

Comment 30

[Håkan Waara]

(In reply to comment #29)
> Is there a reason to remove the standard NSCell tracking methods
> (-startTrackingAt:inView: etc) and roll your own (buggy) event tracking loop?
> 

Those methods were overridden by us, but either hardcoded or not very useful, so I don't see a need to override them.  

trackMouse: was always overridden by us to track click-holding, etc... you mean I could do it some other way?

Comment 31

[Simon Fraser [no longer active]]

(In reply to comment #30)

> Those methods were overridden by us, but either hardcoded or not very useful,
> so I don't see a need to override them.  

Those are the "standard" NSCell methods that you override to customize event handling in your cell class. if you ignore them, and instead just run your own event loop in trackMouse: you're potentially breaking NSControl event tracking logic. So it's best to use those methods to do whatever you need to, if possible.

Comment 32

[Håkan Waara]

Attachment: Simpler approach

Factoring all tracking code (click-hold, left click, dragging, middle click) into one tracking loop was seriously non-trivial, so here's a simpler approach which actually works fine.

* Implemented a trackOtherMouse: tracking loop, which only tracks other mouse events.
* Fixed so we *always* call the standard NSCell startTracking: method which we didn't before. This was a bug waiting to happen, because the drag system internals get confused if we don't call that.

The only non-trivial changes are in DraggableImageCell.mm

Comment 33

[Simon Fraser [no longer active]]

I still don't really like seeing our own event loops in those cell tracking methods (since NSControl should really be running those), but I guess they were there before. Hwaara, can you try to rewrite it without them?

Comment 34

[Håkan Waara]

(In reply to comment #33)
> I still don't really like seeing our own event loops in those cell tracking
> methods (since NSControl should really be running those), but I guess they were
> there before. Hwaara, can you try to rewrite it without them?

I think we need to poll for NSOtherMouse* mouse events, and do the timeout, to be able to implement click-hold and middle-click behaviors.

This is also how Apple suggests to do mouse tracking (http://developer.apple.com/documentation/Cocoa/Conceptual/BasicEventHandling/Tasks/HandlingMouseEvents.html), and I haven't been able to think of a better way...

Comment 35

[Håkan Waara]

Ping?

Comment 36

[Simon Fraser [no longer active]]

Sorry, swamped. Not gonna get to this for a while.

Comment 37

[Håkan Waara]

Comment on attachment 225184 [details] [diff] [review]
Simpler approach

Trying pink. (See comment 32 for summary of patch)

Comment 38

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

*** Bug 343743 has been marked as a duplicate of this bug. ***

Comment 39

[Mike Pinkerton (not reading bugmail)]

I'm confused, why not just listen for the otherMouseDown: event? Why do all this complicated tracking?  

Comment 40

[Håkan Waara]

(In reply to comment #39)
> I'm confused, why not just listen for the otherMouseDown: event? Why do all
> this complicated tracking?  

The otherMouseDown: event is called on the button when that button is pressed down. However, for the cell, we need to track the mouse (what if the 3rd mouse button is clicked, and move out of the click rect?)

So, we have this custom mouse tracking stuff. We use that for all bookmark items in the toolbar to allow dragging, and click-holding. The only thing I'm doing is really to expand it to support otherMouse tracking as well.  Makes sense?

Comment 41

[Mike Pinkerton (not reading bugmail)]

sorry hwaara, i so don't have time to review this :( can you get someone else on #camino to take a look? I don't want to be the one blocking you.

Comment 42

[Håkan Waara]

Stuart, would you mind having a glance (at the general approach, if a line-by-line review is too much, which I presume it would be)? 

Comment 43

[Chris Lawson (gone)]

Can we get this un-bitrotted and reviewed again soon-ish? This really needs to make 1.1.

cl

Comment 44

[Håkan Waara]

(In reply to comment #43)
> Can we get this un-bitrotted and reviewed again soon-ish? This really needs to
> make 1.1.

The code changes remain the same, so there's no need to make a new patch unless someone wants to test it.

Stuart, are you intending to review this? 

This bug really sucks for all Mighty Mouse-users. (You clickly get used to the power & comfort of the middle-click!)

Comment 45

[Stuart Morgan]

(In reply to comment #44)
> Stuart, are you intending to review this? 

I am, I just haven't had a big enough block of time recently.

Comment 46

[Stuart Morgan]

I'm having a hard time following parts of this out of context, and it's undergone too much bitrot to apply; could you roll a new version?

Comment 47

[Stuart Morgan]

Comment on attachment 225184 [details] [diff] [review]
Simpler approach

Clearing review flag on the stale patch.

Comment 48

[froodian (Ian Leue)]

Attachment: This is the bitrot police

Comment 49

[Stuart Morgan]

Attachment: newly un-rotted

I had to manually un-rot this again, so posting for the benefit of those who follow after.

So this mostly looks good, but I'm confused by the fact that otherMouseDown: forwards to mouseDown:, which eventually goes to a tracking routine that mostly is a big if/else on the button. Is it not possible to have two separate parallel method chains, one for left button and one for middle button, rather than intermingling them then unentangling them?

Comment 50

[Håkan Waara]

(In reply to comment #49)
> So this mostly looks good, but I'm confused by the fact that otherMouseDown:
> forwards to mouseDown:, which eventually goes to a tracking routine that mostly
> is a big if/else on the button. Is it not possible to have two separate
> parallel method chains, one for left button and one for middle button, rather
> than intermingling them then unentangling them?

Sounds like a workable idea to me. I won't have time to work on this though...

Comment 51

[Chris Lawson (gone)]

Pushing to 1.2, since Håkan isn't going to have time to look at this before 1.1.

Comment 53

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

Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.

Comment 55

[dan pritts]

just a ping on this bug - i have wanted this since i first started using camino and finally checked the bugzilla for it, surprised to see the ticket has been alive so long (but hey, i know how it goes).  

I don't have any mac coding expertise so i can't help, but if anyone seeing this has time, know that at least one user would appreciate it.

thanks for all your work.

Comment 56

[Stuart Morgan]

Please use the vote feature, rather than comments, to express interest in seeing a bug fixed.

Comment 57

[Christopher Henderson]

Attachment: Separate method chains

Might as well take this bug, as I've got a working patch for it. There might be some improvements I can make to it, but it works as is, so maybe not.

Most of the work is done in the trackMouse/trackOtherMouse methods, which AFAICT isn't how the standard Cocoa widgets do it. That said, it may be non-trivial to redo the handling and put the logic in the mouseDown/otherMouseDown methods of the Button Cell. This way works, and the logic is split up for the two buttons, so I'm happy with it.

The highlight method had to be redone, as the text would disappear when dragging the cursor outside of the button's bounds with the middle mouse button. The old patch wouldn't update the highlight status at all in this case; this patch does.

Comment 58

[Stuart Morgan]

Comment on attachment 346416 [details] [diff] [review]
Separate method chains

Behaviorally, looks great! Somee code changes:

>+- (BOOL)trackOtherMouse:(NSEvent*)event inRect:(NSRect)cellFrame ofView:(NSView*)controlView untilMouseUp:(BOOL)untilMouseUp;

Break this into four colon-aligned lines (here and at the definition).

> - (id)initTextCell:(NSString*)aString
> {
>   if ((self = [super initTextCell:aString])) {
>     mIsDraggable = NO;
>     mClickHoldTimeoutSeconds = 60.0 * 60.0 * 24.0;
>     mLastClickHoldTimedOut = NO;
>+    mHasShadow = NO;

Don't add this; mIsDraggable and mLastClickHoldTimedOut shouldn't actually be here, since all members are initialized to nil/NULL/NO/0.

>+    if (highlighted) {      
[...]
>+    else if (mHasShadow){

Don't add trailing whitespace to lines. Shouldn't this be |if (highlighted && !hasShadow) {} else if (!highlighted && hasShadow) {}|?

>-  NSPoint firstWindowLocation = [theEvent locationInWindow];
>-  NSPoint lastWindowLocation  = firstWindowLocation;
>-  NSPoint curWindowLocation   = firstWindowLocation;
>+  NSPoint firstMouseLocation = [theEvent locationInWindow];
>+  NSPoint lastMouseLocation  = firstMouseLocation;
>+  NSPoint currentMouseLocation   = firstMouseLocation;

Why the change? Given the context, the risk of confusion seems low enough that the added precision about the coordinate space is probably worthwhile.

>+- (BOOL)trackOtherMouse:(NSEvent*)theEvent inRect:(NSRect)cellFrame ofView:(NSView*)controlView untilMouseUp:(BOOL)untilMouseUp

Unnecessary logic was copied into this method:

>+  NSPoint firstMouseLocation = [theEvent locationInWindow];

You don't need this variable.

>+  NSEventType lastEvent       = (NSEventType)0;

You don't need this either; lastEvent will always be NSOtherMouseUp

>+  return YES;  // XXX fix me

Isn't the correct return value just |untilMouseUp || mouseInsideCell| ?

>+  [mLastMouseDownEvent release];
>+  mLastMouseDownEvent = [aEvent retain];

mLastMouseDownEvent is only used for dragging, so you don't need to set it here.

>+  mLastEventWasMenu = NO;

Same with mLastEventWasMenu.

Comment 59

[Christopher Henderson]

Attachment: Fixed

Patch incorporating fixes for all the comments from the review. Highlighting is now much more efficient.

Comment 60

[Stuart Morgan]

Comment on attachment 347735 [details] [diff] [review]
Fixed

Looks good! r=me

Comment 61

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 347735 [details] [diff] [review]
Fixed

sr=pink

Comment 62

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

Landed on cvs trunk.