194913 - Need ability to drag n drop tabs to reorder them

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, enhancement)

Description

[Dave Saville]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.3b) Gecko/20030211
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.3b) Gecko/20030211

I would like to be able to re-order the channel and server tabs. RMB will drag
the tab but you can't drop it on the bar again to reorder.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Alfonso Martinez]

*** This bug has been marked as a duplicate of 105885 ***

Comment 2

[Samuel Sieb]

This isn't quite a duplicate.  Chatzilla tabs and browser tabs are different, so
we will most likely have to implement this ourselves.  The only way we wouldn't
have to do it is if there is a generic solution created for the browser tabs
that could also be applied to ours.

Comment 3

[James Ross]

Attachment: Allow tabs to be dropped onto the tab bar

So, here you go. This adds the drag-related handlers to the tab bar, including a little blue arrow to indicate drop location, and supports both dragging existing URLs (from existing tabs or just plain URLs) and new URL onto it. In the case of channel/user URLs, both the network/server tab (if it needs to be created) and the channel/user tab will appear at the drop location.

This also should cover bug 343865, which was needed as tabbox.xml totally fails to appreciate the DOM adjustments.

Comment 4

[:Gijs (he/him)]

Comment on attachment 293294 [details] [diff] [review]
Allow tabs to be dropped onto the tab bar

>Index: xul/content/static.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/irc/xul/content/static.js,v
>retrieving revision 1.253
>diff -d -p -u -6 -r1.253 static.js
>--- xul/content/static.js	9 Dec 2007 15:09:43 -0000	1.253
>+++ xul/content/static.js	15 Dec 2007 17:06:00 -0000
>@@ -853,14 +857,21 @@ function processStartupURLs()
>             if (ary[i] && ary[i] != "irc://")
>                 gotoIRCURL(ary[i]);
>         }
>     }
> 
>     if (client.viewsArray.length > 1 && !isStartupURL("irc://"))
>+        dispatch("delete-view", { view: client });
>+
>+    /* XXX: If we have the "stop XBL breaking" hidden tab, remove it, to
>+     * stop XBL breaking later. Oh, the irony.
>+     */
>+    if (client.tabs.firstChild.hidden)
>     {
>-        dispatch("delete-view", {view: client});
>+        client.tabs.removeChild(client.tabs.firstChild);
>+        updateTabAttributes();
>     }
> }

Is there a bug filed on the XBL breakery? Might be worth including that here if there is.

> 
> function destroy()
> {
>     destroyPrefs();
>@@ -1649,31 +1660,33 @@ function doObjectURLtest()
>     }
>     display("Passed " + passed + " out of " + total + " tests (" +
>             passed / total * 100 + "%).", MT_INFO);
> }
> 
> 
>-function gotoIRCURL (url)
>+function gotoIRCURL(url, e)
> {
>     var urlspec = url;
>     if (typeof url == "string")
>         url = parseIRCURL(url);
> 
>     if (!url)
>     {
>-        window.alert (getMsg(MSG_ERR_BAD_IRCURL, urlspec));
>+        window.alert(getMsg(MSG_ERR_BAD_IRCURL, urlspec));

*whimper*

>@@ -1816,23 +1838,26 @@ function gotoIRCURL (url)
<snip>
>         if (url.msg)
>         {
>+            client.pendingViewContext = e;
>             var msg;
>             if (url.msg.indexOf("\01ACTION") == 0)
>             {
>                 msg = filterOutput(url.msg, "ACTION", targetObject);
>                 targetObject.display(msg, "ACTION", "ME!",
>                                      client.currentObject);
>@@ -1842,19 +1867,22 @@ function gotoIRCURL (url)
>                 msg = filterOutput(url.msg, "PRIVMSG", targetObject);
>                 targetObject.display(msg, "PRIVMSG", "ME!",
>                                      client.currentObject);
>             }
>             targetObject.say(msg);
>             dispatch("set-current-view", { view: targetObject });
>+            delete client.pendingViewContext;
>         }
>     }
>     else
>     {
>+        client.pendingViewContext = e;
>         if (!network.messages)
>-            network.displayHere (getMsg(MSG_NETWORK_OPENED, network.unicodeName));
>+            network.displayHere(getMsg(MSG_NETWORK_OPENED, network.unicodeName));
>         dispatch("set-current-view", { view: network });
>+        delete client.pendingViewContext;
>     }
> }

