Closed Bug 393050 Opened 17 years ago Closed 17 years ago

[SoC] Camino AppleScript Support: Toolbar Items

Categories

(Camino Graveyard :: OS Integration, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: peeja, Assigned: peeja)

References

Details

(Keywords: fixed1.8.1.10, Whiteboard: [new strings in comment 6])

Attachments

(4 files, 5 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
This bug is phase three of the AppleScript Summer of Code project: Toolbar Items.  This patch adds a new class, ToolbarScriptItem, and modifies BWC to add TSIs to the toolbar palette.  The TSIs correspond to AppleScripts found in ~/Scripts/Applications/Camino, and when clicked they run their associated script.  Any errors are logged to the console.  The icon for the toolbar item is the icon of the script file.

The files ToolbarScriptItem.* need to be added to the project with the appropriate patch (coming in a moment.)
Attachment #277558 - Flags: review?(murph)
Sorry, that should be ~/Libarary/Scripts/Applications/Camino.
Project patch for trunk.
Attachment #277571 - Flags: review?(murph)
Project patch for branch.  The code is currently untested on branch, but ought to work.  Smokey says he'll test tonight-ish; anyone else who'd like to is encouraged as well. :)
Attachment #277574 - Flags: review?(murph)
Attached file Test Scripts
Two scripts for testing TSI:

Hello, World.scpt: Displays a simple "Hello, World."  Has a custom icon.
Houston....scpt: Causes a deliberate error.

To test:
1. Place the scripts in ~/Library/Scripts/Applications/Camino
2. Run Camino with TSI patch.
3. Customize toolbar.
4. Make sure both scripts are available and their icons are correct.
5. Add them to the toolbar.
6. Click each and verify their behaviors.  The error script should cause a beep and log an error to the console.

Note that these scripts do not require any of the new AppleScript support.
Attachment #277574 - Attachment description: Trunk Project Patch → Branch Project Patch
The patch appears to work fine, but there are some issues with the overall experience:

1) First and foremost, there's only room for five new items in the sheet here, so if you have more than 5 scripts, the last n alphabetically get left off and can't be added.  (I also noticed that the sheet got narrower; in an unpatched build, I have 4 columns, but patched I am reduced to 3).  I'm not sure there's anything we can do here, but it does hurt the overall experience.

2) Items in the toolbar sheet are named "ScriptItem: Name of Script" (in the toolbar, they're simply "Name of Script", which helps with spacing issues).  I know that we do use more descriptive names in the sheet, but given 1) above, perhaps just "Script: Name of Script"?  At the very least, "Script: " removes the camelCaps code from the user's world.

3) If you have a bundle script, it's listed as "ScriptItem: main" instead of "ScriptItem: Name of Bundle" (and doesn't pick up bundle's icon).  Again, not sure it's anything we can change, but it's ugly.  I'm not positive that bundles will work; the one I tried was CamiLink, which started to work and then errored, but it's old and I haven't torn it apart to see what removed commands it might be trying to use....

4) Tooltips all come up "ScriptItemToolTipFormat"; missing a localizable.strings entry?  (Is there any way to let the script define its tooltip? That would be cool....)

