Last Comment Bug 337958 - Dragging folder/tab group from Bookmark Bar to tab strip should respect shift toggle
: Dragging folder/tab group from Bookmark Bar to tab strip should respect shift...
Status: RESOLVED FIXED
: fixed1.8.1.1, polish
Product: Camino Graveyard
Classification: Graveyard
Component: Tabbed Browsing (show other bugs)
: unspecified
: PowerPC Mac OS X
-- normal (vote)
: Camino1.5
Assigned To: Chris Lawson (gone)
:
:
Mentors:
Depends on:
Blocks: 183401 320668 331670
  Show dependency treegraph
 
Reported: 2006-05-14 22:28 PDT by Smokey Ardisson (offline for a while; not following bugs - do not email)
Modified: 2006-10-27 14:58 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix (2.98 KB, patch)
2006-05-14 22:32 PDT, Chris Lawson (gone)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
shared implementation (8.01 KB, patch)
2006-05-26 20:32 PDT, Chris Lawson (gone)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
fixed (26.36 KB, patch)
2006-05-27 12:42 PDT, Chris Lawson (gone)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
uses renamed class method (13.29 KB, patch)
2006-10-04 21:08 PDT, Chris Lawson (gone)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
no more ignoreShift parameter (11.13 KB, patch)
2006-10-12 21:34 PDT, Chris Lawson (gone)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
whew (12.20 KB, patch)
2006-10-15 20:12 PDT, Chris Lawson (gone)
hwaara: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-14 22:28:40 PDT
Bug 320668 will get dragging indiv. bookmarks to respect not only the "Load in background" pref but also the toggle.  Folders/tab groups already respect the pref, but they do not honor the toggle.
Comment 1 User image Chris Lawson (gone) 2006-05-14 22:32:37 PDT
Created attachment 222007 [details] [diff] [review]
fix

Also fixes some whitespace goofs in the file. The meat of the patch is the last 15 lines or so.

cl
Comment 2 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-14 22:41:01 PDT
Comment on attachment 222007 [details] [diff] [review]
fix

This also works as expected, but I didn't test extensively for possible side-effects.  r=me, FWIW
Comment 3 User image Stuart Morgan 2006-05-16 21:38:32 PDT
Comment on attachment 222007 [details] [diff] [review]
fix

>-  int numItems 		 = (int)[urlArray count];
>+  int numItems 	       = (int)[urlArray count];

Let's go crazy here, and remove both tabs from this line.

