Adium

Opened 15 years ago

Closed 15 years ago

Last modified 12 years ago

#1099 closed defect (fixed)

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

Reported by: wootest Owned by: wootest
Milestone: Adium X 1.0 Component: Adium UI
Version: Severity: minor
Keywords: link safari camino Cc: wootest+adium@… evan@…
Patch Status:

Description

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.

Attachments (4)

insertlinkpatch.zip (6.6 KB) - added by wootest 15 years ago.
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.
insertlink-reflectdefault.diff (8.2 KB) - added by wootest 15 years ago.
Toolbar item reflects default browser (if it's one that can be handled) (take three: patch actually works, supposedly)
Safari.scpt (13.7 KB) - added by wengero 15 years ago.
theres you firefox evands
firefoxsupport.diff (2.4 KB) - added by wengero 15 years ago.

Download all attachments as: .zip

Change History (22)

Changed 15 years ago by wootest

Attachment: insertlinkpatch.zip added

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.

comment:1 Changed 15 years ago by Evan Schoenberg

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

comment:2 Changed 15 years ago by 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.

comment:3 Changed 15 years ago by Evan Schoenberg

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).

comment:4 Changed 15 years ago by 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.

comment:5 Changed 15 years ago by Evan Schoenberg

Resolution: fixed
Status: newclosed

(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

comment:6 Changed 15 years ago by Evan Schoenberg

Resolution: fixed
Status: closedreopened

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.

comment:7 Changed 15 years ago by Evan Schoenberg

Milestone: Adium X 0.83Adium X 0.90

comment:8 Changed 15 years ago by 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?

comment:9 Changed 15 years ago by Evan Schoenberg

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?

comment:10 Changed 15 years ago by wootest

Good points - I reworked the code to that end.

comment:11 Changed 15 years ago by Evan Schoenberg

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

Changed 15 years ago by wootest

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

comment:12 Changed 15 years ago by Evan Schoenberg

Resolution: fixed
Status: reopenedclosed

(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.

Changed 15 years ago by wengero

Attachment: Safari.scpt added

theres you firefox evands

Changed 15 years ago by wengero

Attachment: firefoxsupport.diff added

comment:13 Changed 15 years ago by wengero

field_haspatch: 0

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

comment:14 Changed 15 years ago by Evan Schoenberg

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

comment:15 Changed 15 years ago by wengero

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

comment:16 Changed 15 years ago by Chris Forsythe

Yes, Yes you must.

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

comment:17 Changed 15 years ago by wootest

Cc: wootest+adium@… added; wootest+adium@… removed
Keywords: camino added
Resolution: fixed
Status: closedreopened
Version: 0.83b1.0

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?

comment:18 Changed 15 years ago by Evan Schoenberg

Resolution: fixed
Status: reopenedclosed

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

Note: See TracTickets for help on using tickets.