Also, your BWC was about a week out-of-date and I had to do some manual merging (luckily simple merging, but not being a code, I really hate doing it) :(  Please always update your tree before attaching a new patch for review.  http://wiki.caminobrowser.org/Development:Reviewing#Requesting_Review (pgh 4) ;)
Ugh, was it?  I swear I *just* up'ed it.  I'll respin.

#2 & #4: Mea culpa.  Those are localized strings, which I meant to put in the original bug comments.  Here are the new strings:

---
"ScriptItem" = "Script";
"ScriptItemToolTipFormat" = "Run script \"%@\"";
---

#1: I have no idea why that's happening.  I just assumed the sheet made room for more items.  I'll do some poking, but there may not be much we can do.  I have no idea why you have fewer columns.  I'm still getting 4 on trunk.

#3: I'd forgotten about script bundles.  I can probably make it handle script bundles' icons and names pretty easily, but I'm not sure we can allow bundled scripts to access their resources.  It looks like the only way a bundled script can get to its files is via "path to me" which returns the path to the bundle.  However, in the context of another app (even Script Editor) "path to me" returns the path to the app instead.  Therefore script bundles are pretty much only useful when used from the Script Menu, where "path to me" is useful.

If that's wrong or changes in the future, we might be able to make it work.  In the meantime, I'm going to have the code not pick up script bundles.  Unfortunately, the user may not realize that it's a bundle, but I think it's the most graceful way to go.  Other opinions are welcome.
Whiteboard: [new strings in comment 6]
Comment on attachment 277558 [details] [diff] [review]
Patch

Nice code Peter.  r=me with the following comments:

>+NSString* const ScriptItemIdentifierPrefix = @"ScriptItem:";

Prefix this constant with a 'k'.

>++ (NSArray *)scriptItemIdentifiers
>+{
>+  // Get script paths.
>+  NSString *libraryPath = [NSSearchPathForDirectoriesInDomains(NSLibraryDirectory, NSUserDomainMask, YES) objectAtIndex:0];
>+  NSString *appName = [[[NSBundle mainBundle] infoDictionary] valueForKey:@"CFBundleName"];
>+  NSString *scriptDirPath = [[libraryPath stringByAppendingPathComponent:@"Scripts/Applications"] stringByAppendingPathComponent:appName];

Concerning the script location in "Scripts/Applications/", are these folder names expected to be localized (I don't think they are, but just wanted to make sure).

>+  NSMutableArray *identifiers = [[NSMutableArray alloc] init];
...
>+  return identifiers;

|identifiers| is leaking; You'll need to return an autoreleased object.

>+  while (eachRelativePath = [dirEnum nextObject])

Throw an extra set of parens around the condition.

>+  // Try to lead the script.

I wasn't sure if you meant 'load' here instead of 'lead'?

Also, keep an eye out for pointer style.  Elsewhere we seem to be following the (NSString*) convention as opposed to (NSString *).

(In reply to comment #5)
 
> 1) First and foremost, there's only room for five new items in the sheet here,
> so if you have more than 5 scripts, the last n alphabetically get left off and
> can't be added.  (I also noticed that the sheet got narrower; in an unpatched
> build, I have 4 columns, but patched I am reduced to 3).  I'm not sure there's
> anything we can do here, but it does hurt the overall experience.

On Tiger, the customize sheet will scroll to support more than can visibly fit in and none get left off.  Unfortunately, the column issue is the same though.  Neither of these are Peter's fault, but hopefully we can think of a workaround.
Attachment #277558 - Flags: review?(murph) → review+
The Localized.strings fixed the number of columns issue :)  "ScriptItem:" plus a script name was just too wide ;)  Users with long script names might still have the problem, but...

As it turns out, the problem with "sheet overflow" is not a problem at all, but just me experiencing a very bad coincidence of other issues: the script names being too long without the loc.string, the fact I had exactly the number of items it would show minus one, the fact that one of the scripts doesn't have a .scpt extension, and the fact that that script was alphabetically last.  Instead of doing more testing (adding another script), I drew conclusions from the available facts :(  Copying a bunch of "valid" scripts gets me a scrollbar in the sheet :)

Is it reasonable to handle scripts without the .scpt extension?  Or should we assume that everyone's "gotten with the program" as it were? (This was a script from before the era where we all gave up on having to live with extensions.)  I can add a FAQ just as easily, if that's the way we want to go.

Regarding bundles: what about bundles which are bundles just for the sake of being bundles (e.g., application bundles to be Universal) rather than using for using "path to me"?  I'm fine with skipping them altogether, but I just wanted to toss that thought out there.  Again, this is easy to FAQ if need be.
Comment on attachment 277571 [details] [diff] [review]
Trunk Project Patch

On disk, the files are created under the appleevents directory but in the project they're listed under the "Categories and Extensions" group.

