556887 - Reordering of newsgroups using drag & drop doesn't work anymore

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

With recent SM trunk nightlies (regression range unknown), reordering of newsgroups using drag & drop doesn't work anymore. This still works with recent 2.0 branch nightlies.

I remember some d&d related core changes happened on trunk. Neil, maybe you have an idea?

TB trunk is not affected (anymore?).

Comment 1

[Hartmut Figge]

(In reply to comment #0)
> With recent SM trunk nightlies (regression range unknown), ...

According to my private builds the regression range is

hafi@e675 ~ $ fenster 1003040003-1003050007
2010-03-03 15:03:00 PDT
2010-03-04 15:07:00 PDT

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Thanks. Probably <http://hg.mozilla.org/comm-central/rev/3d75fae6a1ed> (bug 537448). Hartmut, can you please try with a local backout?

Comment 3

[Hartmut Figge]

(In reply to comment #2)
> Thanks. Probably <http://hg.mozilla.org/comm-central/rev/3d75fae6a1ed> (bug
> 537448). Hartmut, can you please try with a local backout?

After backing out the problem was gone. Because there were a lot of changes since my last build i have double checked with an immediately following new build without backing out. The problem was back again then.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

It looks to me as if we used to pass a FolderResource but now we're passing an nsIMsgFolder instance:

old:
folderArray.push(GetFolderResource(folderTree, j).Value);
->
BeginDragTree(event, folderTree, folderArray, flavor);
-> (folderArray -> selArray)
dataTransfer.mozSetDataAt(flavor, selArray[i], i);

new:
var folders = GetSelectedMsgFolders();
->
dataTransfer.mozSetDataAt(flavor, folders[i], i);

GetSelectedMsgFolders does:
var folderResource = GetFolderResource(folderTree, j);
let msgFolder = folderResource.QueryInterface(Components.interfaces.nsIMsgFolder);
folderArray.push(msgFolder);


The problem is likely cut&paste from TB code which works differently than ours. Our CanDropOnFolderTree [1] gets the data flavor using trans.getAnyTransferData (after calling dragSession.getData) which now fails for newsgroup d&d. TB gets the data flavor from the dataTransfer object itself which they keep available in their gFolderTreeView [2] (set with _onDragOver, checked in canDrop).

[1] suite/mailnews/messengerdnd.js
[2] mail/base/content/folderPane.js

Comment 5

[Robert Kaiser]

I think we should align ourselves as much as possible with how Thunderbird does it, esp. in the light of (hopefully) porting the folderPane.js work as well.

Comment 6

[Philip Chee]

I'll be travelling for the next two weeks so I probably won't have time to work on this. I hope someone else can pick this up.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Some observations:
1. TB seems to do the same in its _onDragStart that we do in our BeginDragFolderTree, and in both cases folders[i] is [XPComponent] [class: XPCWrappedNative_NoHelper] (according to Venkman) when dataTransfer.mozSetDataAt is called.
2. The dataTransfer object can be retrieved using dragSession.dataTransfer, i.e. we don't need to port _onDragOver and _currentTransfer from TB (at this point).
3. The reason why thread pane d&d (DragOverThreadPane and DropOnThreadPane in messengerdnd.js) still works is because it works the same way as in TB: accessing dataTransfer (dt), dt.mozItemCount, dt.mozTypesAt, and dt.mozGetDataAt.
4. Our CanDropOnFolderTree and DropOnFolderTree try to get the drag data from dragSession into a transferable which fails now. This is consistent with dataTransfer.getData("text/x-moz-newsfolder") returning an empty string in both our CanDropOnFolderTree and TB's canDrop (checked using Venkman; originally there is no such code in either function).
5. Unless reverting parts of bug 537448 is an option (and comment 5 suggests the opposite) the way to go would be to adapt CanDropOnFolderTree and DropOnFolderTree according to point 3.

If a simpler solution is viable (and likely to pass review!) please let me know. Or if my analysis is flawed. Otherwise I could try and implement the above but if in the end someone came up with a one-line fix that would really bug me (pun intended).

Comment 8

[Philip Chee]

I've come to the same conclusion re #2 and #3. Please go ahead. I'd do this myself but will be away for a few days. I'll have internet access from the hotel room but not the time to work on bugs.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Attachment: canDrop proof of concept

This is a proof of concept for the first part, canDrop (in TB folder pane terms) which decides whether a drop is allowed for a certain combination. The drop itself obviously won't work yet because that's still the old DropOnFolderTree code.

* kept the old code around for now (renamed old function) to get a contiguous code block for the new code (hence the "proof of concept"; the code is finished AFAIC but obviously not ready for check-in) and allow for checking against the old code (review)
* code mostly ripped from TB (folderPane.js canDrop), adjusted to our environment (e.g. targetFolder, dt) and code style (hopefully)
* order still according to old CanDropOnFolderTree to hopefully aid review (can be changed to TB order at any time)
* added some more comments in comparison to the TB code
* pulled up RSS /https?/ check from TB's drop function to our canDrop function (we also had it in our old one and it's only consistent to do it early on)

