Closed
Bug 53403
Opened 24 years ago
Closed 24 years ago
Dragging folder into child folder deletes folder
Categories
(SeaMonkey :: Bookmarks & History, defect, P1)
SeaMonkey
Bookmarks & History
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.1
People
(Reporter: devsin, Assigned: morse)
References
Details
(Keywords: dataloss, Whiteboard: waiting for 42080 to verify)
Attachments
(6 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Mozilla M18 2000092008, Mac OS 9.0.4 -
1) Open Manage Bookmarks window
2) Select a folder
3) Drag the folder onto itself (like you were going to move the folder inside
another folder)
The entire folder is deleted. Hopefully you have a backup of your Bookmarks file.
Comment 1•24 years ago
|
||
Holy data loss batman! Such a small gesture, such a completly gone, nonrecoverable
bookmarks file!
don who gets this???
That's right Robin! It looks like the Joker is up to something evil again!
Please tell me we can fix this in RTM?!? Claudius, would you hold beta 3 for this?
Assignee: don → ben
Priority: P3 → P1
Whiteboard: [nsbeta3-][rtm+]
Assignee | ||
Comment 3•24 years ago
|
||
Assignee | ||
Comment 4•24 years ago
|
||
Based on the patch being so simple and safe, and the bug being so devastating, I
would recommend getting this into beta3. But I'll leave that for ben and don to
argue for.
Comment 5•24 years ago
|
||
Shouldn't that return be return false; instead? That's what the final return in
the function does. I can't tell, due to the lamely screwy indentation and tabs
in the file, where the for loop ends, at least when reading the patch via
bugzilla. But if you just want to exit the loop, and run whatever's after the
loop body, then break; rather than return false; might be even better.
/be
Assignee | ||
Comment 6•24 years ago
|
||
Yes, return false is the more correct thing to do. But this routine never
returns anything but false so apparently the return value is never used.
As far as using a break instead of a return, that's even better. The code after
the loop does some processing if "dirty" is true. And "dirty" will be true if
the loop had been executed at least once. So if several items were selected to
be dragged and at least one of them was not being dragged onto itself, then
"dirty" will get set and we would want to do the final processing.
Finally, there's another test just after which probably needs the same fix. It
tests for dropping a node onto its parent container. I'm not sure what problem
the current coding for that test is causing, but it is incorrect.
Attaching a revised patch.
Assignee | ||
Comment 7•24 years ago
|
||
Assignee | ||
Comment 8•24 years ago
|
||
Forget preceding patch -- it doesn't work. Seems like the deletion occurs
unless we actually do a return. (I posted the patch before I tested it --
sorry.)
So now I'm attaching the third (and hopefully final) patch.
Assignee | ||
Comment 9•24 years ago
|
||
Comment 10•24 years ago
|
||
Plussed again for beta 3! :-)
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3+][rtm+]
Comment 12•24 years ago
|
||
nav triage team:
we wanted this in beta3, but too late now. nsbeta3-
Whiteboard: [nsbeta3+][rtm+] → [nsbeta3-][rtm+]
Comment 13•24 years ago
|
||
Vera: release note
Comment 14•24 years ago
|
||
PDT marking [rtm need info] since we'd like a fix for this after the patch has
had code reviews.
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm need info]
Comment 15•24 years ago
|
||
Nav triage team: Serious data loss; remains P1.
Comment 16•24 years ago
|
||
Poking around looking at release note items, I found a similar bug that seems to
have slipped through the cracks. See bug 47146.
Assignee | ||
Comment 17•24 years ago
|
||
*** Bug 47146 has been marked as a duplicate of this bug. ***
Comment 18•24 years ago
|
||
Can someone review and approve this, please!!!
Whiteboard: [nsbeta3-][rtm need info] → [nsbeta3-][rtm need info]Fix in hand
Comment 20•24 years ago
|
||
PDT, this fix from Steve Morse has now been reviewed and approved by Ben (even
though he will check in Steve's patch). Please mark this "++".
Whiteboard: [nsbeta3-][rtm need info]Fix in hand → [nsbeta3-][rtm+]Fix in hand, reviewed and approved
Comment 21•24 years ago
|
||
PDT marking [rtm need info] because the only reviewer is an sr. Need to have two
reviews for all patches.
Whiteboard: [nsbeta3-][rtm+]Fix in hand, reviewed and approved → [nsbeta3-][rtm need info]Fix in hand, reviewed and approved
Comment 22•24 years ago
|
||
I've got problems with this code. The patch itself is pretty simple, but the
loop is big and hairy and living in an even bigger and hairier function. I
understand how the patch stops the crash, but in the case of a drag of multiple
items it can leave the drag half-done. On close inspection of the function with
help from Ben Goodger, it seems that there is another more promising site within
the function that may be causing the bug... I'll let him comment further. In
the meantime, I don't think we can let this patch in as it stands.
Comment 23•24 years ago
|
||
Nice catch, working on new fix ...
Whiteboard: [nsbeta3-][rtm need info]Fix in hand, reviewed and approved → [nsbeta3-][rtm need info]Working on new fix
Assignee | ||
Comment 24•24 years ago
|
||
Drag of multiple bookmarks isn't possible. If you select multiple bookmarks and
try to drag and drop them, only one of the bookmarks actually gets dropped.
(This is probably some other bug.)
So the patch presented here is perfectly safe and is the simplest, lowest-risk
patch to fix the current bug. Anything more involved (such as restructuring the
code to prepare for the day when drag-and-drop of multiple bookmarks works,
should be considered for the tip but not the branch.
Assignee | ||
Comment 25•24 years ago
|
||
I just opend bug 55678 on the inability to drag and drop items in the
manage-bookmark dialog.
Assignee | ||
Comment 26•24 years ago
|
||
correction to above comment. I meant to say "inability to drag and drop
MULTIPLE items in the manage-bookmarks dialog."
Reporter | ||
Comment 27•24 years ago
|
||
See bug 42080 for problems with drag and drop of multiple bookmarks/folders.
Comment 28•24 years ago
|
||
Steve, the bug number you cite (the one you say you wrote) is incorrect. Regardless, that
bug, wherever it may be is a dupe of bug 42080 (can't drag mult. bm's)
Assignee | ||
Comment 29•24 years ago
|
||
Oops, I meant bug 55768, not 55678
Comment 30•24 years ago
|
||
Miunsing this. Sorry. Ben just doesn't think he can find the right fix for
this in time for N6 RTM.
Whiteboard: [nsbeta3-][rtm need info]Working on new fix → [nsbeta3-][rtm-]Working on new fix
Comment 31•24 years ago
|
||
Can we stop the crash on the branch with a hack, and then leave the bug open for
the trunk, dependent on the multi drag/drop bug? Then the forward-looking trunk
doesn't get polluted by the hack. What say, Scott?
Comment 32•24 years ago
|
||
isn't there at least a dirty hack to disallow a drop of this kind?
Comment 33•24 years ago
|
||
OK, given that drag of multiple items currently doesn't work (_and_ that a bug
has been filed on that) I can give this a cautious sr=scc for the branch _only_.
I hope this helps.
Comment 34•24 years ago
|
||
In that case, moving back to rtm+ so PDT can approve.
Whiteboard: [nsbeta3-][rtm-]Working on new fix → [nsbeta3-][rtm+]Fix (hack) in hand for branch-only, reviewed/approved
Comment 35•24 years ago
|
||
rtm++ for this patch on branch only, as scc said.
Whiteboard: [nsbeta3-][rtm+]Fix (hack) in hand for branch-only, reviewed/approved → [nsbeta3-][rtm++]Fix (hack) in hand for branch-only, reviewed/approved
Comment 36•24 years ago
|
||
fix checked in to branch only.
marking rtm-, leaving open to find real fix once drag of multiple items is
repaired.
Whiteboard: [nsbeta3-][rtm++]Fix (hack) in hand for branch-only, reviewed/approved → [nsbeta3-][rtm-] Need to find real fix
Comment 37•24 years ago
|
||
Updated status whiteboard to show that a hack was put into NS6 to make sure that
dragging a folder on top of itself did not delete the folder.
Personally, I think we should get rid of the rtm keywords and the status
whiteboard markings in these kinds of situations after we've made the rtm fix.
Whiteboard: [nsbeta3-][rtm-] Need to find real fix → [nsbeta3-][rtm-] Hacked fix checked into NS6, Need to find real fix
Comment 38•24 years ago
|
||
This no longer needs a relnote, right? It's fixed on the branch...
Just in case, here it is again. All sing:
It seems unclear to me whether this bug requires either of a "developer" or
"user" release note for Netscape 6 RTM. If anyone feels it does, can they please
draft one and then nominate with the relnote-user or relnote-devel strings in
the Status Whiteboard.
Thanks :-)
Gerv
Comment 39•24 years ago
|
||
*** Bug 58400 has been marked as a duplicate of this bug. ***
Comment 40•24 years ago
|
||
This is an edge case for the release notes... but I'll keep it in. Marking
"relnote-user" in whiteboard.
Whiteboard: [nsbeta3-][rtm-] Hacked fix checked into NS6, Need to find real fix → [nsbeta3-][rtm-][relnote-user] Hacked fix checked into NS6, Need to find real fix
Assignee | ||
Comment 41•24 years ago
|
||
vera, there's nothing to release note. There is no way that the user can run
into this problem with the current branch build.
Comment 42•24 years ago
|
||
*** Bug 59175 has been marked as a duplicate of this bug. ***
Comment 43•24 years ago
|
||
*** Bug 60716 has been marked as a duplicate of this bug. ***
Comment 44•24 years ago
|
||
nav triage team:
Nominate for nsbeta1. Check patch onto trunk, then wait for multiple bm drags to
fix real problem
Comment 45•24 years ago
|
||
*** Bug 62850 has been marked as a duplicate of this bug. ***
Comment 46•24 years ago
|
||
*** Bug 64779 has been marked as a duplicate of this bug. ***
Comment 47•24 years ago
|
||
this bug is fixed correctly by my patches to 64723 and 64722
Comment 48•24 years ago
|
||
*** Bug 62172 has been marked as a duplicate of this bug. ***
Updated•24 years ago
|
Target Milestone: --- → mozilla0.8
Updated•24 years ago
|
Comment 49•24 years ago
|
||
Fix checked in (this is ben, using hyatt's machine, oops)
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 50•24 years ago
|
||
*** Bug 71636 has been marked as a duplicate of this bug. ***
Comment 51•24 years ago
|
||
Reopening
It happens again in 2001031008 win98
and on win 2000 ( see bug 71636 )
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 52•24 years ago
|
||
I didn't see this with dragging a folder onto itself, but I do see it dragging a
folder onto a child folder. Accepting and retargeting at .9
Status: REOPENED → ASSIGNED
Summary: Dragging folder onto itself deletes folder → Dragging folder into child folder deletes folder
Target Milestone: mozilla0.8 → mozilla0.9
Comment 53•24 years ago
|
||
clearing status/keyword cruft.
Comment 54•24 years ago
|
||
nav triage team:
Marking nsbeta1+
Comment 55•24 years ago
|
||
ack! this explains my problems in the personal toolbar. setting up dependancies.
Blocks: 46456
Comment 56•24 years ago
|
||
alas, this is ongoing. tossing into .9.1
Target Milestone: mozilla0.9 → mozilla0.9.1
Assignee | ||
Comment 57•24 years ago
|
||
This one has been around too long. Attaching patch.
Assignee | ||
Comment 58•24 years ago
|
||
Assignee | ||
Comment 59•24 years ago
|
||
Assignee | ||
Comment 60•24 years ago
|
||
ben and alecf, please review and super-review. Thanks.
Comment 61•24 years ago
|
||
Thanks for the patch, steve, I'll take a look at this tomorrow.
Comment 62•24 years ago
|
||
If I understand this correctly, this only fires if the item dropped on is the
container that we're going to insert the bookmarks into? (the case in which
containerItem == dropItem, or rTarget == rContainer).
I can see how this would prevent you from dropping items onto the toplevel
container, but imagine this scenario:
Take a deeply nested folder structure: say three deep. Add several dummy
bookmarks, or just use the default bookmarks file provided with new profiles, I
think there are cases in that. Drag a toplevel folder into a subfolder nested
several levels down.
I see you doing what appears to be a climb up the document, which is probably
what is required, but only in the case where the dropItem == containerItem.
Am I missing something?
Assignee | ||
Comment 63•24 years ago
|
||
Do you have a test case that demonstrates the problem you are now describing?
Is so, then can we simply remove the "if (rTarget == rContainer)" part of the
patch to cover that case?
Comment 64•24 years ago
|
||
ok, I'll sr=alecf that for moz 0.9 but not for the trunk - we need to fix the
real problem and it's pretty evil not to allow you to drop something into one if
it's own subfolders..
Assignee | ||
Comment 65•24 years ago
|
||
Alec, I don't understand. What is the "real problem" as opposed to the problem
that my patch (without the enclosing "if") fixes?
Comment 66•24 years ago
|
||
oh I misread this - I thought this was that dragging bookmarks into a subfolder
at the same or lower level was causing destruction (because I am seeing that in
the personal toolbar)
nevermind, I'll defer to ben on this.
Comment 67•24 years ago
|
||
Yes, without the check it works. How about this as a simplification?:
// Climb up the document to ensure that we're not going to
// drop this item anywhere into itself.
do {
var targetAncestor = NODE_ID(dropItem);
dropItem = dropItem.parentNode;
}
while (targetAncestor != "NC:BookmarksRoot" && targetAncestor != sourceID);
if (targetAncestor == sourceID)
continue;
Assignee | ||
Comment 68•24 years ago
|
||
It looks like its equivalent. If you've tested it out and it works in all
cases, then it probably is. r=morse
Assignee | ||
Comment 69•24 years ago
|
||
alecf, please super-review ben's modification to the patch. Thanks.
Comment 70•24 years ago
|
||
much clearer... sr=alecf
Comment 71•24 years ago
|
||
Great, thanks for debugging this steve!
Assignee | ||
Comment 73•24 years ago
|
||
fix checked in
Status: NEW → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Comment 74•24 years ago
|
||
it's kinda lame that we get no cursor feedback for drops that are disallowed, but that is for a different bug.
I tried dropping a folder into itself, into one if its children and dropping the child into the folder that contained
it. It all worked.
This bug is VERIFIED Fixed on all platforms with the 2001050304 builds
Status: RESOLVED → VERIFIED
Comment 75•24 years ago
|
||
*** Bug 79406 has been marked as a duplicate of this bug. ***
Comment 76•24 years ago
|
||
This is not exactly fixed yet. As pointed out in the dupe bug 79406, you can
still drag and drop onto itself if you select two folders.
reopening.
2001050714 trunk for MacOS9
Severity: major → critical
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment 77•24 years ago
|
||
So this remaining problem is caused by reappearance of bug 42080.
Assignee | ||
Comment 78•24 years ago
|
||
Change "continue" to "break" in bookmarkDD.js where the onDrop handler tests for
dropping a folder on itself and also where it tests for dropping a folder into
one of its own subfolders (the latter is the patch that was presented here
(4/21/01 10:14). That will solve half the problem. Namely it will catch the
case where we are dropping folders A and B (where A precedes B) onto A. However
it will not catch the case of dropping A and B onto B. The reason this one and
not the other is obvious.
Still investigating to come up with a simple patch that will fix the whole
problem.
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 79•24 years ago
|
||
OK, here's a patch that solves the problem. Basically what I had to do was to
break up the loop into two loops -- one that tests each folder being dragged to
make sure it is not being dropped on itself or on one of its children. The
second loop does the actual dropping. This way if one of the folders fails the
drop test, we can do a return before doing any of the drops in the second loop.
In order to avoid code duplication between the loops, some of the simple
variables were changed into arrays.
Assignee | ||
Comment 80•24 years ago
|
||
Assignee | ||
Comment 81•24 years ago
|
||
alecf, ben: Will one of you please review this and the other superreview it.
Thanks.
Comment 82•24 years ago
|
||
I'm looking forward to this last patch :)
Comment 83•24 years ago
|
||
sr=alecf
Comment 84•24 years ago
|
||
sure. r=ben@netscape.com
Assignee | ||
Comment 85•24 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Comment 86•24 years ago
|
||
Hmmm, i don't think I can properly check this bug until bug 42080 is fixed. Nonetheless , although I can't
reproduce it yet i did lose one of my folders while trying to verify this bug so I bet there's still something wrong.
Whiteboard: waiting for 42080 to verify
Comment 87•24 years ago
|
||
VERIFIED Fixed with 2001053108 builds.
I paid special attention to the case where "we are dropping folders A and B
(where A precedes B) onto A...[and]...the case of dropping A and B onto B." As
well as dropping a folder onto one of its (grand)children. Whew.
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•