390846 - [SoC] Camino AppleScript Support: Bookmarks

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[Peter Jaros (:peeja)]

The patch for this bug will add bookmark, bookmark folder, and bookmark item scripting classes to the Camino scripting dictionary.  Bookmarks and folders will be able to be read, edited, and created from AppleScript.

Comment 1

[Peter Jaros (:peeja)]

Stuart: I'm corralling the scripting code into ScriptingSupport.mm.  I'm a little confused by some of the "scripting methods" in BookmarkFolder.mm.  For each of the keys childArray, childBookmarks, and childFolders, there are four methods:

-insertIn<key>:atIndex:
-addIn<key>:
-removeFrom<key>AtIndex:
-replaceIn<key>:atIndex:

Three of those methods are specified by the NSScriptKeyValueCoding protocol, but I've never heard of addIn<key>:.  Do you know when, or if, this is called?  If it's not, we should cut it.

Ref:
Code: <http://mxr.mozilla.org/mozilla/source/camino/src/bookmarks/BookmarkFolder.mm#1369>
NSScriptKeyValueCoding: <http://developer.apple.com/documentation/Cocoa/Reference/Foundation/Protocols/NSScriptKeyValueCoding_Protocol/Reference/Reference.html>

Comment 2

[Peter Jaros (:peeja)]

Attachment: Bookmark Scripting Support

Makes bookmarks scriptable, as described above.  Also corrals scripting code from the bookmark classes into SS.mm.

Comment 3

[Stuart Morgan]

Comment on attachment 275649 [details] [diff] [review]
Bookmark Scripting Support

>+ *   Stuart Morgan <[e-mail]>

I didn't write any of the code you moved, so you can leave me out of the list.

>+// Returns the top-level BookmarkFolders from the Bookmark Manager's Collections pane.
>+- (NSArray *)topLevelBookmarkFolders

This should be called "bookmarkCollections". Can the AppleScript term be |bookmark collection|, or does that mess with the type parity with folders?

>+- (void)insertInTopLevelBookmarkFolders:(BookmarkFolder *)aItem atIndex:(unsigned)aIndex
...
>+- (void)insertInTopLevelBookmarkFolders:(BookmarkFolder *)aItem
...
>+- (void)removeFromTopLevelBookmarkFoldersAtIndex:(unsigned)aIndex
...
>+- (void)replaceInTopLevelBookmarkFolders:(BookmarkFolder *)aItem atIndex:(unsigned)aIndex

Am I correctly understanding these to be methods to add and remove arbitrary collections? If so, that's not okay; very bad things happen if you start mucking with any of the pre-existing collections (that's a problem we should address eventually, but it's the reality now). Either they need to error out on an attempt to change anything before the last smart folder, or bookmarkCollections needs to just be read only.

> +      [[NSScriptCommand currentCommand] setScriptErrorString:[NSString stringWithFormat:@"A bookmark item's %@ can't be that type.", scriptingClassName]];

Are AS errors generally not localized, due to the English-centric nature of AS?

>+  NSArray *kiddies = [self childArray];

This would be an opportune moment to change |kiddies| to |children|.

>+// These two methods currently treat the incoming index as an index into the filtered array of
>+// folders or bookmarks, and find a place to put the new item in the full childArray so they come
>+// out at the right index of the applicable filtered array.  This may or may not be desired by
>+// scripters, depending on the reference form used.

Could the filtered forms be read-only to avoid this confusion?

Comment 4

[Peter Jaros (:peeja)]

Attachment: Take 2

Addresses Stuart's comments and cleans up the sdef a bit.

