The default bug view has changed. See this FAQ.

Dragging folder/tab group from Bookmark Bar to tab strip should respect shift toggle

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Tabbed Browsing
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Chris Lawson (gone))

Tracking

({fixed1.8.1.1, polish})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1, polish
Dependency tree / graph

Details

Attachments

(1 attachment, 5 obsolete attachments)

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

Comment 1

11 years ago
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
Attachment #222007 - Flags: review?(stuart.morgan)
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

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

Comment 4

11 years ago
Created attachment 223522 [details] [diff] [review]
shared implementation

How's this?
Attachment #222007 - Attachment is obsolete: true
Attachment #223522 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
Blocks: 320668
(Assignee)

Updated

11 years ago
Blocks: 331670
(Assignee)

Updated

11 years ago
Blocks: 183401

Comment 5

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

Comment 6

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

Comment 7

11 years ago
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
Attachment #223522 - Attachment is obsolete: true
Attachment #223559 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED

Comment 8

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

11 years ago
Comment on attachment 223559 [details] [diff] [review]
fixed

(Forgot the r-; sorry for bugspam)
Attachment #223559 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 10

11 years ago
CCing Mike, Simon, and Mark per comment 8.

cl
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. 
Target Milestone: --- → Camino1.1

Comment 12

11 years ago
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.
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

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

Comment 15

11 years ago
Created attachment 241283 [details] [diff] [review]
uses renamed class method

Something like this, then?

cl
Attachment #223559 - Attachment is obsolete: true
Attachment #241283 - Flags: review?(stuart.morgan)
(Assignee)

Comment 16

11 years ago
BTW, the above patch was -uw. I'll have one without the -w flag for checkin.

cl

Comment 17

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

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

Comment 19

11 years ago
Created attachment 242145 [details] [diff] [review]
no more ignoreShift parameter

No more ignoreShift parameter, and no changes to handleCommandReturn: either.
Attachment #241283 - Attachment is obsolete: true
Attachment #242145 - Flags: review?(stuart.morgan)

Comment 20

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

Comment 21

11 years ago
Created attachment 242371 [details] [diff] [review]
whew

OK, I think this should do it.
Attachment #242145 - Attachment is obsolete: true
Attachment #242371 - Flags: review?(stuart.morgan)

Comment 22

11 years ago
Comment on attachment 242371 [details] [diff] [review]
whew

Looks very clean to me!

Comment 23

11 years ago
Comment on attachment 242371 [details] [diff] [review]
whew

It looks so good I have to steal it from smorgan.
Attachment #242371 - Flags: review?(stuart.morgan) → review+
Attachment #242371 - Flags: superreview?(mikepinkerton)
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 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.
Attachment #242371 - Flags: superreview?(mikepinkerton) → superreview+
Whiteboard: [needs checkin]

Comment 26

11 years ago
Checked in on 1.8branch and trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
You need to log in before you can comment on or make changes to this bug.