These changes are unhappy. Why are you setting the pendingViewContext for the display() calls as well? Are those things creating the views? Might be better to put the set-current-view stuff before the display in the first case (might be 'hard' in the second case as the !network.messages check would fail...). Right now this scares me a bit, also because theoretically display() will lose its innocence when I get to finishing that filter patch.

<snip>
>@@ -2978,49 +3006,75 @@ function getTabForObject (source, create
<snip>
>+    // Didn't found a <tab>, but we're allowed to create one.

Nit: find

<snip>
>+function updateTabAttributes()
>+{
>+    /* XXX: Workaround for Gecko bugs 272646 and 261826. Note that this breaks
>+     * the location of the spacers before and after the tabs but, due to our
>+     * own <spacer>, their flex was not being utilised anyway.
>+     */

Could themes be changing that? Have you tested the patch with some other themes? :-)

>+    var tabOrdinal = 0;
>+    for (var tab = client.tabs.firstChild; tab; tab = tab.nextSibling)
>+        tab.ordinal = tabOrdinal++;
> 
>+    /* XXX: Workaround for tabbox.xml not coping with updating attributes when
>+     * tabs are moved. We correct "first-tab", "last-tab", "beforeselected" and
>+     * "afterselected".
>+     */
>+    var tabAttrs = {
>+        "first-tab": null,
>+        "last-tab": null,
>+        "beforeselected": null,
>+        "afterselected": null
>+    };
>+    var foundSelected = "before";
>+    for (tab = client.tabs.firstChild; tab; tab = tab.nextSibling)
>+    {
>+        if (tab.collapsed || tab.hidden)
>+            continue;
>+
>+        if (!tabAttrs["first-tab"])
>+            tabAttrs["first-tab"] = tab;
>+        tabAttrs["last-tab"] = tab;
>+
>+        if ((foundSelected == "before") && tab.selected)
>+            foundSelected = "on";
>+        else if (foundSelected == "on")
>+            foundSelected = "after";
>+
>+        if (foundSelected == "before")
>+            tabAttrs["beforeselected"] = tab;
>+        if ((foundSelected == "after") && !tabAttrs["afterselected"])
>+            tabAttrs["afterselected"] = tab;
>+    }

This is truly some of the most hard-to-read/understand code I've ever seen (yes, that means I haven't seen much, but anyway) - I can't really find a good way to make it more sane. I would think that it would help to keep the things that you're advancing all the time (beforeselected, last-tab) were together, and the things that depend on the foundselected and selection were separated from those, but then, there is also something to be said for the current setup... Meh. If you can make this more readable, please do. Otherwise perhaps leave a comment that last-tab and beforeselected are continually updated through the loop, and the others are set once only.


