Closed
Bug 337958
Opened 19 years ago
Closed 18 years ago
Dragging folder/tab group from Bookmark Bar to tab strip should respect shift toggle
Categories
(Camino Graveyard :: Tabbed Browsing, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino1.5
People
(Reporter: alqahira, Assigned: bugzilla-graveyard)
References
Details
(Keywords: fixed1.8.1.1, polish)
Attachments
(1 file, 5 obsolete files)
12.20 KB,
patch
|
hwaara
:
review+
mikepinkerton
:
superreview+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
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)
Reporter | ||
Comment 2•19 years ago
|
||
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•19 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•19 years ago
|
||
How's this?
Attachment #222007 -
Attachment is obsolete: true
Attachment #223522 -
Flags: review?(stuart.morgan)
Comment 5•19 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•19 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•19 years ago
|
||
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•18 years ago
|
Status: NEW → ASSIGNED
Comment 8•18 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•18 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•18 years ago
|
||
CCing Mike, Simon, and Mark per comment 8.
cl
Comment 11•18 years ago
|
||
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.
Reporter | ||
Updated•18 years ago
|
Target Milestone: --- → Camino1.1
Comment 12•18 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.
Reporter | ||
Comment 13•18 years ago
|
||
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•18 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•18 years ago
|
||
Something like this, then?
cl
Attachment #223559 -
Attachment is obsolete: true
Attachment #241283 -
Flags: review?(stuart.morgan)
Assignee | ||
Comment 16•18 years ago
|
||
BTW, the above patch was -uw. I'll have one without the -w flag for checkin.
cl
Comment 17•18 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•18 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•18 years ago
|
||
No more ignoreShift parameter, and no changes to handleCommandReturn: either.
Attachment #241283 -
Attachment is obsolete: true
Attachment #242145 -
Flags: review?(stuart.morgan)
Comment 20•18 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•18 years ago
|
||
OK, I think this should do it.
Attachment #242145 -
Attachment is obsolete: true
Attachment #242371 -
Flags: review?(stuart.morgan)
Comment 22•18 years ago
|
||
Comment on attachment 242371 [details] [diff] [review]
whew
Looks very clean to me!
Comment 23•18 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+
Reporter | ||
Updated•18 years ago
|
Attachment #242371 -
Flags: superreview?(mikepinkerton)
Reporter | ||
Comment 24•18 years ago
|
||
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•18 years ago
|
||
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+
Reporter | ||
Updated•18 years ago
|
Whiteboard: [needs checkin]
Comment 26•18 years ago
|
||
Checked in on 1.8branch and trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 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.
Description
•