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

RESOLVED FIXED in Camino2.0

Status

--
enhancement
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: jdance, Assigned: chris)

Tracking

unspecified
Camino2.0
PowerPC
Mac OS X
Bug Flags:
camino1.5 -

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
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
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → Camino1.0

Comment 2

14 years ago
*** Bug 257848 has been marked as a duplicate of this bug. ***
Again, is this something likely to get fixed for 1.0?
Target Milestone: Camino1.0 → Camino1.1
*** Bug 315842 has been marked as a duplicate of this bug. ***

Comment 5

13 years ago
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

13 years ago
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

13 years ago
Created attachment 206134 [details] [diff] [review]
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!
Attachment #206134 - Flags: superreview?
Attachment #206134 - Flags: review?

Comment 8

13 years ago
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

13 years ago
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

13 years ago
Created attachment 206184 [details] [diff] [review]
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.
Assignee: mikepinkerton → hwaara
Attachment #206134 - Attachment is obsolete: true
Attachment #206184 - Flags: superreview?
Attachment #206184 - Flags: review?(bugzilla)
Attachment #206134 - Flags: superreview?
Attachment #206134 - Flags: review?

Updated

13 years ago
Attachment #206184 - Flags: review?(bugzilla) → review?(mikepinkerton)

Comment 11

13 years ago
Comment on attachment 206184 [details] [diff] [review]
Patch v2

Stuart Morgan, you interested in reviewing this?
Attachment #206184 - Flags: review?(mikepinkerton) → review?(stuart.morgan)

Comment 12

13 years ago
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.
Attachment #206184 - Flags: superreview?
Attachment #206184 - Flags: review?(stuart.morgan)
Attachment #206184 - Flags: review-
*** Bug 325858 has been marked as a duplicate of this bug. ***

Comment 14

13 years ago
Created attachment 212351 [details] [diff] [review]
Patch v3

Better patch addressing Stuart's comments.
Attachment #206184 - Attachment is obsolete: true
Attachment #212351 - Flags: superreview?(sfraser_bugs)
Attachment #212351 - Flags: review?(stuart.morgan)

Comment 15

13 years ago
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

13 years ago
(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

13 years ago
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

13 years ago
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

13 years ago
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

13 years ago
Created attachment 213035 [details] [diff] [review]
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.
*** Bug 328501 has been marked as a duplicate of this bug. ***

Comment 22

13 years ago
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.
Attachment #212351 - Flags: superreview?(sfraser_bugs)
Attachment #212351 - Flags: superreview-
Attachment #212351 - Flags: review?(stuart.morgan)

Comment 23

13 years ago
Comment on attachment 212351 [details] [diff] [review]
Patch v3

Whoops, wrong flag.
Attachment #212351 - Flags: superreview- → review-

Updated

13 years ago
Blocks: 228839

Updated

13 years ago
Attachment #213035 - Attachment is obsolete: true
Håkan, are you still looking at this?

Comment 25

13 years ago
(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.
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.
Assignee: hwaara → nobody
Status: ASSIGNED → NEW
QA Contact: toolbars

Updated

13 years ago
Assignee: nobody → hwaara

Comment 27

13 years ago
Created attachment 224910 [details] [diff] [review]
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.
Attachment #212351 - Attachment is obsolete: true

Comment 28

13 years ago
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

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

Comment 30

13 years ago
(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

13 years ago
(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

13 years ago
Created attachment 225184 [details] [diff] [review]
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
Attachment #225184 - Flags: review?(sfraser_bugs)

Comment 33

13 years ago
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

13 years ago
(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

13 years ago
Ping?

Comment 36

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

Comment 37

13 years ago
Comment on attachment 225184 [details] [diff] [review]
Simpler approach

Trying pink. (See comment 32 for summary of patch)
Attachment #225184 - Flags: review?(sfraser_bugs) → review?(mikepinkerton)
*** Bug 343743 has been marked as a duplicate of this bug. ***
I'm confused, why not just listen for the otherMouseDown: event? Why do all this complicated tracking?  

Comment 40

13 years ago
(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?
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

13 years ago
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)? 

Updated

13 years ago
Attachment #225184 - Flags: review?(mikepinkerton) → review?(stuart.morgan)
Can we get this un-bitrotted and reviewed again soon-ish? This really needs to make 1.1.

cl
Flags: camino1.1?

Comment 44

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

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

12 years ago
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

12 years ago
Comment on attachment 225184 [details] [diff] [review]
Simpler approach

Clearing review flag on the stale patch.
Attachment #225184 - Flags: review?(stuart.morgan)

Updated

12 years ago
Attachment #225184 - Flags: review?(stuart.morgan)
Created attachment 245007 [details] [diff] [review]
This is the bitrot police
Attachment #224910 - Attachment is obsolete: true
Attachment #225184 - Attachment is obsolete: true
Attachment #245007 - Flags: review?(stuart.morgan)
Attachment #225184 - Flags: review?(stuart.morgan)

Comment 49

12 years ago
Created attachment 245909 [details] [diff] [review]
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?
Attachment #245007 - Attachment is obsolete: true
Attachment #245909 - Flags: review?(stuart.morgan)
Attachment #245007 - Flags: review?(stuart.morgan)

Comment 50

12 years ago
(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...
Assignee: hwaara → nobody
Pushing to 1.2, since Håkan isn't going to have time to look at this before 1.1.
Flags: camino1.1? → camino1.1-
Target Milestone: Camino1.1 → Camino1.2

Updated

12 years ago
Duplicate of this bug: 372341
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*.
Target Milestone: Camino1.6 → ---
Summary: Bookmarks toolbar - Middle-click should open bookmark in new tab → [patch] Bookmarks toolbar - Middle-click should open bookmark in new tab

Updated

11 years ago
Duplicate of this bug: 408977

Comment 55

10 years ago
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

10 years ago
Please use the vote feature, rather than comments, to express interest in seeing a bug fixed.
(Assignee)

Comment 57

10 years ago
Created attachment 346416 [details] [diff] [review]
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.
Assignee: nobody → trendyhendy2000
Attachment #245909 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #346416 - Flags: review?
Attachment #245909 - Flags: review?(stuart.morgan+bugzilla)
(Assignee)

Updated

10 years ago
Attachment #346416 - Flags: review? → review?(stuart.morgan+bugzilla)

Updated

10 years ago
Attachment #346416 - Flags: review?(stuart.morgan+bugzilla) → review-

Comment 58

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

Comment 59

10 years ago
Created attachment 347735 [details] [diff] [review]
Fixed

Patch incorporating fixes for all the comments from the review. Highlighting is now much more efficient.
Attachment #346416 - Attachment is obsolete: true
Attachment #347735 - Flags: review?(stuart.morgan+bugzilla)

Updated

10 years ago
Attachment #347735 - Flags: review?(stuart.morgan+bugzilla) → review+

Comment 60

10 years ago
Comment on attachment 347735 [details] [diff] [review]
Fixed

Looks good! r=me

Updated

10 years ago
Attachment #347735 - Flags: superreview?(mikepinkerton)
Comment on attachment 347735 [details] [diff] [review]
Fixed

sr=pink
Attachment #347735 - Flags: superreview?(mikepinkerton) → superreview+
Landed on cvs trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Target Milestone: --- → Camino2.0
You need to log in before you can comment on or make changes to this bug.