Closed
Bug 324591
Opened 18 years ago
Closed 18 years ago
Allow me to drop URLs in the tab strip and get a new tab at the drop site (instead of replacing an existing tab)
Categories
(SeaMonkey :: UI Design, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey1.1alpha
People
(Reporter: csthomas, Assigned: csthomas)
References
Details
(Keywords: fixed-seamonkey1.1a, relnote)
Attachments
(2 files, 9 obsolete files)
9.21 KB,
patch
|
jag+mozilla
:
review+
neil
:
superreview+
neil
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
2.70 KB,
patch
|
neil
:
review+
neil
:
superreview+
neil
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
See firefox bug 320638 It would be nice to be able to drop URLs between tabs to open a new tab at that location. [very crude] patch in a minute...
Assignee | ||
Comment 1•18 years ago
|
||
No RTL support yet. I suspect I could use my "getDropIndex2" in onDragOver to make that code simpler. It works, though.
Assignee | ||
Comment 2•18 years ago
|
||
(same thing, -w)
Assignee | ||
Comment 3•18 years ago
|
||
This also covers firefox bug 320639.
Assignee | ||
Comment 4•18 years ago
|
||
original summary was confusing ;)
Status: NEW → ASSIGNED
Summary: allow D&D in between tabs → Allow me to drop URLs in the tab strip and get a new tab at the drop site (instead of replacing an existing tab)
Assignee | ||
Comment 5•18 years ago
|
||
I messed up the RTL code, but I have a hard time figuring it out, so someone will have to help me with that. Having two functions for drop indexes is probably not the best way to do this... I thought about returning values like "3.5" to indicate whether it's at a tab or between tabs, but that doesn't lead to pretty code. The current method does make the code pretty simple. Maybe it would be ok with a better name than "getDropIndex2".
Attachment #209537 -
Attachment is obsolete: true
Attachment #209538 -
Attachment is obsolete: true
Attachment #209542 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209542 -
Flags: review?(jag)
Assignee | ||
Comment 6•18 years ago
|
||
Comment 7•18 years ago
|
||
When I saw bug 320638 I started thinking about how to fix this in SeaMonkey. I followed the same path. Use .5 to indicate in between, but that seemed like it'd get messy, so then I thought maybe getTabDropIndex() and getURLDropIndex(), which is pretty much what you have. Great work, few nits, e.g. s/\* .5/\/ 2/, a better name for the second method, as you already pointed out, and I'd probably replace |if (...) a = true; else a = false;| with |a = ...;|. And if we can make the ltr/rtl hit point detection and indicator placement code easier to read, that'd be a nice bonus. However: The other solution I was thinking about was to add a second parameter to getDropIndex(), e.g. allowOnTab, and return a small object with .index and .between (boolean), where |between| can only be false if |allowOnTab| is true. That would let us walk the tabs only once. The only thing I'd have done differently (sorry, no code to show for the above thought process, you beat me to it before I started writing) is when dragging a URL I would leave the "on tab" drag feedback the same as it is now instead of moving the arrow to the middle of the target tab.
Assignee | ||
Comment 8•18 years ago
|
||
(In reply to comment #7) > URL I would leave the "on tab" drag feedback the same as it is now instead of > moving the arrow to the middle of the target tab. How would the user distinguish between drops that are on tabs and drops that are between tabs? With my patch you still have the option of replacing a tab if you're dropping a URL (i.e., if the indicator shows up between tabs, you get a new tab there, and if the indicator shows up on a tab, that tab gets replaced). The bug summary doesn't mention this, because it's long already and I couldn't come up with a concise wording :). Or did I misunderstand what you mean?
Comment 9•18 years ago
|
||
For URL drops in between tabs we'd show the arrow, as you suggested, but drops on top of tabs would either give no feedback (other than the mouse cursor) like we currently do, or maybe some kind of drop ring, like we show when dropping on bookmark folders / bookmark groups.
Comment 10•18 years ago
|
||
Comment on attachment 209542 [details] [diff] [review] the patch, cleaned up >+ var betweenOnly; > if (aDragSession.sourceNode && >- aDragSession.sourceNode.parentNode == this.mTabContainer) { >- var ib = document.getAnonymousElementByAttribute(this, "class", "tab-drop-indicator-bar"); >+ aDragSession.sourceNode.parentNode == this.mTabContainer) >+ betweenOnly = true; >+ else >+ betweenOnly = false; > >- if (!aDragSession.canDrop) >- { >- ib.hidden = true; >- return; >- } Here I'd probably not show the indicator for a drop on. But if you do, I'd code it as var dropOnIndex = -1; if (canDropOn) dropOnIndex = this.getDropOnIndex(event); ... if (dropOnIndex != -1) showIndicatorDropOn(); >- // We're adding a new tab. >- // Load in an existing tab. You lost these comments. Maybe reverse your test so that they don't need to be swapped? >+ <method name="getDropIndex2"> getDropOnIndex? >+ if (window.getComputedStyle(this, null).direction == "ltr") { No rtl issues here, your drop area is the centre of the tab. >+ return null; I'd prefer -1 so that the return value is always an integer.
Attachment #209542 -
Flags: superreview?(neil) → superreview-
Assignee | ||
Comment 11•18 years ago
|
||
(In reply to comment #10) > (From update of attachment 209542 [details] [diff] [review] [edit]) > >+ var betweenOnly; > Here I'd probably not show the indicator for a drop on. But if you do, I'd code > it as var dropOnIndex = -1; if (canDropOn) dropOnIndex = > this.getDropOnIndex(event); ... if (dropOnIndex != -1) showIndicatorDropOn(); Why break it out into a separate function? If we do that, shouldn't we have showIndicatorDropOn() and showIndicatorDropBetween() both as functions? Also, I think some indication that the drop is going onto a tab is useful - suggested alternatives to the arrow are welcome (though I think the arrow works pretty well). > >- // We're adding a new tab. > >- // Load in an existing tab. > You lost these comments. Maybe reverse your test so that they don't need to be > swapped? Added comments. I slightly prefer this order, let me know if you'd prefer flipping it. > >+ <method name="getDropIndex2"> > getDropOnIndex? Fixed > >+ if (window.getComputedStyle(this, null).direction == "ltr") { > No rtl issues here, your drop area is the centre of the tab. Fixed > >+ return null; > I'd prefer -1 so that the return value is always an integer. Fixed Patch will be attached when questions are resolved.
Assignee | ||
Updated•18 years ago
|
Attachment #209542 -
Attachment is obsolete: true
Attachment #209542 -
Flags: review?(jag)
Assignee | ||
Comment 12•18 years ago
|
||
Questions were answered over IRC.
Attachment #209543 -
Attachment is obsolete: true
Attachment #210049 -
Flags: superreview?(neil)
Attachment #210049 -
Flags: review?(jag)
Comment 13•18 years ago
|
||
+ if (canDropOn && newIndexOn != null) { I assume that was meant to be -1. Also, you're still multiplying by 0.5 instead of dividing by 2. However, the arrow indenting code can be compressed to this (not tested for typos): var ltr = window.getComputedStyle(this, null).direction == "ltr"; var arrowX; var tabBoxObject; if (canDropOn && newIndexOn != -1) { tabBoxObject = this.mTabs[newIndexOn].boxObject; arrowX = tabBoxObject.x + tabBoxObject.width / 2; } else if (newIndexBetween == this.mTabs.length) { tabBoxObject = this.mTabContainer.lastChild.boxObject; arrowX = tabBoxObject.x; if (ltr) // for LTR "after" is on the right-hand side of the tab arrowX += tabBoxObject.width; } else { tabBoxObject = this.mTabs[newIndexBetween].boxObject; arrowX = tabBoxObject.x; if (!ltr) // for RTL "before" is on the right-hand side of the tab arrowX += tabBoxObject.width; } if (ltr) { ind.style.marginLeft = (arrowX - this.boxObject.x) + "px"; } else { ind.style.marginRight = (this.boxObject.x + this.boxObject.width - arrowX) + "px"; } What do you think? On for more nits: + for (var i = 0; i < this.mTabs.length; i++) { Try to get into the habit of using ++i unless you really need to do something with the old value of |i|. It doesn't matter much here, but with non-trivial iterators you'll be glad to have formed the habit. + if (aEvent.clientX > this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width * .25 + && aEvent.clientX < this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width * .75) While this isn't a big deal, I would put |this.mTabs[i].boxObject| in a var to avoid some look-ups. General nit: try to keep the same curly-brace style as the file you're editing. Some of the ones visible in your patch were introduced earlier, probably by others. Could you clean up a bit in the areas you're touching? I think I might be able to get used to having the arrow always indicate where the URL is gonna drop. Let's run with it for a while and see where we end up.
Assignee | ||
Comment 14•18 years ago
|
||
I really like the new shorter code :) + tabBoxObject = this.mTabContainer.lastChild.boxObject; I'd slightly prefer to use this.mTabs[] here just for consistency (and so you don't have to know that this.mTabs = this.mTabContainer.childNodes...
Attachment #210049 -
Attachment is obsolete: true
Attachment #210493 -
Flags: superreview?(neil)
Attachment #210493 -
Flags: review?(jag)
Attachment #210049 -
Flags: superreview?(neil)
Attachment #210049 -
Flags: review?(jag)
Comment 15•18 years ago
|
||
Comment on attachment 210493 [details] [diff] [review] patch CTho, could you r= the arrowX bits? >Index: mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml >=================================================================== >@@ -1235,65 +1235,74 @@ >+ var canDropOn; > if (aDragSession.sourceNode && >+ aDragSession.sourceNode.parentNode == this.mTabContainer) >+ canDropOn = false; >+ else >+ canDropOn = true; Nit: prefer: var canDropOn = !aDragSession.sourceNode || aDragSession.sourceNode.parentNode != this.mTabContainer; >+ this.moveTabTo(this.mTabs.length-1, newIndex); Nit: spaces around '-' >+ tabBoxObject = this.mTabContainer.lastChild.boxObject; I guess for consistency we could use mTabs. In method getDropIndex(): > if (aEvent.clientX < this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width / 2) > if (aEvent.clientX > this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width / 2) Would you mind pulling the right-hand side of both comparisons into a single variable? In method getDropOnIndex(): >+ if (aEvent.clientX > tabBoxObject.x + tabBoxObject.width * .25 >+ && aEvent.clientX < tabBoxObject.x + tabBoxObject.width * .75) Nit: Put the && on the first line so the two lines line up nicely. r=jag
Attachment #210493 -
Flags: review?(jag) → review+
Assignee | ||
Comment 16•18 years ago
|
||
Comment on attachment 210493 [details] [diff] [review] patch r=me on the parts jag wrote
Attachment #210493 -
Flags: review+
Assignee | ||
Updated•18 years ago
|
Whiteboard: [cst: r+nits sr?]
Comment 17•18 years ago
|
||
Actually, I'd prefer var newIndexOn = aDragSession.sourceNode && aDragSession.sourceNode.parentNode == this.mTabContainer ? -1 : this.getDropOnIndex(event); Then later simply check if (newIndexOn == -1)
Comment 18•18 years ago
|
||
Forest, trees. Good suggestion, Neil! New patch with all nits addressed and we'll slap on all the pretty flags?
Assignee | ||
Comment 19•18 years ago
|
||
Hopefully I got all of them...
Attachment #210493 -
Attachment is obsolete: true
Attachment #210662 -
Flags: superreview?(neil)
Attachment #210493 -
Flags: superreview?(neil)
Comment 20•18 years ago
|
||
Comment on attachment 210662 [details] [diff] [review] patch r=jag, though I would've picked something like "tabMiddle" instead of "coord". Nit: please put |else| on the same line as the } (I notice I didn't quite get that right in comment 13). Actually, looking at the code, |newIndexBetween| and |ltr| are only needed in the else block of |if (newIndexOn != -1)|. That has the nice side effect that the 2nd and 3rd cases (drop between) will be more closely grouped, and offset vs. the 1st case (drop on).
Attachment #210662 -
Flags: review+
Comment 21•18 years ago
|
||
Btw, here are two more ways of doing this: 1) var tabBoxObject; var arrowX; if (newIndexOn != -1) { tabBoxObject = this.mTabs[newIndexOn].boxObject; arrowX = tabBoxObject.x + tabBoxObject.width / 2; } else { var afterLast = false; var newIndexBetween = this.getDropIndex(aEvent); if (newIndexBetween == this.mTabs.length) { afterLast = true; --newIndexBetween; } tabBoxObject = this.mTabs[newIndexBetween].boxObject; arrowX = tabBoxObject.x; var ltr = window.getComputedStyle(this, null).direction == "ltr"; if (ltr && afterLast || !ltr && !afterLast) arrowX += tabBoxObject.width; // right-hand side of the tab } The above is a bit harder to document though, since we're merging the case where for LTR we get the (last + 1) tab's X by getting the last tab's X + its width, with the case where for RTL we get each tab's "RTL-X" by taking its X and adding its width, except for the (last + 1) tab, where we just take the last tab's X. If we change the RTL case to always take the previous tab's X to get a tab's "RTL-X", your special case becomes the first tab, for which you'll still need to take its X and width to get its "RTL-X": 2) var tabBoxObject; var arrowX; if (newIndexOn != -1) { tabBoxObject = this.mTabs[newIndexOn].boxObject; arrowX = tabBoxObject.x + tabBoxObject.width / 2; } else { var moveOneToTheRight = false; var ltr = window.getComputedStyle(this, null).direction == "ltr"; var newIndexBetween = this.getDropIndex(aEvent); if (ltr && newIndexBetween == this.mTabs.length || !ltr && newIndexBetween != 0) { --newIndexBetween; moveOneToTheRight = true; } tabBoxObject = this.mTabs[newIndexBetween].boxObject; arrowX = tabBoxObject.x; if (moveOneToTheRight) arrowX += tabBoxObject.width; // right-hand side of the tab } While both of these reduce duplication I think they're harder to understand and not worth replacing my original suggestion with.
Comment 22•18 years ago
|
||
(In reply to comment #21) > var afterLast = false; > var newIndexBetween = this.getDropIndex(aEvent); > if (newIndexBetween == this.mTabs.length) { > afterLast = true; > --newIndexBetween; > } I'd write this var newIndexBetween = this.getDropIndex(aEvent); var afterLast = newIndexBetween == this.mTabs.length; if (afterLast) --newIndexBetween; //I guess newIndexBetween -= afterLast; is harder to understand ;-) > if (ltr && afterLast || !ltr && !afterLast) if (ltr == afterlast) // but again, harder to understand. I like the idea of not computing newIndexBetween if it's not used though.
Comment 23•18 years ago
|
||
Comment on attachment 210662 [details] [diff] [review] patch >+ var arrowX; >+ var tabBoxObject; var arrowX, tabBoxObject; > var tabIndex = this.getTabIndex(aDragSession.sourceNode); >+ var tabIndex = this.getDropOnIndex(aEvent); Nit: redeclaration of var tabIndex (I know Brendan turned off the warnings but I still care). >+ // We're adding a new tab >+ tab = this.addTab(getShortcutOrURI(url)); >+ this.moveTabTo(this.mTabs.length - 1, newIndex); Check for newIndex == this.mTabs.length - 1 first?
Attachment #210662 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [cst: r+nits sr?] → [cst: baking on trunk]
Comment 24•18 years ago
|
||
(In reply to comment #22) > I'd write this > var newIndexBetween = this.getDropIndex(aEvent); > var afterLast = newIndexBetween == this.mTabs.length; > if (afterLast) > --newIndexBetween; Yeah! Though I kinda like my second alternative better, since it means on average adding tabBoxObject.width less often for RTL. > //I guess newIndexBetween -= afterLast; is harder to understand ;-) That, and I don't really like using that true evaluates to 1 in numeric context. > > if (ltr && afterLast || !ltr && !afterLast) > if (ltr == afterlast) // but again, harder to understand. Yeah, I considered that, but decided against it 'coz it'd warrent a comment longer than the test I ended up using ;-)
Comment 25•18 years ago
|
||
Attachment #210885 -
Flags: superreview?(neil)
Attachment #210885 -
Flags: review?
Comment 26•18 years ago
|
||
Comment on attachment 210885 [details] [diff] [review] (diff -uw) Move newIndexBetween and ltr into the |else| >+ var newIndexBetween = this.getDropIndex(aEvent); >+ var ltr = window.getComputedStyle(this, null).direction == "ltr"; >+ if (newIndexBetween == this.mTabs.length) { I'd swap the declarations because it looks more efficient to use newIndexBetween immediately after calculating it ;-)
Attachment #210885 -
Flags: superreview?(neil) → superreview+
Comment 27•18 years ago
|
||
Yes, of course
Comment 28•18 years ago
|
||
Attachment #210885 -
Attachment is obsolete: true
Attachment #210940 -
Flags: superreview?(neil)
Attachment #210940 -
Flags: review?(neil)
Attachment #210885 -
Flags: review?
Comment 29•18 years ago
|
||
Attachment #210940 -
Attachment is obsolete: true
Attachment #210962 -
Flags: superreview?(neil)
Attachment #210962 -
Flags: review?(neil)
Attachment #210940 -
Flags: superreview?(neil)
Attachment #210940 -
Flags: review?(neil)
Updated•18 years ago
|
Attachment #210962 -
Flags: superreview?(neil)
Attachment #210962 -
Flags: superreview+
Attachment #210962 -
Flags: review?(neil)
Attachment #210962 -
Flags: review+
Assignee | ||
Updated•18 years ago
|
Attachment #210662 -
Flags: approval-branch-1.8.1?(neil)
Assignee | ||
Updated•18 years ago
|
Attachment #210962 -
Flags: approval-branch-1.8.1?(neil)
Updated•18 years ago
|
Attachment #210662 -
Flags: approval-branch-1.8.1?(neil) → approval-branch-1.8.1+
Updated•18 years ago
|
Attachment #210962 -
Flags: approval-branch-1.8.1?(neil) → approval-branch-1.8.1+
Assignee | ||
Comment 30•18 years ago
|
||
Checking in mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml; /cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v <-- tabbrowser.xml new revision: 1.126.2.16; previous revision: 1.126.2.15 done
Keywords: fixed-seamonkey1.1a
Whiteboard: [cst: baking on trunk]
Comment 31•18 years ago
|
||
'approval-branch-1.8.1=?': (SeaMonkey only)
Attachment 210662 [details] [diff] landed on (Trunk and) MOZILLA_1_8_BRANCH,
but these simple enhanced bits never made it to the branch: synchronizing.
Attachment #222621 -
Flags: superreview?(neil)
Attachment #222621 -
Flags: review?(neil)
Attachment #222621 -
Flags: approval-branch-1.8.1?(neil)
Updated•18 years ago
|
Attachment #222621 -
Flags: superreview?(neil)
Attachment #222621 -
Flags: superreview+
Attachment #222621 -
Flags: review?(neil)
Attachment #222621 -
Flags: review+
Attachment #222621 -
Flags: approval-branch-1.8.1?(neil)
Attachment #222621 -
Flags: approval-branch-1.8.1+
Comment 32•18 years ago
|
||
Comment on attachment 222621 [details] [diff] [review] (Cv1-181) <tabbrowser.xml> [Checkin: Comment 32] Checkin: { 2006-05-19 08:05 neil%parkwaycc.co.uk mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml 1.126.2.21 MOZILLA_1_8_BRANCH }
Attachment #222621 -
Attachment description: (Cv1-181) <tabbrowser.xml> → (Cv1-181) <tabbrowser.xml>
[Checkin: Comment 32]
Attachment #222621 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•