Closed Bug 465015 Opened 13 years ago Closed 12 years ago

drag+drop of message not blocked to its originating folder

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
minor

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 3.0b2

People

(Reporter: wsmwk, Assigned: iann_bugzilla)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 5 obsolete files)

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
Confirming, new code does not check this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows Vista → All
Hardware: x86 → All
Blocks: 414038
Keywords: regression
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Depends on: 467994
This patch adds back checks for:
* Dropping onto self
* Dropping onto immediate parent
* Dropping onto ancestor
* Dropping virtual folders across accounts
Attachment #357254 - Flags: review?(mkmelin+mozilla)
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.
Attachment #357254 - Flags: review?(mkmelin+mozilla) → review-
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?
Does this happen immediately or about 5 or 6 seconds after you start trying to drag and drop?
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.
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.
Attachment #357254 - Attachment is obsolete: true
Attachment #357559 - Flags: review?(mkmelin+mozilla)
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.
Attachment #357559 - Flags: review?(mkmelin+mozilla) → review-
(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?
(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.
(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.
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.
Raised bug 474238 to do with this bug in "Drag and Drop"
Depends on: 474238
It happens almost immediately (not a question of seconds). The test folder just have a few mails FWIW.
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.
( before folders.push(dt.mozGetDataAt("text/x-moz-folder", tmp)); )
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.
Attachment #357559 - Attachment is obsolete: true
Attachment #359993 - Flags: review?(mkmelin+mozilla)
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?
Attachment #359993 - Flags: review?(mkmelin+mozilla)
Changes since v0.1b:
* Fix issue with moving of special folders.
* Inline some variables as requested.
* Tidy up comment.
Attachment #359993 - Attachment is obsolete: true
Attachment #360003 - Flags: review?(mkmelin+mozilla)
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.
Attachment #360003 - Flags: review?(mkmelin+mozilla) → review+
Changes since v0.1c:
* Inlining dt.mozItemCount.

Carrying forward r+ and requesting sr.
Attachment #360003 - Attachment is obsolete: true
Attachment #360008 - Flags: superreview?(neil)
Attachment #360008 - Flags: review+
Blocks: 473001
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.
Attachment #360008 - Flags: superreview?(neil) → superreview+
Changes since v0.1d:
* Remove use of dropOkay as per sr.
* Tidy up loops as per sr.

Carrying forward r/sr.
Attachment #360008 - Attachment is obsolete: true
Attachment #360023 - Flags: superreview+
Attachment #360023 - Flags: review+
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
Attachment #360023 - Attachment description: Add various checks for canDrop patch v0.1e → Add various checks for canDrop patch v0.1e (Pushed: Comment 25)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b2
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
Status: RESOLVED → VERIFIED
Summary: drag+drop of message not blocked to it's originating folder → drag+drop of message not blocked to its originating folder
You need to log in before you can comment on or make changes to this bug.