I'll be away Tuesday to Thursday so cannot provide patch updates until I'm back but will try to read bugmail.

@HaFi: You're invited to test as well. You'll see that there are many test cases (sources: file, message, folder, NG and RSS URL) and I only checked NG reordering and RSS URLs until now. Be reminded that only drop indication works for now, though, not the actual drop.

Comment 10

[Hartmut Figge]

(In reply to comment #9)
> Created an attachment (id=437090) [details]
> canDrop proof of concept

Drop indication works for NGs. Of course the error console is complaining following the attempt to move one NG. I will not use RSS until there is a possibility to avoid acoustic notifications, so i haven't tested this.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Attachment: complete proof of concept

Since I didn't get any feedback at all from my fellow devs (hint) I thought I could go forward and do the second part as well meanwhile. This time it's really a TB rip-off; I didn't even try to fully understand the old code any more. I did the usual code adaptions and clean-up, though, and of course the changes required to make the code work at all (e.g. the text/x-moz-newsfolder handling is quite different).

@HaFi: Again, your feedback regarding functionality is very welcome. If you click Details in the attachments box you should be able to +/- your feedback request. At least that's what I'd expect, I hope it doesn't require special permissions.

Comment 12

[Hartmut Figge]

Comment on attachment 439820 [details] [diff] [review]
complete proof of concept

Your proof_of_concept does allow moving newsgroups around in the folderpane again.

Comment 13

[[e-mail]]

Comment on attachment 439820 [details] [diff] [review]
complete proof of concept

Sorry for the delay, it's been a busy month.

>+    for (let i = 0; i < dt.mozItemCount; i++)
>+    {
>+      let extFile = dt.mozGetDataAt("application/x-moz-file", i)
>+                      .QueryInterface(Components.interfaces.nsIFile);
>+      return extFile.isFile();
I don't think this does what you want.

>+      // Don't copy within same server.
>+      if (folder.server == targetFolder.server && dt.dropEffect == 'copy')
>+        return false;
Odd; the drop code doesn't look at the drop effect...

>+    let uri = Components.classes["@mozilla.org/network/io-service;1"]
>+                        .getService(Components.interfaces.nsIIOService)
>+                        .newURI(url, null, null);
>+    if ((uri.schemeIs("http") || uri.schemeIs("https")) &&
Perhaps use extractScheme instead? Also, don't we support feed: URLs?

>+    for (let i = 0; i < count; i++)
>+    {
>+      let folders = new Array;
>+      folders.push(dt.mozGetDataAt("text/x-moz-folder", i)
>+                     .QueryInterface(Components.interfaces.nsIMsgFolder));
>+      let array = toXPCOMArray(folders, Components.interfaces.nsIMutableArray);
>+      cs.CopyFolders(array, targetFolder,
>+                    (folders[0].server == targetFolder.server), null,
>+                     msgWindow);
Shouldn't you copy the folders after the loop? Also, why not put the folders directly in an nsIMutableArray in the first place? (nsIMutableArray takes nsISupports so you don't need to QI to nsIMsgFolder.)

>+    for (let i = 0; i < count; i++)
>+    {
>+      let folder = dt.mozGetDataAt("text/x-moz-newsfolder", i)
>+                     .QueryInterface(Components.interfaces.nsIMsgFolder);
>+      newsFolder.moveFolder(folder, targetFolder, aOrientation);
>+      SelectFolder(folder.URI);
Surely you only want to select one folder (preferably the loaded folder)?

>+    let isMove = Components.classes["@mozilla.org/widget/dragservice;1"]
>+                           .getService(Components.interfaces.nsIDragService)
>+                           .getCurrentSession().dragAction ==
You already have a variable for the current session.

>+    prefBranch.setCharPref("last_msg_movecopy_target_uri", targetFolder.URI);
>+    prefBranch.setBoolPref("last_msg_movecopy_was_move", isMove);
Debugging?

>+    let uri = Components.classes["@mozilla.org/network/io-service;1"]
>+                        .getService(Components.interfaces.nsIIOService)
>+                        .newURI(url, null, null);
>+    if (!(uri.schemeIs("http") || uri.schemeIs("https")) ||
>+        targetFolder.server.type != "rss")
>+      return;
You already checked this ;-)

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #13)
> >+    for (let i = 0; i < dt.mozItemCount; i++)
> >+    {
> >+      let extFile = dt.mozGetDataAt("application/x-moz-file", i)
> >+                      .QueryInterface(Components.interfaces.nsIFile);
> >+      return extFile.isFile();
> I don't think this does what you want.

You mean because it only handles the i==0 case (unconditional return once the loop is entered) or do you mean something else? I just ripped it off from TB...

> Odd; the drop code doesn't look at the drop effect...

Yes but neither TB's nor our existing code does either (if that counts as an argument).

> >+    if ((uri.schemeIs("http") || uri.schemeIs("https")) &&
> Perhaps use extractScheme instead? Also, don't we support feed: URLs?

1. I don't know how much we'd gain from using extractScheme and I'm pretty sure I'd break something if I tried to change the code to use it.

2. AFAICS neither we (existing code) nor TB support feed URIs. Probably something for another bug.

> >+    for (let i = 0; i < count; i++)
> >+    {
> >+      let folders = new Array;
> >+      folders.push(dt.mozGetDataAt("text/x-moz-folder", i)
> >+                     .QueryInterface(Components.interfaces.nsIMsgFolder));
> >+      let array = toXPCOMArray(folders, Components.interfaces.nsIMutableArray);
> >+      cs.CopyFolders(array, targetFolder,
> >+                    (folders[0].server == targetFolder.server), null,
> >+                     msgWindow);
> Shouldn't you copy the folders after the loop? Also, why not put the folders
> directly in an nsIMutableArray in the first place? (nsIMutableArray takes
> nsISupports so you don't need to QI to nsIMsgFolder.)

Again I just copied what TB has. Maybe you're right but I cannot estimate what might break if I tried changing the code (all those JS/C++ bridge things scare me, to be honest).

> >+    for (let i = 0; i < count; i++)
> >+    {
> >+      let folder = dt.mozGetDataAt("text/x-moz-newsfolder", i)
> >+                     .QueryInterface(Components.interfaces.nsIMsgFolder);
> >+      newsFolder.moveFolder(folder, targetFolder, aOrientation);
> >+      SelectFolder(folder.URI);
> Surely you only want to select one folder (preferably the loaded folder)?

This is actually a case where I tried to use what we have while porting code from TB. TB does this.selection.toggleSelect(this.getIndexOfFolder(folder));
 there which refers to parts of their gFolderTreeView object that we do not have yet. AFAICS their toggleSelect adds a folder to the selection in contrast to our SelectFolder which changes (replaces) the selection. So yes we should probably either call SelectFolder only once (then what do you mean with "loaded folder" and how could I identify it?) or find a way to expand the selection like TB does.

> >+    let isMove = Components.classes["@mozilla.org/widget/dragservice;1"]
> >+                           .getService(Components.interfaces.nsIDragService)
> >+                           .getCurrentSession().dragAction ==
> You already have a variable for the current session.

Right, changed locally to dragSession.dragAction.

> >+    prefBranch.setCharPref("last_msg_movecopy_target_uri", targetFolder.URI);
> >+    prefBranch.setBoolPref("last_msg_movecopy_was_move", isMove);
> Debugging?

Both are in our existing code and TB's code. Unlike TB we don't seem to use them anywhere, though, and do not initialize them (TB has them in all-thunderbird.js). I'll leave it to you whether we should keep them for compatibility purposes (extensions?). I should not that TB sets them in mailWindowOverlay.js as well, though, while we don't so it's already not completely in sync.

> >+    let uri = Components.classes["@mozilla.org/network/io-service;1"]
> >+                        .getService(Components.interfaces.nsIIOService)
> >+                        .newURI(url, null, null);
> >+    if (!(uri.schemeIs("http") || uri.schemeIs("https")) ||
> >+        targetFolder.server.type != "rss")
> >+      return;
> You already checked this ;-)

Right, removed locally.

Comment 15

[[e-mail]]

(In reply to comment #14)
> (In reply to comment #13)
>>>+      return extFile.isFile();
>>I don't think this does what you want.
>You mean because it only handles the i == 0 case
Exactly.

>>Odd; the drop code doesn't look at the drop effect...
>Yes but neither TB's nor our existing code does either (if that counts as an
>argument).
Does our existing drag code look at it? (What I found odd was the inconsistent drag vs. drop handling.)

>>>+    if ((uri.schemeIs("http") || uri.schemeIs("https")) &&
>>Perhaps use extractScheme instead? Also, don't we support feed: URLs?
>1. I don't know how much we'd gain from using extractScheme and I'm pretty sure
>I'd break something if I tried to change the code to use it.
extractScheme is basically legitimate string-twiddling to get the scheme without having to construct the whole URI.

> 2. AFAICS neither we (existing code) nor TB support feed URIs. Probably
> something for another bug.
Fair enough.

>>>+      cs.CopyFolders(array, targetFolder,
>>>+                    (folders[0].server == targetFolder.server), null,
>>>+                     msgWindow);
>>Shouldn't you copy the folders after the loop?
>Again I just copied what TB has.
That doesn't make them right :-( I guess I will have to ask bienvenu.

>>>+      SelectFolder(folder.URI);
>>Surely you only want to select one folder (preferably the loaded folder)?
>This is actually a case where I tried to use what we have while porting code
>from TB. TB does this.selection.toggleSelect(this.getIndexOfFolder(folder));
>there which refers to parts of their gFolderTreeView object that we do not
>have yet. AFAICS their toggleSelect adds a folder to the selection in contrast
>to our SelectFolder which changes (replaces) the selection. So yes we should
>probably either call SelectFolder only once (then what do you mean with "loaded
>folder" and how could I identify it?) or find a way to expand the selection
>like TB does.
toggleSelect is the easy bit, since it's a method of the folder tree view's selection. I don't know whether we have a method to get the index of a folder, but SelectFolder might already call it.

>>>+    prefBranch.setCharPref("last_msg_movecopy_target_uri", targetFolder.URI);
>>>+    prefBranch.setBoolPref("last_msg_movecopy_was_move", isMove);
>>Debugging?
> Both are in our existing code and TB's code. Unlike TB we don't seem to use
> them anywhere, though, and do not initialize them (TB has them in
> all-thunderbird.js). I'll leave it to you whether we should keep them for
> compatibility purposes (extensions?).
Ah, it's part of TB's "Move/Copy Again" menuitem. Maybe Mnyromyr wants to port it anyway.

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #15)
> > (In reply to comment #13)
> >>>+      return extFile.isFile();
> >>I don't think this does what you want.
> >You mean because it only handles the i == 0 case
> Exactly.

Hmm, when we arrive there we already know that the 0th element exists and is of type application/x-moz-file and in that case we always return extFile.isFile() so this:

    for (let i = 0; i < dt.mozItemCount; i++)
    {
      let extFile = dt.mozGetDataAt("application/x-moz-file", i)
                      .QueryInterface(Components.interfaces.nsIFile);
      return extFile.isFile();
    }
    return false;

can be replaced by this:

    let extFile = dt.mozGetDataAt("application/x-moz-file", 0)
                    .QueryInterface(Components.interfaces.nsIFile);
    return extFile.isFile();

unless I'm missing something.

> >>Odd; the drop code doesn't look at the drop effect...
> >Yes but neither TB's nor our existing code does either (if that counts as an
> >argument).
> Does our existing drag code look at it? (What I found odd was the inconsistent
> drag vs. drop handling.)

Yes:

    // no copy for folder drag within a server
    if (dragSession.dragAction == nsIDragService.DRAGDROP_ACTION_COPY && sourceServer == targetServer)
      return false;

is inside a "if (dragFolder)" of the old/existing CanDropOnFolderTree block.

> toggleSelect is the easy bit, since it's a method of the folder tree view's
> selection. I don't know whether we have a method to get the index of a folder,
> but SelectFolder might already call it.

I was a bit confused because TB uses a JS tree selection implementation in which their toggleSelect can be found. Now that you mentioned it I checked again and saw that nsITreeSelection indeed provides toggleSelect as well. Together with the SelectFolder implementation details I was able to come up with the below as a replacement for SelectFolder(folder.URI) which works like a charm:

    let folderIndex = EnsureFolderIndex(folderTree.builderView, folder);
    if (folderIndex >= 0)
    {
      folderTree.view.selection.toggleSelect(folderIndex);
      folderTree.treeBoxObject.ensureRowIsVisible(folderIndex);
    }

Thus that part is corrected locally.

Next I'll continue with checking how to use extractScheme. Maybe you can ask bienvenu about that other question meanwhile.

Comment 17

[[e-mail]]

(In reply to comment #16)
> Hmm, when we arrive there we already know that the 0th element exists and is of
> type application/x-moz-file and in that case we always return extFile.isFile()
> so this:
That's not true, because this is in the CanDrop code; ironically the actual Drop code checks specifically for .eml files.

> > >Yes but neither TB's nor our existing code does either (if that counts as an
> > >argument).
> > Does our existing drag code look at it? (What I found odd was the inconsistent
> > drag vs. drop handling.)
> Yes:
Fair enough.

> Maybe you can ask bienvenu about that other question meanwhile.
Would you mind pinging me when we're all on IRC?

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #17)
> (In reply to comment #16)
> > Hmm, when we arrive there we already know that the 0th element exists and is of
> > type application/x-moz-file and in that case we always return extFile.isFile()
> > so this:
> That's not true, because this is in the CanDrop code; ironically the actual
> Drop code checks specifically for .eml files.

Huh? It's inside this:

  let types = dt.mozTypesAt(0);
  if (Array.indexOf(types, "application/x-moz-file") != -1)

Or am I getting you wrong?

> > Maybe you can ask bienvenu about that other question meanwhile.
> Would you mind pinging me when we're all on IRC?

I'll see what I can do.

Comment 19

[[e-mail]]

OK, it looks like I oversimplified. The Drop code drops all .eml files. The CanDrop returns true if the first item is a file. The two are not the same.

Comment 20

[Karsten Düsterloh]

Comment on attachment 439820 [details] [diff] [review]
complete proof of concept

Given that your announced local code should be fairly different from this, I'll skip this instance.

Comment 21

[Jens Hatlak (:InvisibleSmiley)]

Attachment: complete patch

This is the first complete patch (i.e. including removing the old code).

Changes in addition to addressing review comments (I hope I don't forget anything...):
- reordered the ifs in CanDropOnFolderTree according to TB's canDrop
- since we're already changing the whole file I took the liberty to wrap the dragService variable definition, too
- applied several of Philip's suggestions from <http://pastebin.mozilla.org/720706> (thanks!), i.e. actually all except:
  * the DropOnFolderTree text/x-moz-folder loop change which didn't work out as expected, d&d moving of multiple folders didn't work anymore. bienvenu said on #maildev: "I'm not sure...I doubt that folders of different types can be copied at the same time..."
  * the prefBranch -> pref change (didn't check where "pref" would be imported from)
  * the nsIMutableArray change (no point IMHO)
- added gIOService lazy initialization like in msgHdrViewOverlay.js's setFromBuddyIcon(). That file contains the gIOService variable. The file is included by msgHdrViewOverlay.xul which is included by mailWindowOverlay.xul which is included by messenger.xul and messageWindow.xul (main mail window and standalone mail window files).
- gCopyService is contained in mailWindowOverlay.js which is included by mailWindowOverlay.xul (see above) and SearchDialog.xul (Advanced Search dialog)
- I'm still not using extractScheme. See the previous comments for why I'm hesitant to do this. Maybe you can forgive me that. Or provide ready code. ;-)

Again anyone's invited to test the patch.

Comment 22

[Robert Kaiser]

for pref and io services, we really should make sure Services.jsm is included in the window scope and just use Services.prefs and Services.io for getting those. This has lazy getters and does caching across all places where the services are used.

Comment 23

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #22)
> for pref and io services, we really should make sure Services.jsm is included
> in the window scope

mailWindowOverlay.js seems to be a good place for that (see comment 21) and it already contains another jsm import (folderUtils). Mnyromyr, Neil, are you OK with that?

Comment 24

[Jens Hatlak (:InvisibleSmiley)]

FTR: Services.io is nsIIOService2 while the last patch uses nsIIOService. Shouldn't be a problem, though, since the former just extends the latter (without changing any of the existing functionality). Correct me if I'm wrong.

Comment 25

[Karsten Düsterloh]

> mailWindowOverlay.js seems to be a good place for that (see comment 21) and it
> already contains another jsm import (folderUtils). Mnyromyr, Neil, are you OK
> with that?

Sounds reasonable.

Comment 26

[Jens Hatlak (:InvisibleSmiley)]

Attachment: complete patch v1a

new in this version:
- introduced Services for .io and .prefs as proposed by KaiRo, obsoleting gIOService
- moved "mail." prefix into the two pref strings to get rid of prefBranch (previously proposed by Philip, now actually makes sense with Services.prefs shorthand)
- finally used extractScheme (and regex for testing). Wasn't all that hard in the end. ;-)

That's it from my side now. Hopefully.

Comment 27

[[e-mail]]

Comment on attachment 443439 [details] [diff] [review]
complete patch v1a

> Components.utils.import("resource:///modules/folderUtils.jsm");
>+Components.utils.import("resource://gre/modules/Services.jsm");
Nit: not sure whether we've been consistent here before but if we have then I'd like to import gre modules first.
>+var dragService = Components.classes["@mozilla.org/widget/dragservice;1"]
>+                            .getService()
>+                            .QueryInterface(Components.interfaces.nsIDragService);
> var nsIDragService = Components.interfaces.nsIDragService;
As you're changing this, you can simplify it to .getService(nsIDragService)
(obviously moving it after the var for maximum effect!)

>-  // uncomment for noise
[Ironic, because this is a noise deletion in the patch...]

>+    if (/https?/.test(scheme) && targetFolder.server.type == "rss")
Nit: your regexp needs some ^$ love

>+      let folders = new Array;
>+      folders.push(dt.mozGetDataAt("text/x-moz-folder", i)
>+                     .QueryInterface(Components.interfaces.nsIMsgFolder));
>+      let array = toXPCOMArray(folders, Components.interfaces.nsIMutableArray);
I still don't like this way of creating an nsIArray, but at the very least you can simplify the first two lines like this:
folders = [dt.mozGetDataAt("text/x-moz-folder", i)]
because an nsIArray uses nsISupports internally anyway.

>+                               (folders[0].server == targetFolder.server),
[Does the drop effect update automatically for this, or do we just guess?]

>+    for (let i = 0; i < count; i++)
>+    {
>+      let folder = dt.mozGetDataAt("text/x-moz-newsfolder", i)
>+                     .QueryInterface(Components.interfaces.nsIMsgFolder);
>+      newsFolder.moveFolder(folder, targetFolder, aOrientation);
[Not sure this works if count > 1 since the folders might reverse order]

>+    let array = Components.classes["@mozilla.org/array;1"]
>+                          .createInstance(Components.interfaces.nsIMutableArray);
See, it wasn't too hard, was it ;-)

>+      if (!i)
>+        sourceFolder = msgHdr.folder;
[if (!sourceFolder) looks tempting]

Comment 28

[Jens Hatlak (:InvisibleSmiley)]

[changed locally what I didn't quote]

(In reply to comment #27)
> >+                               (folders[0].server == targetFolder.server),
> [Does the drop effect update automatically for this, or do we just guess?]

We don't allow copy in this case at all:

      // Don't copy within same server.
      if (folder.server == targetFolder.server && dt.dropEffect == 'copy')
        return false;

> >+    for (let i = 0; i < count; i++)
> >+    {
> >+      let folder = dt.mozGetDataAt("text/x-moz-newsfolder", i)
> >+                     .QueryInterface(Components.interfaces.nsIMsgFolder);
> >+      newsFolder.moveFolder(folder, targetFolder, aOrientation);
> [Not sure this works if count > 1 since the folders might reverse order]

I could successfully move two newsgroups at the same time. Does that answer your question or are you concerned about the ordering before/after the d&d here rather than that moving works at all?

> >+    let array = Components.classes["@mozilla.org/array;1"]
> >+                          .createInstance(Components.interfaces.nsIMutableArray);
> See, it wasn't too hard, was it ;-)

Hmm? I think you quoted the wrong lines here and were trying to talk about extractScheme, right? Otherwise I don't get it.

> >+      if (!i)
> >+        sourceFolder = msgHdr.folder;
> [if (!sourceFolder) looks tempting]

Since I copied that from TB (yeah, I know...) I wonder what they were thinking. This is basically equivalent to if (i==0), right? I take it you want me to change it to if (!sourceFolder) rather than adding another check after the existing one (and do what?)?

Comment 29

[[e-mail]]

(In reply to comment #28)
> (In reply to comment #27)
> > >+                               (folders[0].server == targetFolder.server),
> > [Does the drop effect update automatically for this, or do we just guess?]
> We don't allow copy in this case at all
Great!

> > [Not sure this works if count > 1 since the folders might reverse order]
> I could successfully move two newsgroups at the same time. Does that answer
> your question or are you concerned about the ordering before/after the d&d here
> rather than that moving works at all?
I was concerned about the ordering. In particular the results if you drop "after" a newsgroup or "before" the following newsgroup might be different.

> > >+    let array = Components.classes["@mozilla.org/array;1"]
> > >+                          .createInstance(Components.interfaces.nsIMutableArray);
> > See, it wasn't too hard, was it ;-)
> Hmm? I think you quoted the wrong lines here and were trying to talk about
> extractScheme, right?
No, I was referring to the use of toXPCOMArray.

> This is basically equivalent to if (i==0), right?
Yeah, I was just wondering which version looks more readable.

Comment 30

[Robert Kaiser]

(In reply to comment #24)
> FTR: Services.io is nsIIOService2 while the last patch uses nsIIOService.
> Shouldn't be a problem, though, since the former just extends the latter
> (without changing any of the existing functionality). Correct me if I'm wrong.

Right. Services.jsm does that for all services that have an extended *2 interface for "frozen API" reasons. One usually doesn't need to be concerned with that and we can consistently use everything through the *2 interfaces without any problems.

Comment 31

[Jens Hatlak (:InvisibleSmiley)]

Attachment: dnd v2

(In reply to comment #29)
> > > [Not sure this works if count > 1 since the folders might reverse order]
> > I could successfully move two newsgroups at the same time. Does that answer
> > your question or are you concerned about the ordering before/after the d&d here
> > rather than that moving works at all?
> I was concerned about the ordering. In particular the results if you drop
> "after" a newsgroup or "before" the following newsgroup might be different.

As discussed on IRC, adapted the TB way now which takes care of correct sorting.

At first this resulted in an endless loop. aOrientation was always 0. Then I noticed that the drop marker didn't appear between NGs anymore but on NGs. Further investigation showed that one of Philip's optimizations, the extraction of the aOrientation != DROP_ON check, was wrong but actually my fault: It's actually != in all cases except NG d&d where it's ==. I remembered that I had copied the comment from the NG d&d to the other cases together with the different code (so the code was correct but the comments were not) when I ported what TB has.

Anyway, now it's fixed (code moved to where it was before), with new comments.

> > > >+    let array = Components.classes["@mozilla.org/array;1"]
> > > >+                          .createInstance(Components.interfaces.nsIMutableArray);
> > > See, it wasn't too hard, was it ;-)
> > Hmm? I think you quoted the wrong lines here and were trying to talk about
> > extractScheme, right?
> No, I was referring to the use of toXPCOMArray.

As discussed on IRC I got rid of toXPCOMArray().

> > This is basically equivalent to if (i==0), right?
> Yeah, I was just wondering which version looks more readable.

Now that I got what you meant I agreed and used !sourceFolder instead of "!i-which-actually-is-used-as-i==0".

Additionally I replaced another occurrence of Components.interfaces.nsIDragService with the variable defined at the beginning of the file.


The only other thing I noticed was that you can d&d a feed to any folder inside an RSS account. The feed is added as a new folder one level below the account in any case, though. I think we shouldn't further complicate things and just let that slip to offer people more drop targets (just my POV of course).

Comment 32

[[e-mail]]

Comment on attachment 443746 [details] [diff] [review]
dnd v2

>+    // Start by getting folders into order.
[Strange, I would have thought that the folders would already be in order.]

>+    let folders = new Array;
>+    for (let i = 0; i < count; i++) {
>+      let folder = dt.mozGetDataAt("text/x-moz-newsfolder", i)
>+                     .QueryInterface(Components.interfaces.nsIMsgFolder);
>+      let folderIndex = EnsureFolderIndex(folderTree.builderView, folder);
>+      folders[folderIndex] = folder;
>+    }
>+    let newsFolder = targetFolder.rootFolder
>+                                 .QueryInterface(Components.interfaces.nsIMsgNewsFolder);
>+    // When moving down, want to insert first one last.
>+    // When moving up, want to insert first one first.
>+    let i = (aOrientation == 1) ? folders.length - 1 : 0;
>+    while (i >= 0 && i < folders.length) {
>+      let folder = folders[i];
>+      if (folder) {
[So I would written something like this:
  let newsFolder = targetFolder.rootFolder
                               .QueryInterface(...);
  // When moving down, want to insert first one last.
  // When moving up, want to insert first one first.
  let i = (aOrientation == 1) ? count - 1 : 0;
  while (i >= 0 && i < count) {
    let folder = dt.mozGetDataAt("text/x-moz-newsfolder", i)
                   .QueryInterface(Components.interfaces.nsIMsgFolder);
etc.]

Comment 33

[[e-mail]]

Comment on attachment 443746 [details] [diff] [review]
dnd v2

>+var dragService = Components.classes["@mozilla.org/widget/dragservice;1"]
>+                            .getService().QueryInterface(nsIDragService);
Didn't I say .getService(nsIDragService)? sr=me with that fixed.

>+    // Don't allow dragging onto element.
Perhaps // Only allow dragging between newsgroups.

>+    // When moving down, want to insert first one last.
I think "insert last one first" would make slightly more sense.

Comment 34

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #33)
> (From update of attachment 443746 [details] [diff] [review])
> >+var dragService = Components.classes["@mozilla.org/widget/dragservice;1"]
> >+                            .getService().QueryInterface(nsIDragService);
> Didn't I say .getService(nsIDragService)?

Seems I missed that you actually requested two changes at once there. Fixed locally.

> sr=me with that fixed.

Thanks for bearing with me! :-)

> >+    // Don't allow dragging onto element.
> Perhaps // Only allow dragging between newsgroups.

I chose "onto newsgroup" instead now (less ambiguous).

> >+    // When moving down, want to insert first one last.
> I think "insert last one first" would make slightly more sense.

Agreed.


Regarding the loop optimizations suggested in comment 32 I don't think that'll work. As I understand it the code from my patch first creates an array with folder indices (e.g. 3, 14, 29) and walks through that in a second loop (e.g. 0-29 or 29-0). Your suggestion just walks through the elements of the transferable (e.g. 0-2 or 2-0) without caring about the folder indices (order).

If you'd still like to see this evaluated or changed maybe it's an option to talk with IanN who implemented the code I ported in bug 473001.


@Mnyromyr: Since you seem to be busy, would it be an option to hand the review request over to IanN? I'd really like to get this into 2.1a1 if still possible.

Comment 35

[Karsten Düsterloh]

Comment on attachment 443746 [details] [diff] [review]
dnd v2

First of all, dragging folder into Trash and canceling the confirmation dialog will result in

Error: uncaught exception: [Exception... "Component returned failure code: 0x8055001a [nsIMsgCopyService.CopyFolders]" nsresult: "0x8055001a (<unknown>)" location: "JS frame :: chrome://messenger/content/messengerdnd.js :: DropOnFolderTree :: line 204" data: no]

which is intended by the backend, so we need to catch this.


One other comment:

>+++ b/suite/mailnews/messengerdnd.js
>+      // Don't copy within same server.
>+      if (folder.server == targetFolder.server && dt.dropEffect == 'copy')

For consistency, please use "" instead of '' here.

Comment 36

[Jens Hatlak (:InvisibleSmiley)]

Attachment: dnd v3 [Checkin: comment 37]

(In reply to comment #35)
> (From update of attachment 443746 [details] [diff] [review])
> First of all, dragging folder into Trash and canceling the confirmation dialog
> will result in
> 
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x8055001a [nsIMsgCopyService.CopyFolders]" nsresult: "0x8055001a (<unknown>)"
> location: "JS frame :: chrome://messenger/content/messengerdnd.js ::
> DropOnFolderTree :: line 204" data: no]
> 
> which is intended by the backend, so we need to catch this.

Similarly for the second case I found.

I borrowed the catch syntax from sidebarOverlay.js, found the constants in msgCore.h.

Comment 37

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 444768 [details] [diff] [review]
dnd v3 [Checkin: comment 37]

http://hg.mozilla.org/comm-central/rev/ef1c30b97714