Insert Link from Safari should support choosing link if several web browsers are open

[wootest]

Say you have Safari and OmniWeb open (the two browsers currently supported). You have no open window in Safari, but one in OmniWeb. The script 'eats' the OmniWeb link because Safari is open, and the link is picked in an if-elseif-else fashion. Even if you had one window open in Safari, you'd have to close it to get to pick the OmniWeb link.

[wootest]

Fixed in patch. (Patch includes tentative Camino support, which is commented out because Camino's support won't supply the right objects as of 0.9b2.) Also includes NetNewsWire support.

[evands]

Nice. Only change I'd recommend is that all strings (including those in the alert panel) need to use AILocalizedString().

[wootest]

Thanks.

The only strings that are actually shown are the OK/Cancel buttons in the choose list (they are automatically localized by the system), the names of the web browsers as prefixes to their respective pages (and Safari, OmniWeb, Camino and NetNewsWire are all recognized by their english names in any locale) and the actual prompt, which is being localized in the ESSafariLinkToolbarItemPlugin class and then fed to the AppleScript as a parameter to the function (localization of AppleScripts has got to be solved, by the way - passing parameters seems like a hack). I can't see any strings left to localize here.

I tried to have Adium show the choose list, by the way, and invariably it wouldn't show up. Same if I put it outside of the tell app "System Events" block.

[evands]

Very interesting. I applied your patch and then just did svn diff... my local copy has:

Index: ESSafariLinkToolbarItemPlugin.m
===================================================================
--- ESSafariLinkToolbarItemPlugin.m     (revision 12968)
+++ ESSafariLinkToolbarItemPlugin.m     (working copy)
@@ -77,7 +77,8 @@
 
        if (earliestTextView) {
                NSTask                  *scriptTask;
-               NSMutableArray  *applescriptRunnerArguments = [NSArray arrayWithObjects:SAFARI_LINK_SCRIPT_PATH, @"substitute", nil];
+               NSMutableArray  *applescriptRunnerArguments = 
+                       [NSArray arrayWithObjects:SAFARI_LINK_SCRIPT_PATH, @"substitute", AILocalizedString(@"Several browsers were open. Please select one link:", @"Prompt when more than one web browser was available to get a link from."), nil];
                NSString                *applescriptRunnerPath;
                NSPipe                  *standardOuptut = [NSPipe pipe];
                NSString                *scriptResult;
@@ -112,6 +113,9 @@
                        [earliestTextView insertText:attributedScriptResult];
                        if (attributes) [earliestTextView setTypingAttributes:attributes];
                        [attributes release];
+               } else {
+                       NSRunAlertPanel(@"A link of the frontmost web page in Safari could not be inserted.",@"This could be because Safari isn't displaying a page or Adium was unable to get the link from Safari.",@"OK",nil,nil);
+
                }
 
                [scriptResult release];

and I assumed that the full changeset was from your patch. Looking at the patch directly, though, you didn't add the NSRunAlertPanel(), which explains why my comment made no sense. :) I must have applied a patch from someone else at some point and not committed it (as I know that isn't a change I made directly).

[wootest]

That makes way more sense.

Shouldn't this function be changed to dynamically grab the icon and name of the default web browser and use that instead, so that it's clearer that more browsers are supported? I could do that.

[evands]

(In [13034]) Insert Link from Safari now supports choosing the link if multiple supported web browsers are open. Added NetNewsWire support (in addition to existing Safari and OmniWeb support).

Patch from wootest in #1099, with the following modifications:

• Changed the NSTask invocation to be non-blocking, instead usingNSTaskDidTerminateNotification
• This allowed modification of the new Safari.scpt to display the applescript dialogue box within Adium rather than via System Events
• Added NSBeep() calls if a link can't be inserted

Closes #1099

[evands]

Good idea. Too bad we can't do Firefox... but for the supported browsers it'd be good for it to show the default, and have a title reflecting where we expect to get the link. i.e. if Omniweb is default, indicate Omniweb info, but still do the dialog if Safari is also running, etc.

[wootest]

With the latest patch, the code now changes the toolbar item (post creation, to avoid messing up localization macros) to have the name and icon of the default browser if it's a supported one.

Still to do: moving the list from AppleScript to Adium - pop up a "completion" list instead, somehow?

[evands]

The latest patch will load the Safari icon even if the Omniweb icon should be used. Programming in general, and specifically in Cocoa, is best if lazy:

1. set it to nil initially.
2. attempt to find the default and supported broswer.
3. after doing that, check that it's not nil; if it is nil, set it to the icon for Safari.

Does that make sense?

[wootest]

Good points - I reworked the code to that end.

[evands]

The patch fails to apply against current svn ([13157])... one chunk is rejected. Could you please upload an updated patch?

[wootest]

Toolbar item reflects default browser (if it's one that can be handled) (take three: patch actually works, supposedly)

[evands]

(In [14425]) The Insert Safari Link toolbar item now takes the icon and name of the default browser if it is one we specify supporting. I'd love for someone to make Firefox work (fix the Safari.scpt in Resources to handle it, then uncomment @"Firefox" in this code). Patch from wootest with modifications by me. Closes #1099.

[wengero]

theres you firefox evands

[wengero]

hmm my patch came out funny looking, all it does is make it accept my firefox script

[evands]

(In [14459]) Along with [14458], patch from wengero which adds Firefox support for the Insert Link toolbar button. Thanks :). Refs #1099

[wengero]

shall i keep going and add the other browsers from my xtra(and a few i never got around to adding)?

[tick]

Yes, Yes you must.

Although, everyone uses Safari. Nobody would ever use the others, ever.

[wootest]

I just want to note that as of 1.0 (and quite possibly a bit earlier - I haven't tried all that often), this part of Camino's AppleScript support is no longer broken.

The hook involves something along the lines of (and this is a standalone proof that the support now works, by the way, feel free to run it):

tell application "Camino"
	if version >= 1.0 then
		set urlvar to URL of front window
	end if
end tell

I am not in a position to type the version of the script that would be used myself currently, but this should provide enough info for anyone to do it within a minute or two, especially since I remember writing it up and trying it until realizing that the support wasn't in yet.

Thusly I'm reopening this ticket temporarily. Evan?

[evands]

(In [15263]) Updated for Camino 1.0 and later. Closes #1099