Otherwise, patch works fine; r=me.
Attachment #277571 - Flags: review?(murph) → review+
(In reply to comment #9)
> (From update of attachment 277571 [details] [diff] [review])
> On disk, the files are created under the appleevents directory but in the
> project they're listed under the "Categories and Extensions" group.

We discussed this with Stuart; he said the more critical factor (for the project) is that the files relate to toolbar stuff rather than apple events, and there not being a toolbar folder there, to put them in "Categories and Extensions" until he busts that up into something more orderly ;)
(In reply to comment #8)
> Is it reasonable to handle scripts without the .scpt extension?  Or should we
> assume that everyone's "gotten with the program" as it were? (This was a script
> from before the era where we all gave up on having to live with extensions.)  I
> can add a FAQ just as easily, if that's the way we want to go.

Let's stick to using the extension.  Anything else in OS X seems silly.  Actually, the "best" way to do this is with UTIs, but that's more code for pretty much no payoff.

Re: bundles:  Good news!  If we load the whole bundle "path to me" works perfectly.  This version accepts .scpt, .scptd, .app, and even .applescript source files.  The only issue is that it'll pick up any application in the script folder (and fail to load it as a script).  Shouldn't be a problem, since there's no reason to put a normal app in there.

Also incorporated Murph's changes.
Attachment #277558 - Attachment is obsolete: true
Attachment #277910 - Flags: review?(nick.kreeger)
Comment on attachment 277910 [details] [diff] [review]
Patch w/ Murph's Changes & Bundle Support


>+  // Items defined statically (unlike script items, which are dynamically defined).
>+  NSArray *staticItems = [NSArray arrayWithObjects: BackToolbarItemIdentifier,
>+                                                    ForwardToolbarItemIdentifier,
>+                                                    ReloadToolbarItemIdentifier,
>+                                                    StopToolbarItemIdentifier,
>+                                                    HomeToolbarItemIdentifier,
>+                                                    CombinedLocationToolbarItemIdentifier,
>+                                                    BookmarksToolbarItemIdentifier,
>+                                                    ThrobberToolbarItemIdentifier,
>+                                                    PrintToolbarItemIdentifier,
>+                                                    ViewSourceToolbarItemIdentifier,
>+                                                    BookmarkToolbarItemIdentifier,
>+                                                    NewTabToolbarItemIdentifier,
>+                                                    CloseTabToolbarItemIdentifier,
>+                                                    TextBiggerToolbarItemIdentifier,
>+                                                    TextSmallerToolbarItemIdentifier,
>+                                                    SendURLToolbarItemIdentifier,
>+                                                    NSToolbarCustomizeToolbarItemIdentifier,
>+                                                    NSToolbarFlexibleSpaceItemIdentifier,
>+                                                    NSToolbarSpaceItemIdentifier,
>+                                                    NSToolbarSeparatorItemIdentifier,
>+                                                    DLManagerToolbarItemIdentifier,
>+                                                    FormFillToolbarItemIdentifier,
>+                                                    HistoryToolbarItemIdentifier,
>+                                                    nil];
>+  // Add script items and return.
>+  return [staticItems arrayByAddingObjectsFromArray:[ToolbarScriptItem scriptItemIdentifiers]];
> }

These really aren't _static_, so lets pick a better name like |toolbarIdentifiers|.

>+  else if ([itemIdent hasPrefix:kScriptItemIdentifierPrefix]) {
>+    // Make a new ToolbarScriptItem instead.
>+    toolbarItem = [[[ToolbarScriptItem alloc] initWithItemIdentifier:itemIdent] autorelease];
>+  }

Comment isn't really beneficial here.

