Last Comment Bug 748799 - Extra link inserted into Filelink emails on receiver's end
: Extra link inserted into Filelink emails on receiver's end
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 15.0
Assigned To: David :Bienvenu
:
:
Mentors:
Depends on:
Blocks: BigFiles
  Show dependency treegraph
 
Reported: 2012-04-25 08:43 PDT by Mike Conley (:mconley)
Modified: 2012-04-30 10:02 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
Screenshot showing the inserted URL (23.88 KB, image/png)
2012-04-25 08:43 PDT, Mike Conley (:mconley)
no flags Details
proposed fix (2.84 KB, patch)
2012-04-26 14:11 PDT, David :Bienvenu
mconley: review-
Details | Diff | Splinter Review
don't snarf attachment... (2.87 KB, patch)
2012-04-27 16:26 PDT, David :Bienvenu
mconley: review+
mozilla: approval‑comm‑aurora+
mozilla: approval‑comm‑beta+
Details | Diff | Splinter Review
make filelink attachments interet-shortcut (3.05 KB, patch)
2012-04-28 12:24 PDT, Magnus Melin
mozilla: review-
Details | Diff | Splinter Review

Description Mike Conley (:mconley) 2012-04-25 08:43:38 PDT
Created attachment 618287 [details]
Screenshot showing the inserted URL

When sending a Filelink email, receivers still see a URL attachment, distinct from the Filelink URL in the body of the message.

See screenshot.

David:  is this on purpose?
Comment 1 David :Bienvenu 2012-04-25 09:00:41 PDT
yes, it's on purpose. We need a mimepart to store all the stuff that lets us edit drafts correctly in the mime headers. I decided to put the url in the body of the mime part because I thought it might be more useful than an empty mime part.
Comment 2 Mike Conley (:mconley) 2012-04-25 10:15:21 PDT
David:

Ok, so we'll mark this one WONTFIX?

