Adium

Ticket #1099 (closed defect: fixed)

Opened 3 years ago

Last modified 3 months ago

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

Reported by: wootest Assigned to: wootest
Priority: low Milestone: Adium X 1.0
Component: Adium UI Version: 1.0
Severity: minor Keywords: link safari camino
Cc: wootest+adium@gmail.com evan@adiumx.com Patch:
Pending:

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

insertlinkpatch.zip (6.6 kB) - added by wootest on 08/05/2005 12:34:47 PM.
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 on 08/20/2005 04:38:34 PM.
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 on 12/15/2005 01:07:41 AM.
theres you firefox evands
firefoxsupport.diff (2.4 kB) - added by wengero on 12/15/2005 01:13:44 AM.

Change History

08/05/2005 12:34:47 PM changed 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.

08/05/2005 06:51:20 PM changed by evands

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

08/05/2005 08:14:08 PM changed 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.

08/05/2005 08:29:23 PM changed by 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).

08/05/2005 08:38:03 PM changed 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.

08/05/2005 09:02:08 PM changed by evands

  • status changed from new to closed.
  • resolution set to fixed.

(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

08/05/2005 09:04:01 PM changed by evands

  • status changed from closed to reopened.
  • resolution deleted.

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.

08/05/2005 09:06:20 PM changed by evands

  • milestone changed from Adium X 0.83 to Adium X 0.90.

08/05/2005 09:33:30 PM changed 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?

08/06/2005 12:08:09 AM changed by 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?

08/06/2005 02:20:26 PM changed by wootest

Good points - I reworked the code to that end.

08/19/2005 08:28:16 PM changed by evands

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

08/20/2005 04:38:34 PM changed by wootest

  • attachment insertlink-reflectdefault.diff added.

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

12/14/2005 05:00:29 PM changed by evands

  • status changed from reopened to closed.
  • resolution set to fixed.

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

12/15/2005 01:07:41 AM changed by wengero

  • attachment Safari.scpt added.

theres you firefox evands

12/15/2005 01:13:44 AM changed by wengero

  • attachment firefoxsupport.diff added.

12/15/2005 01:16:38 AM changed by wengero

  • field_haspatch changed.

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

12/16/2005 04:09:43 PM changed by evands

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

12/16/2005 04:49:55 PM changed by wengero

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

12/16/2005 04:54:35 PM changed by tick

Yes, Yes you must.

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

02/14/2006 07:07:25 PM changed by wootest

  • cc changed from wootest+adium@gmail.com to wootest+adium@gmail.com evan@adiumx.com.
  • keywords changed from link safari to link safari camino.
  • status changed from closed to reopened.
  • resolution deleted.
  • version changed from 0.83b to 1.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?

02/19/2006 02:54:47 AM changed by evands

  • status changed from reopened to closed.
  • resolution set to fixed.

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