>   for (int i = 0; i < numItems; i++)
>   {
...
>-    else
>-    {
>+    else {

Why doesn't the |for| get any love?

>   BOOL loadInBackground = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL];
>-  
>+
>+  // respect the standard shift toggle
>+  if ([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask)
>+    loadInBackground = !loadInBackground;
>+

This block of code is starting to crop up in more and more places.  I think it's time for a method to encapsulate it and cut down on the wear and tear on copy and paste shortcut keys. Something like
(BOOL)shouldLoadInBackgroundIgnoringShift:(BOOL)ignoreShift
(That's a tad long, but I'm having trouble shortening it without sacrificing clarity.)
Comment 4 User image Chris Lawson (gone) 2006-05-26 20:32:16 PDT
Created attachment 223522 [details] [diff] [review]
shared implementation

How's this?
Comment 5 User image Stuart Morgan 2006-05-27 00:09:38 PDT
Comment on attachment 223522 [details] [diff] [review]
shared implementation

What about all the other places currently using getBooleanPref:"browser.tabs.loadInBackground"?  LXR shows around 15, and this only changes 2.

>-      [[result itemWithTarget:self andAction:@selector(copyAddressToClipboard:)] setTitle:NSLocalizedString(@"Copy Addresses", @"")];
>+      [[result itemAtIndex:[result indexOfItemWithTarget:self andAction:@selector(copyAddressToClipboard:)]] setTitle:NSLocalizedString(@"Copy Addresses", @"")];

Why are you obfuscating this?

>+// -shouldLoadInBackground:ignoringShift

Why is the colon in the wrong place?  That's not the name of the method.

>+// uses shift key during event to toggle foreground/background tab loading
...
>+  // respect the standard shift toggle

This method is very short; no need to explain the shift key thing twice.


>+// Look for the shift key and use it to toggle the load-in-background pref
>+- (BOOL)shouldLoadInBackgroundIgnoringShift:(BOOL)ignoreShift;

The primary purpose of this method is to get the pref, not to look for the shift key. The summary should be something more like "Get the load-in-background pref, optionally reversing it based on the shift key"
Comment 6 User image Chris Lawson (gone) 2006-05-27 08:36:20 PDT
(In reply to comment #5)
> (From update of attachment 223522 [details] [diff] [review] [edit])
> What about all the other places currently using
> getBooleanPref:"browser.tabs.loadInBackground"?  LXR shows around 15, and this
> only changes 2.

I hadn't thought of them? :-p

> >-      [[result itemWithTarget:self andAction:@selector(copyAddressToClipboard:)] setTitle:NSLocalizedString(@"Copy Addresses", @"")];
> >+      [[result itemAtIndex:[result indexOfItemWithTarget:self andAction:@selector(copyAddressToClipboard:)]] setTitle:NSLocalizedString(@"Copy Addresses", @"")];
> 
> Why are you obfuscating this?

Required to fix a compiler warning, unless someone has a better idea.

> >+// -shouldLoadInBackground:ignoringShift
> 
> Why is the colon in the wrong place?  That's not the name of the method.

Because I'm an idiot? :)

I'll fix the comments too.

cl
Comment 7 User image Chris Lawson (gone) 2006-05-27 12:42:50 PDT
Created attachment 223559 [details] [diff] [review]
fixed

Fixes comments above, ignores the protocol warning mentioned in the last comment, and touches all files formerly using the PreferenceManager call directly.

Also fixes several miscellaneous compiler warnings, mostly due to missing casts, in some other files.

cl
Comment 8 User image Stuart Morgan 2006-06-05 23:31:42 PDT
All the call site changes look good from a desired behavior standpoint.  However, 
I'm not convinced that BWC is the right place for the new method, since it has nothing BWC-specific about it (and at the very least it seems like it should be a class method of where ever it ends up).  I'm not sure where it fits best--maybe PreferenceManager?  See what one of the sr folks think.

(Also for the next round: GoMenu.mm, and possibly others since I didn't check them all, have consistent bracing style--do not change just one function in a file like that. If you want to change a file like that, file a cleanup bug with a whole-file-at-once patch.)
Comment 9 User image Stuart Morgan 2006-06-06 00:05:26 PDT
Comment on attachment 223559 [details] [diff] [review]
fixed

(Forgot the r-; sorry for bugspam)
Comment 10 User image Chris Lawson (gone) 2006-06-06 13:13:07 PDT
CCing Mike, Simon, and Mark per comment 8.

cl
Comment 11 User image Mike Pinkerton (not reading bugmail) 2006-07-14 10:11:29 PDT
i don't think the -shouldLoadInBackground... method should go on the pref mgr, it really is a browser-ish thing and the prefMgr doesn't have much specific to the browser (correct me if i'm wrong). I do agree it makes sense as a class method since it doesn't use any instance vars, or it could even be in a new namespace with other functions that do similar things. 

i hesitate to say it might be a a candidate for a category on PrefMgr, but if PrefMgr were apple's we'd probably go that route. 
Comment 12 User image Stuart Morgan 2006-09-17 08:21:38 PDT
Lets just get a version of this patch with the function as a class method on BWC.  We need to get the bugs that depend on this moving.
Comment 13 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-10-03 22:59:55 PDT
cl, any ETA on getting a patch in the style of comment 17 ready?  This really is blocking new/toggle fixup work on a number of bugs.
Comment 14 User image Håkan Waara 2006-10-04 16:27:11 PDT
Comment on attachment 223559 [details] [diff] [review]
fixed

Here are some very late comments on this patch. Just saw the bug ref'd from the meeting log.

* Please name it |shouldLoadInBackground| and skip the "IgnoringShift" suffix. (We can add a new version if it's ever gonna be much needed.)

Making it a class method as previously suggested also sounds good.
Comment 15 User image Chris Lawson (gone) 2006-10-04 21:08:15 PDT
Created attachment 241283 [details] [diff] [review]
uses renamed class method

Something like this, then?

cl
Comment 16 User image Chris Lawson (gone) 2006-10-04 21:08:53 PDT
BTW, the above patch was -uw. I'll have one without the -w flag for checkin.

cl
Comment 17 User image Håkan Waara 2006-10-05 01:54:41 PDT
Comment on attachment 241283 [details] [diff] [review]
uses renamed class method

Sorry, what I meant in the previous comment was to use |shouldLoadInBackground| as name *and* not have the ignoreShift parameter at all, unless we plan to use it now.

Isn't it simpler to leave handleCommandReturn like it is with its current param, and skip it for all shouldLoadInBackground callers (since all of them pass NO anyway).
Comment 18 User image Stuart Morgan 2006-10-09 21:53:50 PDT
Comment on attachment 241283 [details] [diff] [review]
uses renamed class method

Yeah, axe the ignoreShift parameter.  I thought there was a call site that would need it back when I suggested it, but apparently not.
Comment 19 User image Chris Lawson (gone) 2006-10-12 21:34:32 PDT
Created attachment 242145 [details] [diff] [review]
no more ignoreShift parameter

No more ignoreShift parameter, and no changes to handleCommandReturn: either.
Comment 20 User image Stuart Morgan 2006-10-14 18:38:04 PDT
Comment on attachment 242145 [details] [diff] [review]
no more ignoreShift parameter

>Index: src/browser/ContentClickListener.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/ContentClickListener.mm,v
>retrieving revision 1.28
>diff -u -w -r1.28 ContentClickListener.mm
>--- src/browser/ContentClickListener.mm	23 Jun 2006 03:10:23 -0000	1.28
>+++ src/browser/ContentClickListener.mm	5 Oct 2006 03:58:35 -0000
>@@ -100,7 +100,7 @@
>   if ((metaKey && button == 0) || button == 1) {
>     // The command key is down or we got a middle click.  Open the link in a new window or tab.
>     BOOL useTab           = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL];
>-    BOOL loadInBackground = [[PreferenceManager sharedInstance] getBooleanPref:"browser.tabs.loadInBackground" withSuccess:NULL];
>+    BOOL loadInBackground = [BrowserWindowController shouldLoadInBackground];

Making this change without removing the following section that toggles it with the shift key breaks the use of shift in the content area.


There's also a lot of style problems in BWC beyond what you fix, so I'll be doing a large BWC cleanup very soon either way; I'd prefer all the cleanup be done in one cleanup patch rather than tacking parts of it onto this change.
Comment 21 User image Chris Lawson (gone) 2006-10-15 20:12:38 PDT
Created attachment 242371 [details] [diff] [review]
whew

OK, I think this should do it.
Comment 22 User image Håkan Waara 2006-10-17 06:31:56 PDT
Comment on attachment 242371 [details] [diff] [review]
whew

Looks very clean to me!
Comment 23 User image Håkan Waara 2006-10-17 09:46:26 PDT
Comment on attachment 242371 [details] [diff] [review]
whew

It looks so good I have to steal it from smorgan.
Comment 24 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-10-27 00:13:33 PDT
Mike, any idea when you can get to this--or can smorgan take a stab at sr?  This has been sitting for a week or so now, and it's blocking a number of other bugs....
Comment 25 User image Mike Pinkerton (not reading bugmail) 2006-10-27 06:59:24 PDT
Comment on attachment 242371 [details] [diff] [review]
whew

sr=pink

when we clean up BWC, sLIB should probably live elsewhere, but i guess it's ok where it is now.
Comment 26 User image froodian (Ian Leue) 2006-10-27 14:58:54 PDT
Checked in on 1.8branch and trunk.

Note You need to log in before you can comment on or make changes to this bug.