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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
OS Integration
P4
enhancement
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: Matthias Damm, Assigned: Chris Lawson (gone))

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
Matthias, does Mozilla's Send Link... feature operate the same as Camino's?
(Reporter)

Comment 2

15 years ago
Greg: Yes, Mozilla works the same way.

Comment 3

15 years ago
I'll confirm as an RFE, then. If this is done, it should be done for both
Mozilla and Camino.
Severity: minor → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

15 years ago
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).

Updated

13 years ago
Priority: -- → P4
Target Milestone: --- → Camino1.1
Chris, wanna look and see how hard this is?
QA Contact: chrispetersen → os.integration
Summary: The "Send Link" menu item should send (long) URLs in "<>"s → The "Send Link" menu item should send URLs in "<>"s
Target Milestone: Camino1.1 → Camino1.2
(Assignee)

Comment 6

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

cl
Assignee: sfraser_bugs → bugzilla
(Assignee)

Comment 7

12 years ago
Created attachment 216249 [details] [diff] [review]
whitespace changes ignored for review

Patch with some minor whitespace changes forthcoming.

cl
Attachment #216249 - Flags: review?(stuart.morgan)
(Assignee)

Comment 8

12 years ago
Created attachment 216250 [details] [diff] [review]
patch with whitespace changes for checkin

Comment 9

12 years ago
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 ;)
Attachment #216249 - Flags: review?(stuart.morgan) → review-

Comment 10

12 years ago
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.
(Assignee)

Comment 11

12 years ago
Created attachment 216262 [details] [diff] [review]
fixes Stuart's comments
Attachment #216249 - Attachment is obsolete: true
Attachment #216262 - Flags: review?(stuart.morgan)
(Assignee)

Comment 12

12 years ago
Created attachment 216263 [details] [diff] [review]
fixed, with whitespace changes
Attachment #216250 - Attachment is obsolete: true

Comment 13

12 years ago
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.
Attachment #216262 - Flags: review?(stuart.morgan) → review+
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

12 years ago
I move that the checkin comments for this bug include the word 'redonkulously'.  Something like "Redonkulously simple patch by Chris Lawson"
(Assignee)

Comment 16

12 years ago
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
Attachment #216262 - Flags: superreview?(mark)

Comment 17

12 years ago
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.
Attachment #216263 - Flags: superreview+

Updated

12 years ago
Attachment #216262 - Attachment is obsolete: true
Attachment #216262 - Flags: superreview?(mark)

Comment 18

12 years ago
trunk v1.239, 1.8 branch v1.201.2.33.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

Updated

12 years ago
Keywords: fixed1.8 → fixed1.8.1

Comment 19

12 years ago
*** Bug 303074 has been marked as a duplicate of this bug. ***
Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.
Target Milestone: Camino1.2 → Camino1.1
You need to log in before you can comment on or make changes to this bug.