465015 - drag+drop of message not blocked to its originating folder

Status: VERIFIED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Wayne Mery (:wsmwk)]

artifact/regression of the new non-rdf folder pane?  

hourly build Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081114 Shredder/3.0b1pre ID:20081114144033

Comment 1

[Ian Neal]

Confirming, new code does not check this.

Comment 2

[Ian Neal]

Attachment: Add various checks for folder drop patch v0.1

This patch adds back checks for:
* Dropping onto self
* Dropping onto immediate parent
* Dropping onto ancestor
* Dropping virtual folders across accounts

Comment 3

[Ian Neal]

There are probably still other checks for newsfolders that need to be done but I'm not sure that we even allow those to be copied in the new code.

Comment 4

[Ian Neal]

There are still more checks missing probably:
http://mxr.mozilla.org/comm-central/source/mailnews/base/resources/content/messengerdnd.js#193

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 357254 [details] [diff] [review]
Add various checks for folder drop patch v0.1

I'm getting a bunch of 

Error: folder.parent is undefined
Source File: chrome://messenger/content/folderPane.js
Line: 359

errors when hovering over or somewhere in between the parent folders. (IMAP account, second level) Fairly reproducible especially at the start of a session, but doesn't happen always.

I wonder if it's possible to compare the folders directly, or do you need to go through the URI?

Comment 6

[Ian Neal]

Does this happen immediately or about 5 or 6 seconds after you start trying to drag and drop?

Comment 7

[Magnus Melin [:mkmelin]]

Almost immediately. It's easiest to reproduce by dragging back and forwards over a folder hierarchy. Then it stops, but you can see it again by testing with another hierarchy (where it also occurs) and then coming back to the first one again.

Comment 8

[Ian Neal]

Attachment: Add various checks for folder drop patch v0.1a

Changes since v0.1:
* Adds check for existence of folder.parent before trying to get its URI.
* Adds check that targetFolder can create subdirectories before allowing drop.
* Adds check that not renaming a folder that is not allowed to be renamed.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 357559 [details] [diff] [review]
Add various checks for folder drop patch v0.1a

Now I'm getting a bunch of 
Error: folder.isAncestorOf is not a function
Source File: chrome://messenger/content/folderPane.js
Line: 373

instead.

So with this I wouldn't be able to copy Templates/Drafts etc to another account? I'd like that to work. With the patch I am still able to drop Junk inside the same account - which probably shouldn't work.

Did directly comparing folder objects work? (Instead of URIs)

I think it's not necessary to have all the vars for usage once or twice.

Comment 10

[Ian Neal]

