385989 - [SoC] Camino AppleScript support: Windows and Tabs

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[Peter Jaros (:peeja)]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.4) Gecko/20070509 Camino/1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.4) Gecko/20070509 Camino/1.5

Bug for AppleScript support for Windows and Tabs, as part of Summer of Code: AppleScript Support.

http://wiki.caminobrowser.org/Development:Summer_of_Code_2007:AppleScript:Proposal#Details

Details of proposal are available at the above URL.  This bug should be used both for reviewing the implementation and refining the proposal so we (I) know when it's done. :)

Reproducible: Always

Comment 1

[Peter Jaros (:peeja)]

Attachment: camino/src/appleevents/dictionary/ScriptingSupport.mm

Main source file for scripting support.  Pink said to attach my files rather than try to diff a patch since all my work is in new files.

Currently ScriptingSupport.mm consists of categories on MainController, BrowserWindow, and BrowserWrapper ("application" [by proxy], "browser window", and "tab" respectively, in AppleScript) to provide KVC hooks for Cocoa Scripting.

Simon, would you take a look?

Comment 2

[Peter Jaros (:peeja)]

Attachment: camino/src/appleevents/dictionary/Camino.sdef

Here's the sdef that goes with the code.  If you're interested in testing the code, add ScriptingSupport.mm to the target, add Camino.sdef as a resource, and add the following to the Info.plist:

	<key>OSAScriptingDefinition</key>
	<string>Camino.sdef</string>

This will only work on 10.4.  10.3 support will require some extra steps to convert the sdef to the older format; this will come later.

Does this file need a license block?  If so, what do we do about the standard suite, which is supplied by Apple but needs to be included in the file?

Comment 3

[Simon Fraser [no longer active]]

Comment on attachment 270033 [details]
camino/src/appleevents/dictionary/ScriptingSupport.mm


>#import "../../application/MainController.h"
>#import "../../browser/BrowserWindow.h"
>#import "../../browser/BrowserWrapper.h"

I don't think you need relative paths in the includes here. Xcode will find the headers.

>@class AutoCompleteWindow;
>@class BrowserWindowController;
>
>
>// class: application
>
>@implementation MainController (ScriptingSupport)
>
>- (BOOL)application:(NSApplication *)sender delegateHandlesKey:(NSString *)key
>{
>  return [key isEqualTo:@"orderedWindows"] ||
>         [key isEqualTo:@"allBrowserWindows"];
>}

This is a bit oddly named, and you don't actually use 'sender'. So maybe just call it
-applicationDelegateHandlesKey: ?

>/* Filters out AutoCompleteWindows and offscreen (?) NSWindows with uniqueID -1.
>   That seems to leave only the useful windows. */
>- (NSArray *)orderedWindows
>{
>  NSEnumerator* windowEnum = [[NSApp orderedWindows] objectEnumerator];
>  NSMutableArray* windowArray = [NSMutableArray array];
>
>  NSWindow* curWindow;
>  while ((curWindow = [windowEnum nextObject]))
>    if (![curWindow isKindOfClass:[AutoCompleteWindow class]] &&
>        [[curWindow valueForKey:@"uniqueID"] intValue] != -1)
>      [windowArray addObject:curWindow];
>  
>  return windowArray;
>}

This might be better in a category on NSApplication, since it 'refines' -orderedWindows. Maybe -orderedVisibleWindows or something.

>- (NSArray *)tabs

Add a comment to say what the returned array contains.