>@@ -3068,48 +3202,170 @@ function ul_getcellprops(index, column, 
<snip>
>+var tabsDropObserver = new Object();
>+
>+tabsDropObserver.canDrop =
>+function tabdnd_candrop(aEvent, aDragSession)
>+{
>+    if (aEvent.getPreventDefault())
>+        return;
>+
>+    // Check whether the drag session contains a supported flavor.
>+    var flavourSet = this.getSupportedFlavours();
>+    for (var flavour in flavourSet.flavourTable)
>+    {
>+        if (aDragSession.isDataFlavorSupported(flavour))
>+            return true;
>+    }
>+    return false;
>+}
>+
>+tabsDropObserver.onDragOver =
>+function tabdnd_dover(aEvent, aFlavour, aDragSession)
>+{
>+    if (aEvent.getPreventDefault())
>+        return;
>+
>+    // Override the check in nsDragAndDrop.checkCanDrop.
>+    if (aDragSession.sourceNode == aEvent.target)
>+        aDragSession.canDrop = true;

Please move this check to checkCanDrop itself, unless you can provide some magical reason why that's not possible (which should then find its way into a comment here).

<snip>
>+tabsDropObserver.onDrop =
>+function tabdnd_drop(aEvent, aXferData, aDragSession)
>+{
>+    // Extra check, not performed anywhere else, to only accept IRC URLs.
>+    var url = transferUtils.retrieveURLFromData(aXferData.data,
>+                                                aXferData.flavour.contentType);

*why* is this not checked in checkCanDrop? Please move the check appropriately, or provide a comment why that's not possible.

<snip>


r=me with the comments addressed

Comment 5

[James Ross]

(In reply to comment #4)
> Is there a bug filed on the XBL breakery? Might be worth including that here if
> there is.

No, and I don't intend to file one. The only issue that I can remember is that it removes the "first-tab" attribute from the first visible tab because of the hidden tab before it.

> These changes are unhappy. Why are you setting the pendingViewContext for the
> display() calls as well? Are those things creating the views? Might be better
> to put the set-current-view stuff before the display in the first case (might
> be 'hard' in the second case as the !network.messages check would fail...).
> Right now this scares me a bit, also because theoretically display() will lose
> its innocence when I get to finishing that filter patch.

I absolutely have to set it before the display calls, because they create the view. I have no idea what you mean WRT innocence of display().

> Could themes be changing that? Have you tested the patch with some other
> themes? :-)

No, and I don't intend to. We have no choice here.

> This is truly some of the most hard-to-read/understand code I've ever seen
> (yes, that means I haven't seen much, but anyway) - I can't really find a good
> way to make it more sane. I would think that it would help to keep the things
> that you're advancing all the time (beforeselected, last-tab) were together,
> and the things that depend on the foundselected and selection were separated
> from those, but then, there is also something to be said for the current
> setup... Meh. If you can make this more readable, please do. Otherwise perhaps
> leave a comment that last-tab and beforeselected are continually updated
> through the loop, and the others are set once only.

Glad you like it. It's the most logical and shortest arrangement currently, but I could try to expand the comment before it.

> Please move this check to checkCanDrop itself, unless you can provide some
> magical reason why that's not possible (which should then find its way into a
> comment here).

The "magical reason" is that it's not our code, it's the thing calling us. Also, the reason for the check is because the caller's code is badly-designed abd broken, but we have to live with that (it's been the same from Moz 1.0 to today, AFAIK).

> *why* is this not checked in checkCanDrop? Please move the check appropriately,
> or provide a comment why that's not possible.

Well, checkCanDrop is not our code (see above), but the code is not elsewhere mainly for performance and simplicity reasons. It's a right pain to get the data needed for the check, and the calling code only does that work when calling our onDrop code, not for anything else (entirely understandably). Bear in mind that fetching the data for a drag operation could mean *hitting the network*, potentially even hitting the network over a VPN. You don't do it in dragover. :)

Comment 6

[:Gijs (he/him)]

(In reply to comment #5)
> (In reply to comment #4)
> > Is there a bug filed on the XBL breakery? Might be worth including that here if
> > there is.
> 
> No, and I don't intend to file one. The only issue that I can remember is that
> it removes the "first-tab" attribute from the first visible tab because of the
> hidden tab before it.

But the updateAttributes() code copes with that, right?

> > These changes are unhappy. Why are you setting the pendingViewContext for the
> > display() calls as well? Are those things creating the views? Might be better
> > to put the set-current-view stuff before the display in the first case (might
> > be 'hard' in the second case as the !network.messages check would fail...).
> > Right now this scares me a bit, also because theoretically display() will lose
> > its innocence when I get to finishing that filter patch.
> 
> I absolutely have to set it before the display calls, because they create the
> view. I have no idea what you mean WRT innocence of display().

I mean that display() could open views itself - possibly not just this one (cf. copy to network tab) - and with the filter setup, run arbitrary commands. But it doesn't seem like you have much of a choice then. Leave it like this and I'll sanity check it myself when I finish filters.
 
> > Could themes be changing that? Have you tested the patch with some other
> > themes? :-)
> 
> No, and I don't intend to. We have no choice here.

OK.

> > This is truly some of the most hard-to-read/understand code I've ever seen
> > (yes, that means I haven't seen much, but anyway) - I can't really find a good
> > way to make it more sane. I would think that it would help to keep the things
> > that you're advancing all the time (beforeselected, last-tab) were together,
> > and the things that depend on the foundselected and selection were separated
> > from those, but then, there is also something to be said for the current
> > setup... Meh. If you can make this more readable, please do. Otherwise perhaps
> > leave a comment that last-tab and beforeselected are continually updated
> > through the loop, and the others are set once only.
> 
> Glad you like it. It's the most logical and shortest arrangement currently, but
> I could try to expand the comment before it.

That would help. Another option I thought of was adding inline functions for finding the previous/next 'real' tab (ie. not hidden/collapsed), then looping once to find the selection and set before- and afterselected using those... that would probably be longer and slightly slower (not important unless you open *lots* of tabs, which isn't going to happen) but may be more clear. Your choice, you can have r+ either way. :-)

> > Please move this check to checkCanDrop itself, unless you can provide some
> > magical reason why that's not possible (which should then find its way into a
> > comment here).
> 
> The "magical reason" is that it's not our code, it's the thing calling us.
> Also, the reason for the check is because the caller's code is badly-designed
> abd broken, but we have to live with that (it's been the same from Moz 1.0 to
> today, AFAIK).
> 
> > *why* is this not checked in checkCanDrop? Please move the check appropriately,
> > or provide a comment why that's not possible.
> 
> Well, checkCanDrop is not our code (see above), but the code is not elsewhere
> mainly for performance and simplicity reasons. It's a right pain to get the
> data needed for the check, and the calling code only does that work when
> calling our onDrop code, not for anything else (entirely understandably). Bear
> in mind that fetching the data for a drag operation could mean *hitting the
> network*, potentially even hitting the network over a VPN. You don't do it in
> dragover. :)

Yeah, my mistake. I thought you meant canDrop (ie. what's right above it).

So yeah, r=me with the extra comment or a more clear implementation - your pick.

Comment 7

[James Ross]

(In reply to comment #6)
> But the updateAttributes() code copes with that, right?

It would correct it if called, yes, but the patch isn't calling updateAttributes() every time you change tab (as would be needed). It's probably pretty expensive too, and I think we'll be better off in the long run by not having the special hidden tab at run-time.

> I mean that display() could open views itself - possibly not just this one (cf.
> copy to network tab) - and with the filter setup, run arbitrary commands.

As I recall, displayHere/display is the preferred way to create new views within ChatZilla's codebase, even for things like /query and /join.

> Yeah, my mistake. I thought you meant canDrop (ie. what's right above it).

Ah, well I think the interaction of the calling code and ours needs a block-comment or two to explain it, and why some bits of code are in unexpected places. I'll attach a new patch for that, but it'll only be comment changes and shouldn't need the code re-reviewing. :)

Comment 8

[:Gijs (he/him)]

(In reply to comment #7)
> (In reply to comment #6)
> > Yeah, my mistake. I thought you meant canDrop (ie. what's right above it).
> 
> Ah, well I think the interaction of the calling code and ours needs a
> block-comment or two to explain it, and why some bits of code are in unexpected
> places. I'll attach a new patch for that, but it'll only be comment changes and
> shouldn't need the code re-reviewing. :)

Sounds good. r+ from me on that, then. I'll be mostly gone as of tomorrow morning CET (and completely gone on saturday and after). Please do check this in in my absence ;-).

Comment 9

[Reed Loden [:reed]]

Please do not add the checkin-needed keyword to bugs of people with commit access without approval of the patch author. Silver can take care of his own patch.

Comment 10

[Justin Wood (:Callek)]

O, whops... sorry. Read Gijs review, and did not realize Silver did the patch.

Comment 11

[James Ross]

Checked in --> FIXED.

Wooyay wooyay. Please file any issues with the drag and drop of tabs as new bugs.