(In reply to comment #9)
> (From update of attachment 357559 [details] [diff] [review])
> Now I'm getting a bunch of 
> Error: folder.isAncestorOf is not a function
> Source File: chrome://messenger/content/folderPane.js
> Line: 373
> 
Does this happens after about 5 or 6 seconds or does it happen immediately?

Comment 11

[Ian Neal]

(In reply to comment #9)
> (From update of attachment 357559 [details] [diff] [review])
> Did directly comparing folder objects work? (Instead of URIs)
Directly comparing folder objects instead of URIs does not appear to work.

Comment 12

[Ian Neal]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 357559 [details] [diff] [review] [details])
> > Now I'm getting a bunch of 
> > Error: folder.isAncestorOf is not a function
> > Source File: chrome://messenger/content/folderPane.js
> > Line: 373
> > 
> Does this happens after about 5 or 6 seconds or does it happen immediately?

For some reason after 5 or 6 seconds the folder objects with this._currentTransfer become null so things like folder.prettyName returns undefined.
You can see this in evidence without the patch by trying to drag and drop a folder.
If you drag and drop a folder quickly (i.e. taking less than 5 seconds), then it gets moved to where you want it moving.
If you drag and drop a folder less quickly (i.e. dropping it about 10 seconds after starting the drag), then it gets copied rather than moved.
If you put a dump in the drop code you can see for the first case the folder still has its prettyName but in the second case it is undefined.

Comment 13

[Ian Neal]

Oh and the reason why it gets copied rather than moved is that folders[0].server becomes undefined thus cs.CopyFolders thinks you are dragging and dropping between servers rather than on the same server.

Comment 14

[Ian Neal]

Raised bug 474238 to do with this bug in "Drag and Drop"

Comment 15

[Magnus Melin [:mkmelin]]

It happens almost immediately (not a question of seconds). The test folder just have a few mails FWIW.

Comment 16

[Magnus Melin [:mkmelin]]

If I make sure the the clipboard folder

        let tmp = dt.mozGetDataAt("text/x-moz-folder", i);
        tmp.QueryInterface(Components.interfaces.nsIMsgFolder);

... I don't get the error. 

I'm not sure why that is needed some of the time and not always.

Comment 17

[Magnus Melin [:mkmelin]]

( before folders.push(dt.mozGetDataAt("text/x-moz-folder", tmp)); )

Comment 18

[Ian Neal]

Attachment: Add various checks for canDrop patch v0.1b

Changes since v0.1a:
* As suggested make folders nsIMsgFolder objects.
* Compare folders rather than URIs.
* Add test for .isServer to message canDrop.
* Fixes bugs named issue - adds check for dropping message on originating folder.

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 359993 [details] [diff] [review]
Add various checks for canDrop patch v0.1b

>+      let targetServer = targetFolder.server;
>+      let targetServerLocal = targetServer.type == "none";

Please inline these too as that's easier to read than having the tmp variables.

>+        // If there is a folder that can't be renamed, allow it to be dropped
>+        // only if it is to "Local Folders" and not to the same server. 

Trailing space.

Seems to be working, except for the special folders. I can still drag around outbox, sent etc, but that's not supposed to work, right?

Comment 20

[Ian Neal]

Attachment: Add various checks for canDrop patch v0.1c

Changes since v0.1b:
* Fix issue with moving of special folders.
* Inline some variables as requested.
* Tidy up comment.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 360003 [details] [diff] [review]
Add various checks for canDrop patch v0.1c

Works fine now, thx for fixing this!

I'd prefer inlining let count = dt.mozItemCount;
 too, but r=mkmelin either way.

Comment 22

[Ian Neal]

Attachment: Add various checks for canDrop patch v0.1d

Changes since v0.1c:
* Inlining dt.mozItemCount.

Carrying forward r+ and requesting sr.

Comment 23

[[e-mail]]

Comment on attachment 360008 [details] [diff] [review]
Add various checks for canDrop patch v0.1d

>+    let dropOkay = true;
This variable is pointless. Simply return false as soon as you work out that you can't drop. You do that in several places already, after all..

>+      let i = 0;
>+      while (i < dt.mozItemCount && dropOkay) {
Why not a for loop?

>+      let folders = new Array;
>+      for (let i = 0; i < dt.mozItemCount; i++)
>+        folders.push(dt.mozGetDataAt("text/x-moz-folder", i)
>+                       .QueryInterface(Ci.nsIMsgFolder));
>+      let j = 0;
>+      while (j < folders.length && dropOkay) {
>+        let folder = folders[j];
Why not just
for (let i = 0; i < dt.mozItemCount; i++) {
  var folder = dt.mozGetDataAt("text/x-moz-folder", i)
                 .QueryInterface(Ci.nsIMsgFolder));

>+        if (folder.parent && targetFolder == folder.parent)
Don't need to null-check folder.parent because targetFolder won't be null.

>+    if (aOrientation != Ci.nsITreeView.DROP_ON)
>       return false;
Why not do this first, it's a simple test.

Comment 24

[Ian Neal]

Attachment: Add various checks for canDrop patch v0.1e (Pushed: Comment 25)

Changes since v0.1d:
* Remove use of dropOkay as per sr.
* Tidy up loops as per sr.

Carrying forward r/sr.

Comment 25

[Ian Neal]

Comment on attachment 360023 [details] [diff] [review]
Add various checks for canDrop patch v0.1e (Pushed: Comment 25)

http://hg.mozilla.org/comm-central/rev/1fb281cf8e06

Comment 26

[Wayne Mery (:wsmwk)]

v.fixed - can't drop to self/originating folder, or virtual folder on any account. no console errors

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090202 Shredder/3.0b2pre