>Index: src/appleevents/ToolbarScriptItem.h
>===================================================================
>RCS file: src/appleevents/ToolbarScriptItem.h
>diff -N src/appleevents/ToolbarScriptItem.h
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ src/appleevents/ToolbarScriptItem.h	23 Aug 2007 15:24:00 -0000
>@@ -0,0 +1,51 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+ *
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Camino code.
>+ *
>+ * The Initial Developer of the Original Code is The Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2007
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Peter Jaros <peter.a.jaros@gmail.com> (Original Author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#import <Cocoa/Cocoa.h>
>+
>+// Script item identifiers take the form: <ScriptItemIdentifierPrefix><pathToScript>
>+extern NSString* const kScriptItemIdentifierPrefix;

Let's be consistant with style, put a space after your '*'.

>+
>+@interface ToolbarScriptItem : NSToolbarItem {}
>+
>++ (NSArray *)scriptItemIdentifiers;
>+
>+- (NSString *)scriptPath;
>+- (NSString *)scriptName;
>+
>+@end
>Index: src/appleevents/ToolbarScriptItem.mm
>===================================================================
>RCS file: src/appleevents/ToolbarScriptItem.mm
>diff -N src/appleevents/ToolbarScriptItem.mm
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ src/appleevents/ToolbarScriptItem.mm	23 Aug 2007 15:24:06 -0000
>@@ -0,0 +1,125 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+ *
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Camino code.
>+ *
>+ * The Initial Developer of the Original Code is The Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2007
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Peter Jaros <peter.a.jaros@gmail.com> (Original Author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#import "ToolbarScriptItem.h"
>+
>+NSString* const kScriptItemIdentifierPrefix = @"ScriptItem:";
>+
>+@implementation ToolbarScriptItem
>+
>+// Returns an array of toolbar item identifiers for the script items available.
>++ (NSArray *)scriptItemIdentifiers
>+{
>+  // Get script paths.
>+  NSString *libraryPath = [NSSearchPathForDirectoriesInDomains(NSLibraryDirectory, NSUserDomainMask, YES) objectAtIndex:0];
>+  NSString *appName = [[[NSBundle mainBundle] infoDictionary] valueForKey:@"CFBundleName"];
>+  NSString *scriptDirPath = [[libraryPath stringByAppendingPathComponent:@"Scripts/Applications"] stringByAppendingPathComponent:appName];
>+
>+  NSMutableArray *identifiers = [[[NSMutableArray alloc] init] autorelease];
>+  NSDirectoryEnumerator *dirEnum = [[NSFileManager defaultManager] enumeratorAtPath:scriptDirPath];
>+  id eachRelativePath;
>+  // -nextObject yields relative paths.  We need absolute paths.
>+  while ((eachRelativePath = [dirEnum nextObject]))
>+  {

For type safety, do this:
  while ((eachRelativePath = [dirEnum nextObject]) != null) { ...

Let's keep this curly on the same line (style).

>+    NSString *eachAbsolutePath = [scriptDirPath stringByAppendingPathComponent:eachRelativePath];
>+    if ([[eachAbsolutePath pathExtension] isEqualToString: @"scpt"] ||
>+        [[eachAbsolutePath pathExtension] isEqualToString: @"applescript"] ||
>+        [[eachAbsolutePath pathExtension] isEqualToString: @"scptd"] ||
>+        [[eachAbsolutePath pathExtension] isEqualToString: @"app"]) {
>+      [identifiers addObject:[NSString stringWithFormat:@"%@%@", kScriptItemIdentifierPrefix, eachAbsolutePath]];
>+    }
>+    // Don't look inside script bundles or application bundles.
>+    if ([[eachAbsolutePath pathExtension] isEqualToString: @"scptd"] ||
>+        [[eachAbsolutePath pathExtension] isEqualToString: @"app"]) {
>+      [dirEnum skipDescendents];
>+    }
>+  }

While I'm sure that |pathExtension:| is fairly effecient, I would create a local string that you could repeatively check:
NSString *curPathExtension = [eachAbsolutePath pathExtension];
if ([curPathExtension isEqualToString:@""]) ...

Style nit: No space after the ':' 
>+    if ([[eachAbsolutePath pathExtension] isEqualToString: @"scpt"] ||
(Should be |isEqualToString:@""] ...)

Also, on an if statement that spans over more than one line, put the curly on the next line, instead of hiding it at the end of the if structure.
  

>+
>+  return identifiers;
>+}
>+
>+
>+- (id)initWithItemIdentifier:(NSString *)ident
>+{
>+  // Sanity check: Make sure this is a valid ToolbarScriptItem identifier.
>+  if (![ident hasPrefix:kScriptItemIdentifierPrefix]) return nil;
>+
>+  if (![super initWithItemIdentifier:ident]) return nil;

Style nit: Put your |return nil| calls on a new line.
Actually, I would like to see this method w/o early returns.

if ([ident hasPrefix:kScriptItemIdentifierPrefix] && [super initWithItemIdentifier:ident]) {
  ...
}

>+- (void)runScript:(id)sender
>+{
>+  NSDictionary *errDict = nil;
>+  
>+  // Load the script.
>+  NSAppleScript *script = [[[NSAppleScript alloc] initWithContentsOfURL:[NSURL fileURLWithPath:[self scriptPath]] error:&errDict] autorelease];
>+  if (!script) {
>+    NSBeep();
>+    NSLog(@"Error loading script at %@: %@", [self scriptPath], [errDict valueForKey:NSAppleScriptErrorMessage]);
>+    return;
>+  }
>+  
>+  // Run the script.
>+  NSAppleEventDescriptor *result = [script executeAndReturnError:&errDict];
>+  if (!result) {
>+    NSBeep();
>+    NSLog(@"Error running script at %@: %@", [self scriptPath], [errDict valueForKey:NSAppleScriptErrorMessage]);
>+    return;
>+  }
>+}

Obviously, these logs are probably for AppleScript devs, but we should consider #if'defing these for DEBUG. It is just an option I'm throwing out there.

r=me with those changes.
Attachment #277910 - Flags: review?(nick.kreeger) → review+
(In reply to comment #12)
> (From update of attachment 277910 [details] [diff] [review])
> 
> >+  // Items defined statically (unlike script items, which are dynamically defined).
> 
> These really aren't _static_, so lets pick a better name like
> |toolbarIdentifiers|.

Ok, but the point is that they're not *all* of the toolbar identifiers.  Could we decide on a different qualifier than "static"?

> >+  else if ([itemIdent hasPrefix:kScriptItemIdentifierPrefix]) {
> >+    // Make a new ToolbarScriptItem instead.
> >+    toolbarItem = [[[ToolbarScriptItem alloc] initWithItemIdentifier:itemIdent] autorelease];
> >+  }
> 
> Comment isn't really beneficial here.

That comment is there to match similar comments above, eg:

> // create a new toolbar item that knows how to do validation
> toolbarItem = [[[ToolbarViewItem alloc] initWithItemIdentifier:itemIdent] autorelease];

I assumed the style here was to comment if we need to replace toolbarItem.  Is this a different case?

> >+extern NSString* const kScriptItemIdentifierPrefix;
> 
> Let's be consistant with style, put a space after your '*'.

I'm assuming you mean before...that's a typo.  Thanks.

> >+  // -nextObject yields relative paths.  We need absolute paths.
> >+  while ((eachRelativePath = [dirEnum nextObject]))
> >+  {
> 
> For type safety, do this:
>   while ((eachRelativePath = [dirEnum nextObject]) != null) { ...

That's not the style used in the rest of the project; search on "nextObject".

> Let's keep this curly on the same line (style).

That, however, is.  Oops. :)

> While I'm sure that |pathExtension:| is fairly effecient, I would create a
> local string that you could repeatively check:
> NSString *curPathExtension = [eachAbsolutePath pathExtension];
> if ([curPathExtension isEqualToString:@""]) ...

Ok.

> Style nit: No space after the ':' 
> >+    if ([[eachAbsolutePath pathExtension] isEqualToString: @"scpt"] ||
> (Should be |isEqualToString:@""] ...)

Dunno why that happened.  Fixed.

> Also, on an if statement that spans over more than one line, put the curly on
> the next line, instead of hiding it at the end of the if structure.

Sounds good, done.

> Style nit: Put your |return nil| calls on a new line.
> Actually, I would like to see this method w/o early returns.
> 
> if ([ident hasPrefix:kScriptItemIdentifierPrefix] && [super
> initWithItemIdentifier:ident]) {
>   ...
> }

You'd rather see the whole thing inside a conditional than just bail on the initial checks?

> 
> >+- (void)runScript:(id)sender
> >+{
[...]
> >+    NSLog(@"Error loading script at %@: %@", [self scriptPath], [errDict valueForKey:NSAppleScriptErrorMessage]);
[...]
> >+    NSLog(@"Error running script at %@: %@", [self scriptPath], [errDict valueForKey:NSAppleScriptErrorMessage]);
[...]
> >+}
> 
> Obviously, these logs are probably for AppleScript devs, but we should consider
> #if'defing these for DEBUG. It is just an option I'm throwing out there.

Fair point, but I think these messages are more useful to end users than to Camino debuggers.  The logging is an alternative to alerting the user directly with a sheet, which was too harsh.

> r=me with those changes.

Thanks for the review.  Would you respond to the comments above before I spin a final patch?
Comment on attachment 277910 [details] [diff] [review]
Patch w/ Murph's Changes & Bundle Support

>+ * The Original Code is Camino code.
>+ *
>+ * The Initial Developer of the Original Code is The Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2007
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Peter Jaros <peter.a.jaros@gmail.com> (Original Author)

Is MoFo really the initial developer?  It isn't for most Camino code, but I don't know the details of the arrangement for this summer, so I wanted to check.
(In reply to comment #14)
> Is MoFo really the initial developer?  It isn't for most Camino code, but I
> don't know the details of the arrangement for this summer, so I wanted to
> check.

Yes, per our agreement, I am transferring copyright to the Foundation.
Attachment #277574 - Flags: review?(murph) → review+
> > These really aren't _static_, so lets pick a better name like
> > |toolbarIdentifiers|.
> 
> Ok, but the point is that they're not *all* of the toolbar identifiers.  Could
> we decide on a different qualifier than "static"?
> 

That's what I suggested in my comment.

> > >+  else if ([itemIdent hasPrefix:kScriptItemIdentifierPrefix]) {
> > >+    // Make a new ToolbarScriptItem instead.
> > >+    toolbarItem = [[[ToolbarScriptItem alloc] initWithItemIdentifier:itemIdent] autorelease];
> > >+  }
> > 
> > Comment isn't really beneficial here.
> 
> That comment is there to match similar comments above, eg:
> 

It's still not beneficial. If the other comments above aren't beneficial, then let's remove them as well.

> > // create a new toolbar item that knows how to do validation
> > toolbarItem = [[[ToolbarViewItem alloc] initWithItemIdentifier:itemIdent] autorelease];
> 
> I assumed the style here was to comment if we need to replace toolbarItem.  Is
> this a different case?
> 

Like the comment above, it's not beneficial. Anyone can read this code and understand why a |ToolbarViewItem| is getting created.

> > >+  // -nextObject yields relative paths.  We need absolute paths.
> > >+  while ((eachRelativePath = [dirEnum nextObject]))
> > >+  {
> > 
> > For type safety, do this:
> >   while ((eachRelativePath = [dirEnum nextObject]) != null) { ...
> 
> That's not the style used in the rest of the project; search on "nextObject".
> 

It's not necessarily a style thing, but since almost every while loop is written that way...

> > Style nit: Put your |return nil| calls on a new line.
> > Actually, I would like to see this method w/o early returns.
> > 
> > if ([ident hasPrefix:kScriptItemIdentifierPrefix] && [super
> > initWithItemIdentifier:ident]) {
> >   ...
> > }
> 
> You'd rather see the whole thing inside a conditional than just bail on the
> initial checks?

Yes, that's why I made the comment ;-) 

> > 
> > >+- (void)runScript:(id)sender
> > >+{
> [...]
> > >+    NSLog(@"Error loading script at %@: %@", [self scriptPath], [errDict valueForKey:NSAppleScriptErrorMessage]);
> [...]
> > >+    NSLog(@"Error running script at %@: %@", [self scriptPath], [errDict valueForKey:NSAppleScriptErrorMessage]);
> [...]
> > >+}
> > 
> > Obviously, these logs are probably for AppleScript devs, but we should consider
> > #if'defing these for DEBUG. It is just an option I'm throwing out there.
> 
> Fair point, but I think these messages are more useful to end users than to
> Camino debuggers.  The logging is an alternative to alerting the user directly
> with a sheet, which was too harsh.
> 

The more I think about it, the more I think we should #ifdef something here, and document it. If we can |#ifdef CAMINO_APPLESCRIPT_LOG| (or whatever) we can allow a applescript developer to run Camino with that environmental variable.
(In reply to comment #16)
 
> The more I think about it, the more I think we should #ifdef something here,
> and document it. If we can |#ifdef CAMINO_APPLESCRIPT_LOG| (or whatever) we can
> allow a applescript developer to run Camino with that environmental variable.

If we want to do this (it does sound sorta nifty), it's more appropriate as a separate bug.

Right now with the patch (per design discussion with smorgan in #camino) if a toolbar script item fails to run/errors, we just beep and log stuff to the console rather than throwing a sheet, which is good.  Users need to have some way of finding information about the error--either on their own, or when working with a toolbar script developer--and the console.log is the only reasonable way to do that.  And that's what these errors do.  You won't see them in normal usage with working buttons, so they won't spam the console and don't need to be debug-only.
(In reply to comment #16)
> The more I think about it, the more I think we should #ifdef something here,
> and document it. If we can |#ifdef CAMINO_APPLESCRIPT_LOG| (or whatever) we can
> allow a applescript developer to run Camino with that environmental variable.

Huh? #ifdef is a compiler directive; if we #ifdef it then AS developers would have to rebuild Camino themselves from scratch with the define set.
(In reply to comment #18)
> (In reply to comment #16)
> > The more I think about it, the more I think we should #ifdef something here,
> > and document it. If we can |#ifdef CAMINO_APPLESCRIPT_LOG| (or whatever) we can
> > allow a applescript developer to run Camino with that environmental variable.
> 
> Huh? #ifdef is a compiler directive; if we #ifdef it then AS developers would
> have to rebuild Camino themselves from scratch with the define set.
> 

Logging macros are already setup:
http://mxr.mozilla.org/seamonkey/source/modules/plugin/base/public/nsPluginLogging.h#44

We just have to wrap our conditional in a:
#if defined(PR_LOGGING)
  ...
#endif

statement.
Attached patch Patch w/ Nick's Changes (obsolete) — Splinter Review
Attachment #277910 - Attachment is obsolete: true
Attachment #278785 - Flags: superreview?(mikepinkerton)
Sorry, apparently Xcode didn't pick up the changes in the last version when I built it, and didn't catch my typos.  Here's a working version.
Attachment #278785 - Attachment is obsolete: true
Attachment #278796 - Flags: superreview?(mikepinkerton)
Attachment #278785 - Flags: superreview?(mikepinkerton)
+  if ([ident hasPrefix:kScriptItemIdentifierPrefix] && [super initWithItemIdentifier:ident])
+  {

braces on same line as |if|. several places in file.

+    return self;
+  }
+  else
+    return nil;

no else after return

+  if (!result) {
+    NSBeep();
+    NSLog(@"Error running script at %@: %@", [self scriptPath], [errDict valueForKey:NSAppleScriptErrorMessage]);
+    return;
+  }

do you need the return? there's nothing after this line.

sr=pink with those changes.
Comment on attachment 278796 [details] [diff] [review]
Patch w/ Nick's Changes That Works

sr with changes above
Attachment #278796 - Flags: superreview?(mikepinkerton) → superreview+
Attached patch Patch w/ Pink's Changes (obsolete) — Splinter Review
+ Pink's changes.  Finally.
Attachment #278796 - Attachment is obsolete: true
Comment on attachment 284071 [details] [diff] [review]
Patch w/ Pink's Changes

I went to check this in, and I noticed that bundled applet script items are showing up twice in the customize dialogue, once as themselves, and once as their main(.scpt) :(

I'm not sure what version of this patch I last tested, but it supported/recognized the bundled applets and didn't show their "main" as well.
Attached patch Final PatchSplinter Review
Sorry, Smokey.  Had a rogue method call in there.  Don't know why it wasn't a problem before, but now it's working.  (Knock on wood.)
Attachment #284071 - Attachment is obsolete: true
Keywords: checkin-needed
Landed on the trunk and 18branch, despite my best efforts to only land part of it :p
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: