394821 - Moving a bookmark to a folder via AppleScript deletes the folder (and contents)

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The script

tell application "Camino"
    move bookmark "Test" of bookmark bar collection to ¬
        bookmark folder "Test ƒ" of bookmark bar collection
end tell

deletes "Test ƒ" (and any of its contents) instead of moving "Test" into the folder ("Test" remains in the bar).

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

FWIW, it actually moved the bookmark "Test" to after the last bookmark, if the folder "Test ƒ" was situated among bookmarks.

Comment 2

[Peter Jaros (:peeja)]

Taking.  No idea why this is happening, but I'll figure it out.

Comment 3

[Peter Jaros (:peeja)]

Ah, this script is working correctly.  The script that does what you meant would be this:

tell application "Camino"
    move bookmark "Test" of bookmark bar collection to ¬
        end of bookmark items of bookmark folder "Test ƒ" of bookmark bar collection
end tell

Not so great that what you wrote isn't right; worse that it results in data loss.  It's on the agenda for the Oct. 24th meeting.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Not blocking a1, but peeja has an idea how to fix this that he's going to investigate.

Comment 5

[Samuel Sidler (old account; do not CC)]

Peter, any work on this?

Comment 6

[Peter Jaros (:peeja)]

Nothing new yet, but the semester's over now.  I'm getting back to the AppleScript stuff now, though my first task is getting a tree to build again.

Comment 7

[Peter Jaros (:peeja)]

Attachment: MoveCommand(.mm|.h) + Camino.sdef change

Got it.  This patch adds a new MoveCommand handler for the move command.  If it's called on to replace something ("move X to Y"), it will alter its arguments to mean "move X to the end of Y".  If Y is not a collection, the script errors, which makes sense.  Other insertion positions, such as "move X to before Y", work as before.

Project patches for trunk and branch are provided separately.

Comment 8

[Peter Jaros (:peeja)]

Attachment: Trunk Project Patch

Comment 9

[Peter Jaros (:peeja)]

Attachment: Branch Project Patch

Comment 10

[Sean Murphy]

Comment on attachment 300891 [details] [diff] [review]
MoveCommand(.mm|.h) + Camino.sdef change

Behavior works as expected; with the one code change below, r=murph.

>+  NSMutableDictionary *args = [[self arguments] mutableCopy];

Using |mutableCopy| means you'll have to release |args| after calling |setArguments:|.  I'd either tack on an |autorelease| or just use [NSMutableDictionary dictionaryWithDictionary:].

Comment 11

[Sean Murphy]

Comment on attachment 300892 [details] [diff] [review]
Trunk Project Patch

Unfortunately, the project file changed in the last few days, breaking this patch already.  You'll need to re-diff :)

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 300894 [details] [diff] [review]
Branch Project Patch

Cancelling the review request on this, since it's likely bitrotted and we need a new code patch anyway.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 300894 [details] [diff] [review]
Branch Project Patch

Cancelling the review request on this, since it's likely bitrotted and we need a new code patch anyway.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Plussing/targeting 1.6 since this is dataloss and the patch is close.  

Can we get an ETA on a new version?

Comment 15

[Peter Jaros (:peeja)]

I'll be as soon as I can get a tree building, so probably by tonight. If I can't get it going by tonight, I'll try to get someone else to spin the patch for me.

Comment 16

[Peter Jaros (:peeja)]

Attachment: Code respin

+ murph's change.

Comment 17

[Peter Jaros (:peeja)]

Attachment: Trunk Proj. Patch, clean of bitrot

Again, just adds MoveCommand.*.  Probably needs a port to branch, which Smokey offered to do on checkin.

Comment 18

[Peter Jaros (:peeja)]

Comment on attachment 303176 [details] [diff] [review]
Code respin

>Index: src/appleevents/MoveCommand.h
>+ * Portions created by the Initial Developer are Copyright (C) 2007

>Index: src/appleevents/MoveCommand.mm
>+ * Portions created by the Initial Developer are Copyright (C) 2007

Er, make that 2008, would you?  I swear I've gotten it right on all my checks...

Comment 19

[Stuart Morgan]

Comment on attachment 303176 [details] [diff] [review]
Code respin

>+    NSPositionalSpecifier *newToLoc = [[NSPositionalSpecifier alloc] initWithPosition:NSPositionBeginning objectSpecifier:[toLoc objectSpecifier]];

Three things here:
- Why is this NSPositionBeginning, when the discussion was about putting it at the end (which seems more likely to be the expected behavior)?
- The specificier is leaked, and should be autoreleased on this line.
- The line is really long, and should be broken before |objectSpecifier|

Comment 20

[Stuart Morgan]

Comment on attachment 303180 [details] [diff] [review]
Trunk Proj. Patch, clean of bitrot

(No need for separate review of project bits)

Comment 21

[Peter Jaros (:peeja)]

(In reply to comment #19)
> (From update of attachment 303176 [details] [diff] [review])
> >+    NSPositionalSpecifier *newToLoc = [[NSPositionalSpecifier alloc] initWithPosition:NSPositionBeginning objectSpecifier:[toLoc objectSpecifier]];
> 
> Three things here:
> - Why is this NSPositionBeginning, when the discussion was about putting it at
> the end (which seems more likely to be the expected behavior)?

Yes, this was supposed to be NSPositionEnd, but I used NSPositionBeginning first and somehow I diffed from that version.  Sorry I haven't been able to spin a new patch yet.  If you can make the the changes on checkin, Stuart, that would be awesome.

Comment 22

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH with the comment 19 fixes.

Comment 23

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I checked in an additional fix for comment 18.  Farewell bookmark-eating AppleScripts!

One of us needs to update the AppleScript Guide about this.