Closed
Bug 557057
Opened 13 years ago
Closed 11 years ago
Can't "reload current tab" with AppleScript
Categories
(Camino Graveyard :: OS Integration, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: web, Assigned: alqahira)
Details
(Whiteboard: [camino-2.1.1][needs rdar:// on comment 14])
Attachments
(3 files, 6 obsolete files)
18.58 KB,
text/plain
|
Details | |
9.22 KB,
patch
|
stuart.morgan+bugzilla
:
superreview+
|
Details | Diff | Splinter Review |
8.42 KB,
text/plain; charset=utf16
|
Details |
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.19pre) Gecko/2010032100 Camino/2.1a1pre (like Firefox/3.0.19pre) Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.19pre) Gecko/2010032100 Camino/2.1a1pre (like Firefox/3.0.19pre) I see this isn't implemented in Safari either, so maybe it isn't trivial, but I'd like to be able to 'tell application "Camino" to reload the current tab' or something similar, to refresh the current page. Reproducible: Always
Reporter | ||
Updated•13 years ago
|
Summary: Can't "make reload current tab" with AppleScript → Can't "reload current tab" with AppleScript
Assignee | ||
Comment 1•13 years ago
|
||
This is probably do-able, and seems reasonable to me. In the meantime, there's a work-around that's just one more line of code (given the way we currently behave when told by an external app to open URLs that are already open): tell application "Camino" set theURL to URL of current tab of browser window 1 open location theURL end tell Inelegant, but functional.
Reporter | ||
Comment 2•13 years ago
|
||
Thanks, that works exactly as needed! If you could give me wiki access, I'd happily add this to: http://wiki.caminobrowser.org/Development:Camino_AppleScript_Guide ?
Assignee | ||
Comment 3•13 years ago
|
||
Peter actually had a stub reload in the original .sdef (http://hg.mozilla.org/camino/rev/87b3d60e08fe#l4.248), but it wasn't the child of either browser window or tab, so it wouldn't even work (and was commented out, at that). I expect it was probably on the SoC project agenda at some point and later waylaid?
Comment 4•12 years ago
|
||
Yeah, I don't see any reason not to do this either.
Assignee | ||
Comment 5•12 years ago
|
||
I've worked on this bug on and off, never with any success. This is the latest attempt, and iirc it still didn't make the command show up properly in the dictionary (let alone work), but I do think it compiled :P Not sure what I'm missing, but someone with more AS know-how is needed ;)
Assignee | ||
Updated•12 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 6•11 years ago
|
||
From doing some reading, at least part of my problem before was that I wasn't implementing the Cocoa code for the command correctly; in particular, the method should have been: - (void)scriptableReload:(NSScriptCommand*)command or similar.
Assignee | ||
Comment 7•11 years ago
|
||
Here's a skeleton of a fix, finally with working AppleScript dispatch. With this patch, the following commands are successfully dispatched to the appropriate methods in ScriptingSupport.mm: tell application "/path/to/patched/Camino.app" reload tab 1 of browser window 1 reload browser window 1 tell tab 1 of browser window 1 to reload tell browser window 1 to reload end tell I don't like the duplication of methods, but I don't think there's a way for browser window to implicitly inherit reload like it implicitly inherits all of the properties defined in the .sdef only for tab (not defining responds-to for BrowserWindow leads the AS to fail to run, and not adding the method in ScriptingSupport causes nothing to get called when the command is run). So, at this point, what needs to be done is: 1) Figure out what we're asking to reload, and see if it can reload 2) Throw errors as appropriate (if we get bad input; if we have a non-reloadable tab, I think we want to fail silently rather than throw a script error). 3) Reload!
Attachment #500632 -
Attachment is obsolete: true
Assignee | ||
Comment 8•11 years ago
|
||
Well, I have this all implemented, with two problems: 1) Edge cases like "reload every tab of every browser window" and "reload tabs 1 through 2 of browser windows 2 through 3" don't yet work, because they are nested arrays, and I haven't unpacked the second level yet (thankfully, we can only have two levels!). 2) Whenever multiple (N) items are asked to reload, our method gets called N times, every single time *with the same array(s) of N items*! So "reload tabs 1 through 5 of browser window 1" gets called 5 times, each time listing the same 5 tabs to be reloaded! Bill Cheeseman basically describes the problem in http://lists.apple.com/archives/applescript-implementors/2005/May/msg00043.html, though he seems to believe it was tied to a specific 10.3 AppleScript behavior. He does outline a solution, though, which is to somehow keep a count of the number of items and only process the Nth invocation. I'm not sure how to code that exactly, but presumably it could be done. In practical terms, based on watching something like http://www.comptechdoc.org/independent/web/cgi/javamanual/javaihit.html, it doesn't seem like we're actually reloading N times, even though we're clearly calling reload N times, so I'm not sure how much of a problem that might be in reality. 3) The arrays we're being sent are apparently NSCFArrays, but ScriptingSupport doesn't import whatever header is needed to know what the class of NSCFArray is (and the internet is surprisingly light on useful info about said array type); however, testing to see if an object is an NSArray seems to work OK here. The good news is that KVC appears to handle, in the single-tab and single-window cases, the index-out-of-bounds ("valid tab") problem before those instances ever hit our reload handler, and alerts the user in Script Editor. (However, in a multi-tab or multi-window case, those commands just fail silently, e.g. "reload tabs 1 through 5 of browser window 1" where said window has only 4 tabs.)
Attachment #592284 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to comment #8) > In practical terms, based on watching something like > http://www.comptechdoc.org/independent/web/cgi/javamanual/javaihit.html, it > doesn't seem like we're actually reloading N times, even though we're > clearly calling reload N times, so I'm not sure how much of a problem that > might be in reality. I also put http://www.ardisson.org/smokey/moz/ as one of my test tabs and watched my server logs, and we're definitely not actually reloading twice, at least not fast enough for my server to tell. I haven't put any logging down the pipes beyond BrowserWrapper's reload: (which is being called twice), so I don't know if something in CHBV or further down in Gecko is stopping the spurious additional reloads, or what. This WIP iteration also tweaks the .sdef a little and implements support for an "ignoring cache" parameter (which I've also verified works as expected, based on response size in my server logs). Still to do: deal with the nested arrays, and possibly try to short-circuit the extra calls.
Assignee | ||
Comment 10•11 years ago
|
||
This patch properly handles nested arrays (and always type-checks the output of an enumerated array's nextObject to make sure it's the expected type for the given code branch, so that we never assume what class of object we're being given.). I walked well into the guts of Gecko trying to see where (or verify if) Gecko is stopping redundant loads; although there were several promising code-hunks, none of them were being hit. After spending some time tracing load codepaths with a spinning head, I gave up. :-( So, I'm happy to try to implement a short-circuit counter à la comment 8 point 2 in our code given some guidance/pointers, but I think, practically speaking (per testing in comment 8 and comment 9), we're safe without it.
Attachment #592474 -
Attachment is obsolete: true
Attachment #593733 -
Flags: superreview?(stuart.morgan+bugzilla)
Assignee | ||
Comment 11•11 years ago
|
||
Lisa, can you have a look at this script and see if there's anything you can think of that I didn't test but should have (either that should work or that people might try and which we should handle correctly [fail safely] even if we don't support)? The script obviously won't compile properly in (Apple)Script Editor without a patched Camino, but I can make one if you'd like to take it for a spin.
Attachment #593734 -
Flags: feedback?(lthompson.22)
Comment 12•11 years ago
|
||
AFAICT you've covered all the permutations for identifying tab/window. I don't know if referring to tab/window by variable needs to be tested independently set theTab to current tab of front browser window reload theTab since it works in other contexts. The second thing that comes to mind as far as what people might try, is to stick the command in a repeat loop. I don't know if that's worth testing for unforeseen consequences, beyond any that would already apply to running "open location" in a repeat loop. Anyway, if you'd like to provide me a build, I would be happy to hammer on it. :-)
Assignee | ||
Comment 13•11 years ago
|
||
(In reply to Lisa Thompson from comment #12) > AFAICT you've covered all the permutations for identifying tab/window. I > don't know if referring to tab/window by variable needs to be tested > independently > > set theTab to current tab of front browser window > reload theTab > > since it works in other contexts. Yeah, that works, since theTab is just a reference to the underlying tab object. > as what people might try, is to stick the command in a repeat loop. I don't > know if that's worth testing for unforeseen consequences, beyond any that > would already apply to running "open location" in a repeat loop. A loop is just going to send us a series of one of the other underlying commands, so as long as there are not any variations on those we haven't accounted for, a loop won't matter. > Anyway, if you'd like to provide me a build, I would be happy to hammer on > it. :-) http://www.ardisson.org/smokey/moz/Camino-2.1.1pre-i386-applescript.dmg (includes changes for both this bug and bug 718322)
Comment 14•11 years ago
|
||
I put the build and your test script through some paces and everything I could think of as far as identifying the tabs to be reloaded worked. But I did find one oddity. If I append "ignoring cache no" to any valid "reload" command, I get an error on run with the message "can't make no into type boolean". Event log says: error "Camino got an error: Can’t make no into type boolean." number -1700 from no to boolean If I change the clause to "without ignoring cache", it runs fine. Same thing happens with appending "loading in background no" to an open location command. Now, I never use the "yes/no" syntax, always "true/false" and "with/without", so maybe I'm missing something here?
Assignee | ||
Comment 15•11 years ago
|
||
Hrm, I tested the yes/no syntax with both "reload ignoring cache" and "open location in background" before and they both worked fine. I wonder if something changed between OS versions? Thanks for finding this :) I'll try to dig into it later.
Assignee | ||
Comment 16•11 years ago
|
||
Lisa, can I get you to run some tests for me with some logging? (Also, what OS version is this?) 1) In Terminal: defaults write org.mozilla.camino NSScriptingDebugLogLevel 1 2) Launch the Camino that has these commands 3) Open the Console 4) Run an AppleScript command with yes/no syntax and one with true/false (with/without) 5) Console should log something that looks like this: Command: Camino Suite.reload\n Direct Parameter: <NSIndexSpecifier: tabs 1 of allBrowserWindows 1>\n Receivers: <NSIndexSpecifier: tabs 1 of allBrowserWindows 1>\n Arguments: {\n ignoringCache = 0;\n } 6) When you're done, you can defaults delete org.mozilla.camino NSScriptingDebugLogLevel to keep from logging every script command you ever run against Camino ;-) If you don't get that logging (I don't know where in the process the error is happening, and where in the process that logging is being generated), can you also try the extremely verbose AEDebugReceives? 1) In Terminal: env AEDebugReceives=1 open /path/to/with-reload/copy/of/Camino.app 2) Open the Console 3) Run an AppleScript command with yes/no syntax and one with true/false (with/without) 4) Console will log a giant spammy serialization of the raw AppleEvent: AE2000 (4340): Received an event: ------oo start of event oo------ { 1 } 'aevt': MOZC/reld (i386){ return id: 276299876 (0x10780064) transaction id: 0 (0x0) interaction level: 64 (0x40) reply required: 1 (0x1) remote: 0 (0x0) for recording: 0 (0x0) reply port: 52231 (0xcc07) target: { 1 } 'psn ': 8 bytes { { 0x0, 0x1c21c2 } (Script Editor) } fEventSourcePSN: { 0x0,0x1c21c2 } (Script Editor) optional attributes: { 1 } 'reco': - 1 items { key 'csig' - { 1 } 'magn': 4 bytes { 65536l (0x10000) } } event data: { 1 } 'aevt': - 2 items { key 'iCch' - { 1 } 'fals': 0 bytes { false } key '----' - { 1 } 'obj ': - 4 items { key 'form' - { 1 } 'enum': 4 bytes { 'indx' } key 'want' - { 1 } 'type': 4 bytes { 'BTab' } key 'seld' - { 1 } 'long': 4 bytes { 1 (0x1) } key 'from' - { 1 } 'obj ': - 4 items { key 'form' - { 1 } 'enum': 4 bytes { 'indx' } key 'want' - { 1 } 'type': 4 bytes { 'BWin' } key 'seld' - { 1 } 'long': 4 bytes { 1 (0x1) } key 'from' - { -1 } 'null': null descriptor } } } } I noticed in my testing here that the data for "no" is an 'enum' of 'no ' whereas the data for "false"/"without" is a 'fals' of 'false' in the raw AppleEvent, but both are properly coerced into '0' by the time we reach Cocoa Scripting (NSScriptDebugLogging). So, based on the data I have so far, the working hypothesis is that something is broken on the OS level where yes/no aren't being properly coerced into 1/0 before handing them off to Camino's Cocoa Scripting support. Oh, crazy question: have you tried rebooting, or using another user account? I've noticed sometimes (more often than I'd like :/ ) my AppleScript runtime gets b0rked and the most basic commands start failing miserably (I suspect an exception has occurred, hasn't been caught, and has run amok in the runtime). Do other applications with boolean params also fail? E.g. tell application "TextEdit" to print document 1 print dialog yes
Assignee | ||
Comment 17•11 years ago
|
||
Oh, Console might already be logging a condensed form of the raw AppleEvent on error when NSScriptingDebugLogLevel is 1 (I just tried with actual 1/0):
> Error converting apple event to script command: -1700
> Original event: <NSAppleEventDescriptor: 'MOZC'\'reld'{ 'iCch':0, '----':'obj '{ 'form':'indx', 'want':'BTab', 'seld':1, 'from':'obj '{ 'form':'indx', 'want':'BWin', 'seld':1, 'from':'null'() } }, &'csig':65536 }>
> Offending object descriptor: <NSAppleEventDescriptor: 0>
> Expected type descriptor: <NSAppleEventDescriptor: 'bool'>
However, it doesn't tell us what the type is like the big spammy AEDebugReceives logging does, and I think I'd like to know what that type is.
Comment 18•11 years ago
|
||
Mac OS X 10.6.8. Installed the latest from Software Update, rebooted, and repaired permissions. Still get the error. Console output from the first set of tests looked like Comment 17 for the "ignoring cache no" case. Below is the output from the second set of tests for the "ignoring cache no" case. I will attach the full results from both tests for all cases. Let me know if you still think this needs to be done on a fresh user account, I would have to get to it tomorrow. env AEDebugReceives=1 open /Applications/Camino.app "reload current tab of front browser window ignoring cache no" { 1 } 'aevt': MOZC/reld (i386){ return id: 331 (0x14b) transaction id: 0 (0x0) interaction level: 64 (0x40) reply required: 1 (0x1) remote: 0 (0x0) for recording: 0 (0x0) reply port: 433463 (0x69d37) target: { 1 } 'psn ': 8 bytes { { 0x0, 0x1e01e } (AppleScript Editor) } fEventSourcePSN: { 0x0,0x1e01e } (AppleScript Editor) optional attributes: { 1 } 'reco': - 1 items { key 'csig' - { 1 } 'magn': 4 bytes { 65536l (0x10000) } } event data: { 1 } 'aevt': - 2 items {
Comment 19•11 years ago
|
||
Logging for applescript runs of "reload" command with various usage of "ignoring cache"
Comment 20•11 years ago
|
||
Forgot to answer this.
> Do other applications with boolean params also fail?
Yesterday I did the sanity check with
tell application "Finder" to duplicate file "x" to "y" replacing yes/no
and it ran as expected. Your TextEdit script
tell application "TextEdit" to print document 1 print dialog yes/no
also worked both ways with no problems.
Assignee | ||
Comment 21•11 years ago
|
||
(In reply to Lisa Thompson from comment #18) Thanks! > event data: > { 1 } 'aevt': - 2 items { The fact this AppleEvent it truncated is never a good sign :-( Anyway, based on your output, philippe's output on 10.7.3, the output of some additional logging I had philippe do for me, and mountains of heroic remote testing from philippe (we can reproduce this in a stock Apple sample code application, SimpleScriptingVerbs, including the default-compiled-for-10.6 binary Apple shipped) I'm absolutely certain this is an OS bug, and it appears to be related to use of .sdef instead of still(!) using the old-style .scriptTerminology and .scriptSuite files. Since this is an OS bug, since it's a non-default syntax (according to Apple's docs, the with/without syntax is supposed to be the default), and since it errors inside Cocoa Scripting before it gets to our code (so there's nothing we can do about it), I don't think we should do anything here. We could go back to converting the .sdef into three pairs of old-style .scriptTerminology and .scriptSuite files and the aete resource, but that's both ugly and a bit of a pain, and if this broke default syntax I'd do it, but for this, I don't think it's worth it. Big thanks, Lisa and philippe, for all your help tracking down this Apple bug :) In my copious free time, I'll reduce SimpleScriptingVerbs down to a minimal testcase and file a rdar:// with Apple.
Whiteboard: [needs rdar:// on comment 14]
Comment 22•11 years ago
|
||
Comment on attachment 593733 [details] [diff] [review] Working patch, including force-reload support Review of attachment 593733 [details] [diff] [review]: ----------------------------------------------------------------- ::: src/appleevents/ScriptingSupport.mm @@ +317,5 @@ > > +- (void)handleReloadScriptCommand:(NSScriptCommand *)command > +{ > + id browserWindowsToReload = [command evaluatedReceivers]; > + BOOL cacheFlag = [[[command evaluatedArguments] objectForKey:@"ignoringCache"] boolValue]; cacheFlag is confusing; call it ignoreCache @@ +318,5 @@ > +- (void)handleReloadScriptCommand:(NSScriptCommand *)command > +{ > + id browserWindowsToReload = [command evaluatedReceivers]; > + BOOL cacheFlag = [[[command evaluatedArguments] objectForKey:@"ignoringCache"] boolValue]; > + BrowserWindow *windowToReload; Since this isn't actually used at top-level scope, move this to just before the while ((windowToReload = ...)) line, and add the type declaration to the windowToReload in the if. @@ +328,5 @@ > + } > + else if ([browserWindowsToReload isKindOfClass:[NSArray class]]) { > + NSEnumerator *windowsEnumerator = [browserWindowsToReload objectEnumerator]; > + while ((windowToReload = [windowsEnumerator nextObject])) { > + if ([windowToReload isKindOfClass:[BrowserWindow class]]) { This whole if/else is really just the outer block repeated again. Make a helper: - (void)handleReloadForObject:(id)objectToReload ignoringCache:(BOOL)ignoreCache; where the implementation is roughly: if (<class is array>) { id objectToReload; while ((objectToReload = <next object>)) { [self handleReloadForObject:objectToReload ignoringCache:ignoreCache]; } } else { <do the reload/error handling> } Then have this script method just call that with the pieces of the script call. @@ +337,5 @@ > + } > + else { > + // This shouldn't ever happen, but if it does, we need to hear about it. > + [[NSScriptCommand currentCommand] setScriptErrorNumber:NSArgumentsWrongScriptError]; > + NSString *scriptingClassName = [(NSScriptClassDescription *)[NSScriptClassDescription classDescriptionForClass:[windowToReload class]] className]; Are these typecasts really necessary? The docs imply that NSScriptClassDescription classDescriptionForClass: should return that type already. @@ +400,5 @@ > [[self nativeWindow] performClose:self]; > return nil; > } > > +- (void)handleReloadScriptCommand:(NSScriptCommand *)command Same restructuring here. @@ +454,5 @@ > + [[NSScriptCommand currentCommand] setScriptErrorString:[NSString stringWithFormat:@"A %@ can't be reloaded.", scriptingClassName]]; > + } > +} > + > +- (void)scriptableReload:(BOOL)cacheFlag ignoreCache
Attachment #593733 -
Flags: superreview?(stuart.morgan+bugzilla) → superreview-
Assignee | ||
Comment 23•11 years ago
|
||
(In reply to Stuart Morgan from comment #22) > > + else if ([browserWindowsToReload isKindOfClass:[NSArray class]]) { > > + NSEnumerator *windowsEnumerator = [browserWindowsToReload objectEnumerator]; > > + while ((windowToReload = [windowsEnumerator nextObject])) { > > + if ([windowToReload isKindOfClass:[BrowserWindow class]]) { > > This whole if/else is really just the outer block repeated again. Make a > helper: It felt like there had to be a better way than the semi-repetitive nested |if|s, but a recursive helper method didn't occur to me :) I like this a whole lot better; it's clearer and saner, thanks! I consolidated the comments to the beginning of the helper methods, because even without the nesting, I think it's still very helpful to have a reference to the types of receivers we should expect and what syntaxes produce them. > > + [[NSScriptCommand currentCommand] setScriptErrorNumber:NSArgumentsWrongScriptError]; > > + NSString *scriptingClassName = [(NSScriptClassDescription *)[NSScriptClassDescription classDescriptionForClass:[windowToReload class]] className]; > > Are these typecasts really necessary? The docs imply that > NSScriptClassDescription classDescriptionForClass: should return that type > already. It looks like Peter cargo-culted that from haas, and I cargo-culted it from him ;-) I zapped them and forced the error condition, and it does appear they are not necessary when running on 10.5 and built with 10.4u SDK; perhaps in the past they were required? Apple's sample code[1,2] does use the cast, but both examples are for initWithContainerClassDescription: rather than classDescriptionForClass:, so they're inconclusive. [1] https://developer.apple.com/library/mac/#samplecode/SimpleScriptingPlugin/Listings/Element_m.html [2] https://developer.apple.com/library/mac/#samplecode/SimpleScriptingObjects/Listings/Element_m.html Also, this hunk: -- (void)setCurrentTab:(BrowserWrapper *)newTabItemView; +- (void)setCurrentTab:(BrowserWrapper *)newTabItemView was a typo I found (how did this ever compile/work before?); I'll check it in separately.
Attachment #593733 -
Attachment is obsolete: true
Attachment #597296 -
Flags: superreview?(stuart.morgan+bugzilla)
Assignee | ||
Comment 24•11 years ago
|
||
While looking up the various places in Apple's docs that mentioned the NSScriptClassDescription, I came upon the list of object specifier types again, and I added some more. Once I got the right syntax down ("whose" always requires a plural subject, even though Script Editor is happy to compile with a singular subject :P), pretty much every one worked. There are a couple of cases that didn't, but the bug is not in reload, but rather in some object specifier support, which I filed as bug 727374. They're pretty esoteric cases--far much moreso than yes/no--so I don't think anyone will hit them, in regular use or with "reload".
Attachment #593734 -
Attachment is obsolete: true
Attachment #593734 -
Flags: feedback?(lthompson.22)
Comment 25•11 years ago
|
||
Comment on attachment 597296 [details] [diff] [review] v4.6, sr comments addressed Review of attachment 597296 [details] [diff] [review]: ----------------------------------------------------------------- sr=smorgan with nits ::: src/appleevents/ScriptingSupport.mm @@ +434,5 @@ > +- (void)scriptableReload:(BOOL)ignoreCache > +{ > + unsigned int reloadFlag = NSLoadFlagsNone; > + if (ignoreCache) > + reloadFlag = NSLoadFlagsBypassCacheAndProxy; I think these three lines would be more readable as a ternary: unsigned int reloadFlag = ignoreCache ? NSLoadFlagsBypassCacheAndProxy : NSLoadFlagsNone; @@ +435,5 @@ > +{ > + unsigned int reloadFlag = NSLoadFlagsNone; > + if (ignoreCache) > + reloadFlag = NSLoadFlagsBypassCacheAndProxy; > + if ([self canReload]) { I'd move this to the beginning and invert it to: if (![self canReload]) return; No reason to muck about with flags if you can't use them.
Attachment #597296 -
Flags: superreview?(stuart.morgan+bugzilla) → superreview+
Assignee | ||
Comment 26•11 years ago
|
||
(In reply to Stuart Morgan from comment #25) > I think these three lines would be more readable as a ternary: > unsigned int reloadFlag = ignoreCache ? > NSLoadFlagsBypassCacheAndProxy : NSLoadFlagsNone; Agreed. > I'd move this to the beginning and invert it to: > if (![self canReload]) > return; > > No reason to muck about with flags if you can't use them. Indeed; thanks! http://hg.mozilla.org/camino/rev/b9eb83be2017 with those two changes. Someone please remind me in a couple of weeks if I haven't filed the rdar:// yet.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs rdar:// on comment 14] → [camino-2.1.1][needs rdar:// on comment 14]
You need to log in
before you can comment on or make changes to this bug.
Description
•