-Mike
Comment 3 David :Bienvenu 2012-04-25 11:57:49 PDT
(In reply to Mike Conley (:mconley) from comment #2)
> David:
> 
> Ok, so we'll mark this one WONTFIX?
> 
> -Mike

Well, I'm open to suggestions - we could simply not put the link in and have an empty part, but I thought it might come in handy, perhaps for extensions.
Comment 4 Jb Piacentino 2012-04-26 01:37:27 PDT
(In reply to David :Bienvenu from comment #3)
> (In reply to Mike Conley (:mconley) from comment #2)
> > David:
> > 
> > Ok, so we'll mark this one WONTFIX?
> > 
> > -Mike
> 
> Well, I'm open to suggestions - we could simply not put the link in and have
> an empty part, but I thought it might come in handy, perhaps for extensions.

I think having this extra duplicated link is confusing. I understand the reason we need this mime but I'd suggest we leave it empty.
I'm curious of what you have in mind for extensions. This sounds interesting as well so we need to balance user experience and developer provisions...
Comment 5 Blake Winton (:bwinton) (:☕️) 2012-04-26 08:42:54 PDT
Could we leave the link in, but make it a type of mime part we don't show by default?

Could we remove it before sending?
Comment 6 David :Bienvenu 2012-04-26 09:56:32 PDT
(In reply to Jb Piacentino from comment #4)
> I think having this extra duplicated link is confusing. I understand the
> reason we need this mime but I'd suggest we leave it empty.
> I'm curious of what you have in mind for extensions. This sounds interesting
> as well so we need to balance user experience and developer provisions...

things like prefetching the cloud files, indexing them in gloda, etc. But we could do that by putting the link information in a mime header instead of in the body.

Re using some type we don't display, I'm open to suggestions. We'd need something that won't cause interoperability problems with other clients. We could use application/octet-stream, as long as not having a file in the content-type doesn't confuse other apps.
Comment 7 Jb Piacentino 2012-04-26 10:03:36 PDT
(In reply to David :Bienvenu from comment #6)
> (In reply to Jb Piacentino from comment #4)
> > I think having this extra duplicated link is confusing. I understand the
> > reason we need this mime but I'd suggest we leave it empty.
> > I'm curious of what you have in mind for extensions. This sounds interesting
> > as well so we need to balance user experience and developer provisions...
> 
> things like prefetching the cloud files, indexing them in gloda, etc. But we
> could do that by putting the link information in a mime header instead of in
> the body.
If this is a way of hidding the information while maintaining useful information for extensions, that seems a good solution.
> 
> Re using some type we don't display, I'm open to suggestions. We'd need
> something that won't cause interoperability problems with other clients. We
> could use application/octet-stream, as long as not having a file in the
> content-type doesn't confuse other apps.
Comment 8 David :Bienvenu 2012-04-26 14:11:58 PDT
Created attachment 618799 [details] [diff] [review]
proposed fix
Comment 9 Magnus Melin 2012-04-27 04:04:12 PDT
Could we just make the attachment an internet shortcut? (.url file)
Mime type could perhaps be application/internet-shortcut? Would be pretty nice to just be able to drag'n'drop cloud attachments to the desktop and be able to open directly.
Comment 10 Mike Conley (:mconley) 2012-04-27 08:16:02 PDT
Comment on attachment 618799 [details] [diff] [review]
proposed fix

Review of attachment 618799 [details] [diff] [review]:
-----------------------------------------------------------------

Hm - so this seems to introduce a bug.  STR:

1)  Open a compose window, and attach a large Filelink - > 1MB, to fully experience the effects.  (I noticed it with a 10MB file)
2)  Send the message.

What happens?

When sending, the progress bar chugs along, as if the attachment is being sent along with the message.

What's expected?

Sending should be instantaneous.

I should point out that this does fix the other bug - we no longer insert the extra link into the body of the message.
Comment 11 Magnus Melin 2012-04-27 12:14:46 PDT
(In reply to Magnus Melin from comment #9)
> Could we just make the attachment an internet shortcut? (.url file)
> Mime type could perhaps be application/internet-shortcut? Would be pretty
> nice to just be able to drag'n'drop cloud attachments to the desktop and be
> able to open directly.
 
We would of course then name the attachment, like <realfilename>.url.
Firefox creates .url files like so: http://mxr.mozilla.org/comm-central/source/mozilla/widget/windows/nsDataObj.cpp#1156 though i think even a file containing just the url works in practice.
Comment 12 David :Bienvenu 2012-04-27 12:57:58 PDT
(In reply to Mike Conley (:mconley) from comment #10)

> When sending, the progress bar chugs along, as if the attachment is being
> sent along with the message.
ugh, probably the content type change. I'll try to look at this after tb 12.01.

Re internet shortcut, the urls that we get from file link providers usually point at a web page, not the actual file.
Comment 13 David :Bienvenu 2012-04-27 15:52:37 PDT
oh yeah, I remember now - nsMsgSendPart::Write() will write out what's in m_buffer, if it's set; otherwise, it writes out the file in m_file. When I removed the link code, that left m_buffer null... working on a fix.
Comment 14 David :Bienvenu 2012-04-27 16:26:45 PDT
Created attachment 619207 [details] [diff] [review]
don't snarf attachment...
Comment 15 Magnus Melin 2012-04-28 12:24:27 PDT
Created attachment 619331 [details] [diff] [review]
make filelink attachments interet-shortcut

Yes, it may not be a direct link. However, my making it a shortcut you can get to it with a click or two. 

This also have the advantages like
 - attachment icon will be shown in thread pane
 - save all for multiple attachments would do more of what the user expects
 - sending a mail with cloud attachments AND normal attachments is confusing to the receiver as is (also somewhat with several cloud attachments)
Comment 16 David :Bienvenu 2012-04-28 13:27:38 PDT
(In reply to Magnus Melin from comment #15)
> Created attachment 619331 [details] [diff] [review]
> make filelink attachments interet-shortcut
> 
> Yes, it may not be a direct link. However, my making it a shortcut you can
> get to it with a click or two. 
> 
> This also have the advantages like
>  - attachment icon will be shown in thread pane
>  - save all for multiple attachments would do more of what the user expects
>  - sending a mail with cloud attachments AND normal attachments is confusing
> to the receiver as is (also somewhat with several cloud attachments)

I believe JB and Blake don't want to see the attachment icon, or attachments in the attachment pane of the message viewer, so they would see all these as disadvantages. But I might be confused, and they should speak for themselves.
Comment 17 Blake Winton (:bwinton) (:☕️) 2012-04-29 19:34:13 PDT
Ideally, I don't want to see the attachment icon or attachments in the attachment pane.  If we absolutely have to have something there, making it an "internet shortcut" seems like the least bad option.
Comment 18 David :Bienvenu 2012-04-29 21:34:21 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #17)
> Ideally, I don't want to see the attachment icon or attachments in the
> attachment pane. 

That's what my patch that mconley is going to review accomplishes.
Comment 19 Magnus Melin 2012-04-30 05:17:59 PDT
I have to say i'm surprised you don't want an attachment icon for mails that contain attachments (though the actual file is elsewhere). 
More often than not i'm searching old mails that i know had some document attached, and finding which mail that would be in a conversation is a lot easier if there's an icon.
Comment 20 Mike Conley (:mconley) 2012-04-30 07:02:15 PDT
Comment on attachment 619207 [details] [diff] [review]
don't snarf attachment...

Review of attachment 619207 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, and works as advertised.

Thanks David.
Comment 21 David :Bienvenu 2012-04-30 07:19:58 PDT
fixed on trunk - http://hg.mozilla.org/comm-central/rev/c2fc01604fdc
Comment 22 Mike Conley (:mconley) 2012-04-30 07:20:55 PDT
Is this something we wanted for TB 13/14?
Comment 23 David :Bienvenu 2012-04-30 07:22:09 PDT
Comment on attachment 619331 [details] [diff] [review]
make filelink attachments interet-shortcut

I don't disagree with you, Magnus, but I guess I can see the argument that until we treat these more like attachments, we shouldn't show them as attachments.
Comment 24 David :Bienvenu 2012-04-30 07:22:45 PDT
Comment on attachment 619207 [details] [diff] [review]
don't snarf attachment...

[Triage Comment]
we want this for tb 13 and 14, I believe.

Note You need to log in before you can comment on or make changes to this bug.