200040 - The "Send Link" menu item should send URLs in "<>"s

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, enhancement, P4)

Description

[Matthias Damm]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4a) Gecko/20030331 Chimera/0.7+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4a) Gecko/20030331 Chimera/0.7+

Camino's "Send Línk" feature hands the URL to the Mail app as http://www.url.com.
Instead of this, <http://www.url.com> should be used.

This would solve two (small) problems:
- long URLs which don't fit into one line might be split up by some mail
readers, or will not be quoted correctly (although most mail readers should know
format=flowed today). 
Especially, a bug in Mail.app can cause text in the line after a URL without
"<>"s to be added to the URL. URLs in "<>" are used correctly by most mail apps.
One Mac app which has problems with URLs without "<>"s is the Mail/News-Reader
app MacSOUP.

- People might add text to the new Email, and forget to leave a blank after the
http://www.url.com, which obviously can cause problems. 

Using the <http://www.url.com> format will solve these problems very easily,
therefore this form should be prefered.

Reproducible: Always

Steps to Reproduce:
1. Use the "Send Link" command
Actual Results:  
A Mail with a link in the http://www.url.com is created.

Expected Results:  
The <http://www.url.com> format should be used instead.

Comment 1

[Greg K.]

Matthias, does Mozilla's Send Link... feature operate the same as Camino's?

Comment 2

[Matthias Damm]

Greg: Yes, Mozilla works the same way.

Comment 3

[Greg K.]

I'll confirm as an RFE, then. If this is done, it should be done for both
Mozilla and Camino.

Comment 4

[J Luh]

Actually Mozilla adds the <> brackets around the link when you choose Send Link
from Navigator. Camino behaves differently from Mozilla. (I agree Camino should
be changed to match the Mozilla behavior).

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Chris, wanna look and see how hard this is?

Comment 6

[Chris Lawson (gone)]

Without looking at the code, I suspect this is going to be REDONKULOUSLY easy. I'll get a patch in today.

cl

Comment 7

[Chris Lawson (gone)]

Attachment: whitespace changes ignored for review

Patch with some minor whitespace changes forthcoming.

cl

Comment 8

[Chris Lawson (gone)]

Attachment: patch with whitespace changes for checkin

Comment 9

[Stuart Morgan]

Comment on attachment 216249 [details] [diff] [review]
whitespace changes ignored for review

+  if (!titleString)
+    titleString = @"";
+  if (!urlString)
+    urlString   = @"";
+  else
+    urlString = [NSString stringWithFormat:@"<%@>", urlString];

First, put a blank line between the two |if| clauses so it doesn't look at first glance like you meant the second to be an |else if|.  Second, |if (!<expected thing>) ... else ...| is ugly.  Reverse the logic.

Lastly, I really don't see why this continues at all in the !urlString case.  If there's no URL, why open a new email? sendURLFromLink: bails in that case, so I think this should too.

+  NSString* urlString = [NSString stringWithFormat:@"<%@>",[NSString stringWith_nsAString:href]];

Spaces after commas. I expect better from the space nazi ;)

Comment 10

[Stuart Morgan]

Of course, if you are bailing for an empty URL that check should go before all the string tweaking, so ignore the part about reversing the if/else.

Comment 11

[Chris Lawson (gone)]

Attachment: fixes Stuart's comments

Comment 12

[Chris Lawson (gone)]

Attachment: fixed, with whitespace changes

Comment 13

[Stuart Morgan]

Comment on attachment 216262 [details] [diff] [review]
fixes Stuart's comments

+  if (!urlString) // bail like we do in the context menu case

This comment isn't necessary.

+  // put < > around the URL to minimise problems when e-mailing

And I'd vote for the american spelling of minimize.

But as neither of those things actually matter, r=me. I didn't actually test it, but since it is, as was pointed out above, a redonkulously minor change, I trust that it works.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 216263 [details] [diff] [review]
fixed, with whitespace changes

In the spirit of dotting every i and crossing every t, even on redonkulously simple patches, I tested this patch, and it does indeed work as described ;)

Comment 15

[Stuart Morgan]

I move that the checkin comments for this bug include the word 'redonkulously'.  Something like "Redonkulously simple patch by Chris Lawson"

Comment 16

[Chris Lawson (gone)]

Comment on attachment 216262 [details] [diff] [review]
fixes Stuart's comments

sr=mento?

Please check in the other one ;)

I second Stuart's motion.

cl

Comment 17

[Mark Mentovai]

Comment on attachment 216263 [details] [diff] [review]
fixed, with whitespace changes

Please request sr on the patch you actually want checked in and mark the old one obsolete.  That will ease your chickenmaster's task.

Comment 18

[Mark Mentovai]

trunk v1.239, 1.8 branch v1.201.2.33.

Comment 19

[Wayne Mery (:wsmwk)]

*** Bug 303074 has been marked as a duplicate of this bug. ***

Comment 20

[Samuel Sidler (old account; do not CC)]

Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.