>{
>  return [self valueForKeyPath:@"windowController.getTabBrowser.tabViewItems.view"];

It's odd using 'getTabBrowser' in a key-value path. Maybe that method should be renamed to "tabBrowser". I assume that the .view causes an array of views to be returned; does that work on all the OSes we care about?

>- (BrowserWrapper *)currentTab
>{
>  return [self valueForKeyPath:@"windowController.getTabBrowser.selectedTabViewItem.view"];
>}

Ditto.

>// class: tab

eh?

>
>@implementation BrowserWrapper (ScriptingSupport)
>
>- (NSScriptObjectSpecifier *)objectSpecifier
>{
>  BrowserWindow *window = (BrowserWindow *)[self nativeWindow];
>  NSArray *tabArray = [window valueForKeyPath:@"windowController.getTabBrowser.tabViewItems.view"];
>  unsigned index = [tabArray indexOfObjectIdenticalTo:self];
>  
>  if (index != NSNotFound)
>    return [[[NSIndexSpecifier alloc] initWithContainerClassDescription:(NSScriptClassDescription *)[NSScriptClassDescription classDescriptionForClass:[window class]]
>                                                     containerSpecifier:[window objectSpecifier]
>                                                                    key:@"tabs"
>                                                                  index:index] autorelease];
>  else
>    return nil;
>}
>
>// BrowserWindow implements a -currentURI but not a -setCurrentURI:.  This method lets "tab's URL" be a read/write property.
>- (void)setCurrentURI:(NSString *)newURI
>{
>  [self loadURI:newURI referrer:nil flags:NSLoadFlagsNone focusContent:YES allowPopups:YES];
>}

Move this into BrowserWindow perhaps?

Comment 4

[Peter Jaros (:peeja)]

(In reply to comment #3)
> I don't think you need relative paths in the includes here. Xcode will find the
> headers.

You're right, thanks.  Changed.

> >- (BOOL)application:(NSApplication *)sender delegateHandlesKey:(NSString *)key
> 
> This is a bit oddly named, and you don't actually use 'sender'. So maybe just
> call it
> -applicationDelegateHandlesKey: ?

This is an NSApplication delegate method which checks which KVC keys should be passed off to the delegate (possibly exclusively for scripting purposes; the docs are unclear).  I'll add a comment to that effect.

> >/* Filters out AutoCompleteWindows and offscreen (?) NSWindows with uniqueID -1.
> >   That seems to leave only the useful windows. */
> >- (NSArray *)orderedWindows
> 
> This might be better in a category on NSApplication, since it 'refines'
> -orderedWindows. Maybe -orderedVisibleWindows or something.

This method returns what -orderedWindows is supposed to return: "Only windows that are typically scriptable are included in the returned array. For example, panels are not included."  If I could override NSApplication's implementation (and invoke the old implementation) with a category, I would; delegation seems to be the proper substitute.

> >- (NSArray *)tabs
> 
> Add a comment to say what the returned array contains.

Done.

> >{
> >  return [self valueForKeyPath:@"windowController.getTabBrowser.tabViewItems.view"];
> 
> It's odd using 'getTabBrowser' in a key-value path. Maybe that method should be
> renamed to "tabBrowser".

I agree wholeheartedly.  That probably worth breaking out into another bug, though.

> I assume that the .view causes an array of views to be
> returned; does that work on all the OSes we care about?

The docs are *very* unclear on the point, but if memory serves this was part of the KVC overhaul in 10.3's bindings.  Testing will confirm, but it should work fine.

> >- (BrowserWrapper *)currentTab
> Ditto.

Ditto.  :)

> >// class: tab
> 
> eh?

The AS class "tab" is implemented by BrowserWrapper.  I'll note the AS classes at the top of the file, since they're not all obvious.

> >// BrowserWindow implements a -currentURI but not a -setCurrentURI:.  This method lets "tab's URL" be a read/write property.
> >- (void)setCurrentURI:(NSString *)newURI
> >{
> >  [self loadURI:newURI referrer:nil flags:NSLoadFlagsNone focusContent:YES allowPopups:YES];
> >}
> 
> Move this into BrowserWindow perhaps?

Only if we would use it outside of scripting.  In Obj-C, I would expect -loadURI:[...] is good enough; -setCurrentURI: is pretty simplistic.

Comment 5

[Peter Jaros (:peeja)]

Attachment: ScriptingSupport.mm (Take 2)

Changes made as per above.

Simon, thanks for such a speedy review.  Round 2?

Comment 6

[Stuart Morgan]

Comment on attachment 270175 [details]
ScriptingSupport.mm (Take 2)

> * The Original Code is the Mozilla browser.

s/Mozilla browser/Camino code/

> * The Initial Developer of the Original Code is
> * Netscape Communications Corporation.

Not so much... We've been using the original author's name here for files created by individuals.

> * Portions created by the Initial Developer are Copyright (C) 2002

s/2002/2007

>/* 
>  This file adds scripting support to various classes.
> [...]
>   
>*/

Please use C-style comments here and in all your method-level comments for consistency with the rest of the code base.

// class: application

Replace this with
#pragma mark -
#pragma mark Application
(and similarly with the others) for the Xcode benefits.

> /* Delegate method: Declares NSApp should let MainController handle certain KVC keys. */

Can you clarify that this applies only to scripting calls? It wasn't obvious to me that this wouldn't mess up our own use of things like orderedWindows until I went to the Cocoa docs.

>/* Filters out AutoCompleteWindows and (offscreen?) NSWindows with uniqueID -1.
>   That seems to leave only the useful windows. */

This comment should describe what the method is intended to return, not what it's *not* returning.

>        [[curWindow valueForKey:@"uniqueID"] intValue] != -1)

The comment about uniqueID should be moved into the method, but can you make it more clear what it's doing? I'm not sure what it means for uniqueID to be -1, or why it's a key-based lookup rather than an actual call.

>/* Also, uses -isKindOfClass: rather than -isMemberOfClass:, because it's cleaner. */

No need to comment an implementation detail like this (especially one that's really about the implementation of a completely different method).

>  while ((curWindow = [windowEnum nextObject]))
>    if ([[curWindow windowController] isKindOfClass:[BrowserWindowController class]])
>      [windowArray addObject:curWindow];

Multi-line conditionals should have braces, and any |while| whose body isn't a single line generally should as well.

>  if (index != NSNotFound)
>    return [[[NSIndexSpecifier alloc] initWithContainerClassDescription:containerClassDesc
>                                                     containerSpecifier:[NSApp objectSpecifier]
>                                                                    key:@"allBrowserWindows"
>                                                                  index:index] autorelease];

Again, braces since it's multi-line (and then braces for the |else| to match). Same below in the other objectSpecifier method.

>  [self loadURI:newURI referrer:nil flags:NSLoadFlagsNone focusContent:YES allowPopups:YES];

Unless there's a compelling argument otherwise, this should be allowPopups:NO. Allowing popups is pretty much specific to bookmarks to support certain bookmarklets.


That's pretty much all just style fixes though, so r=me with those changes. Looks good!

Comment 7

[Peter Jaros (:peeja)]

Attachment: ScriptingSupport.mm (Take 2, + style changes from smorgan)

(In reply to comment #6)
> [Changes to license block]

Fixed.

> Please use C-style comments here and in all your method-level comments for
> consistency with the rest of the code base.

Changed.

> // class: application
> 
> Replace this with
> #pragma mark -
> #pragma mark Application
> (and similarly with the others) for the Xcode benefits.

Replaced with:
#pragma mark -
#pragma mark Scripting class: application

> > /* Delegate method: Declares NSApp should let MainController handle certain KVC keys. */
> 
> Can you clarify that this applies only to scripting calls? It wasn't obvious to
> me that this wouldn't mess up our own use of things like orderedWindows until I
> went to the Cocoa docs.

It's not just for scripting, but it only intercedes on KVC calls, not direct accessor calls.  That is, KVC simply looks for accessors in the delegate instead of NSApp.  Comment now explains this.

> This comment should describe what the method is intended to return, not what
> it's *not* returning.
> [...]
> The comment about uniqueID should be moved into the method, but can you make it
> more clear what it's doing? I'm not sure what it means for uniqueID to be -1,
> or why it's a key-based lookup rather than an actual call.

There is no actual call; it seems to be only implemented as a KVC key.  No clue what the windows with id -1 are, but they certainly don't belong in the returned array.  Noted in the comments.

> >/* Also, uses -isKindOfClass: rather than -isMemberOfClass:, because it's cleaner. */
> 
> No need to comment an implementation detail like this (especially one that's
> really about the implementation of a completely different method).

Edited.

> >  while ((curWindow = [windowEnum nextObject]))
> >    if ([[curWindow windowController] isKindOfClass:[BrowserWindowController class]])
> >      [windowArray addObject:curWindow];
> 
> Multi-line conditionals should have braces, and any |while| whose body isn't a
> single line generally should as well.

Ok, |while| is now braced, single-line |if| left un-braced.

> > [...]
> 
> Again, braces since it's multi-line (and then braces for the |else| to match).
> Same below in the other objectSpecifier method.

Done.

> >  [self loadURI:newURI referrer:nil flags:NSLoadFlagsNone focusContent:YES allowPopups:YES];
> 
> Unless there's a compelling argument otherwise, this should be allowPopups:NO.
> Allowing popups is pretty much specific to bookmarks to support certain
> bookmarklets.

Ok.  No one in channel could remember why we even had that.  :)

Changed to NO.

> 
> 
> That's pretty much all just style fixes though, so r=me with those changes.
> Looks good!
>

Comment 8

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 271054 [details]
ScriptingSupport.mm (Take 2, + style changes from smorgan)

sr=pink

Comment 9

[Stuart Morgan]

Removing [needs checkin]; this needs a project patch, resource changes, some
decisions about file location, and discussion of how the dictionary is going to
be handled.

(In reply to comment #1)
> camino/src/appleevents/dictionary/ScriptingSupport.mm

We don't have any subfolders anywhere else in our source hierarchy, so creating
a dictionary/ folder here doesn't seem like something we want to do.

(In reply to comment #2)
> camino/src/appleevents/dictionary/Camino.sdef

Shouldn't this file be in resources/application/, not src/appleevents/?

> add the following to the Info.plist:
> 
>         <key>OSAScriptingDefinition</key>
>         <string>Camino.sdef</string>
> 
> This will only work on 10.4.  10.3 support will require some extra steps to
> convert the sdef to the older format; this will come later.

Is it possible to have the old 10.3 stuff and the new stuff coexist in the
short term, or does "later" need to happen before this can be checked in to
avoid breaking AS for 10.3?

Comment 10

[Peter Jaros (:peeja)]

Attachment: Windows and Tabs Patch

Patch to add ScriptingSupport.mm, as reviewed above, except changed two comments from /* */ to // style.

Regarding Stuart's comments: ScriptingSupport.mm is now in src/appleevents; Camino.sdef is in resources/application, though this is now in bug 390072; branch/trunk parallel support is now covered by bug 390072.

Barring any other issues, this patch can land when bug 390072 lands.

Comment 11

[Peter Jaros (:peeja)]

Attachment: Windows and Tabs Patch + Project Change

Oops.  Same thing, now with the new file added to the project.

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 274468 [details] [diff] [review]
Windows and Tabs Patch + Project Change

sr=pink including project changes.

Comment 13

[Stuart Morgan]

Fixed on trunk by the patch landed in bug 390072. Leaving open for branch.

Comment 14

[Stuart Morgan]

Fixed on branch by the patch landed in bug 390072. (I made a nativeWindow -> getNativeWindow adjustment on checkin)