(In reply to comment #3)
> (From update of attachment 275649 [details] [diff] [review])
> >+ *   Stuart Morgan <[e-mail]>
> 
> I didn't write any of the code you moved, so you can leave me out of the list.

Check.

> >+// Returns the top-level BookmarkFolders from the Bookmark Manager's Collections pane.
> >+- (NSArray *)topLevelBookmarkFolders
> 
> This should be called "bookmarkCollections". Can the AppleScript term be
> |bookmark collection|, or does that mess with the type parity with folders?

Key changed.  The AS term has to be the class name, unfortunately; AS has no concept of named one-to-many relationships.

> >+- (void)insertInTopLevelBookmarkFolders:(BookmarkFolder *)aItem atIndex:(unsigned)aIndex
> ...
> 
> Am I correctly understanding these to be methods to add and remove arbitrary
> collections? If so, that's not okay;

The special collections have been removed from the (renamed) bookmrkCollections key and now each have their own property of application.

> > +      [[NSScriptCommand currentCommand] setScriptErrorString:[NSString stringWithFormat:@"A bookmark item's %@ can't be that type.", scriptingClassName]];
> 
> Are AS errors generally not localized, due to the English-centric nature of AS?

Yes, I believe.

> >+  NSArray *kiddies = [self childArray];
> 
> This would be an opportune moment to change |kiddies| to |children|.

Done.

> >+// These two methods currently treat the incoming index as an index into the filtered array of
> >+// folders or bookmarks, and find a place to put the new item in the full childArray so they come
> >+// out at the right index of the applicable filtered array.  This may or may not be desired by
> >+// scripters, depending on the reference form used.
> 
> Could the filtered forms be read-only to avoid this confusion?

I don't think so.  Since AS deals in pure containment rather than named one-to-many relationships, any object which calls itself a bookmark folder is by default in the "bookmark folders" collection, so telling a containing folder to remove it invokes -removeFromChildFoldersAtIndex: (not removeFromChildArrayAtIndex:).

Comment 5

[Stuart Morgan]

Comment on attachment 276696 [details] [diff] [review]
Take 2

>+    // Special folders have non-nil identifiers.
>+    if (![eachFolder identifier])

You should be able to use isSpecial here. (It looks like isSpecial is mysteriously missing the menu collection, but you can just fix that.)

Also, unless I'm just not seeing it, you don't undo this transformation, meaning if I stick something at index 0 of the array of user-created collections, it will actually end up before the bookmark menu, not right after the last special folder.

> property name="bookmark menu"
> property name="bookmark bar"
> property name="top ten list"
> property name="bonjour collection"
> property name="address book collection

Since these don't have context, I think the naming needs more consistency to help group them. I'd suggest:
  bookmark menu collection
  bookmark bar collection
  top ten collection
  bonjour collection
  address book collection
(and possibly even s/collection/bookmark collection/ just to be very clear, but that gets pretty long and probably isn't necessary)

r=me with those changes

Comment 6

[Peter Jaros (:peeja)]

Attachment: Patch w/ Stuart's Changes

Changes made.  As discussed on IRC, BookmarkFolder now implements isSpecial based on whether it has an identifier.

Comment 7

[Mike Pinkerton (not reading bugmail)]

+  NSMutableArray *collections = [[NSMutableArray alloc] init];
+  NSEnumerator *e = [array objectEnumerator];
+  id eachFolder;
+  while (eachFolder = [e nextObject]) {
+    if (![eachFolder isSpecial])
+      [collections addObject:eachFolder];
+  }
+  return collections;

|collections| should be autoreleased if you're returning it. Probably best to just use |[NSMutableArray array]| to allocate it. You also need to check the callers to ensure they're not releasing it then. 

+  while (eachFolder = [e nextObject]) {

won't this have a warning w/out the double-parens? Make sure your code doesn't add any warnings to the project on a compile. 

+  for (i = 0; i < c; i++) {

|i| should be local to the loop, and just call it |count|. Better to use longer variable names for clarity. probably also double-parens.

+  if ([self isSmartFolder]) {
+    [[NSScriptCommand currentCommand] setScriptErrorNumber:NSReceiversCantHandleCommandScriptError];
+    [[NSScriptCommand currentCommand] setScriptErrorString:[NSString stringWithFormat:@"Can't modify contents of smart folder '%@'.", [self title]]];
+    return false;
+  }
+  else
+    return true;

don't put an |else| after a |return|.

Comment 8

[Stuart Morgan]

(In reply to comment #7)
> +    return false;
> +  }
> +  else
> +    return true;

Oops, and |true| and |false| are not BOOL values (you want YES and NO). Sorry I missed that.

Comment 9

[Peter Jaros (:peeja)]

(In reply to comment #7)
> |collections| should be autoreleased if you're returning it. Probably best to
> just use |[NSMutableArray array]| to allocate it. You also need to check the
> callers to ensure they're not releasing it then. 

Thanks.

> +  while (eachFolder = [e nextObject]) {
> 
> won't this have a warning w/out the double-parens? Make sure your code doesn't
> add any warnings to the project on a compile. 

Nope, no warning.  In fact, that's how Apple write their enumerator loops in sample code.  But you're right, it's not Camino style, so I'll change it.

> +  for (i = 0; i < c; i++) {
> 
> |i| should be local to the loop, and just call it |count|. Better to use longer
> variable names for clarity. probably also double-parens.

I take no responsibility for this code (moved from BookmarkFolder.m), but perhaps I should.  I'll rewrite it as an enumerator loop.

> +  if ([self isSmartFolder]) {
[...]
> +    return false;
> +  }
> +  else
> +    return true;
> 
> don't put an |else| after a |return|.

No?  Even if it increases the readability of the method?  As it stands it's a clear, semantic branch: if we're a smart folder, return true, but if not return false.  They execute equivalently, but the else expresses the semantics of the method.

(In reply to comment #8)
> Oops, and |true| and |false| are not BOOL values (you want YES and NO). Sorry I
> missed that.

Thanks, fixing.

Comment 10

[Mike Pinkerton (not reading bugmail)]

(In reply to comment #9)
> No?  Even if it increases the readability of the method?  As it stands it's a
> clear, semantic branch: if we're a smart folder, return true, but if not return
> false.  They execute equivalently, but the else expresses the semantics of the
> method.

We generally do this across the board. It's a mozilla style rule as well IIRC.

Comment 11

[Peter Jaros (:peeja)]

Attachment: Patch w/ Pink's Changes

(In reply to comment #10)
> We generally do this across the board. It's a mozilla style rule as well IIRC.

Ok, done.

Comment 12

[Peter Jaros (:peeja)]

Attachment: Correct Patch w/ Pink's Changes

Er, that wasn't the right patch at all.  Here's the real patch.

Comment 13

[Peter Jaros (:peeja)]

Attachment: Correct Patch w/ Pink's Changes (really)

I don't believe this.  In the ~60 seconds between successfully testing and posting the patch, I seem to have managed to type the letters "nt" and press save, thereby inserting a very small, but no less compiler-confusing, amount of gibberish into the file.  Brilliant.  Here's an actual, functioning patch of the correct functionality.

Jeez.

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 278955 [details] [diff] [review]
Correct Patch w/ Pink's Changes (really)

sr=pink

Comment 15

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH.