forward a message as attachment #1728

the0ne · 2016-10-18T12:27:58Z

enables the user to forward a message as attachment of mime type "message/rfc822".
this is often required to avoid the original message headers being changed.

philipwhiuk · 2016-10-18T13:54:39Z

k9mail/src/main/java/com/fsck/k9/activity/MessageCompose.java

+        } else {
+            if (K9.DEBUG) {
+                Log.d(K9.LOG_TAG, "could not get Message-ID.");
+            }


Collapse this else { if { into else if. And print something about the message you do know in the debug line.

i didn't want to modify the code, its copied from a few lines above - see function processMessageToForward.

if we need to change it, then we should also modify it above.
what do you think?

Be bold. :)

philipwhiuk

Just some feedback based on visual review. I'll try and load it on my device for testing later.

philipwhiuk · 2016-10-18T13:55:23Z

k9mail/src/main/java/com/fsck/k9/activity/compose/AttachmentPresenter.java

+                attachments.put(attachment.uri, attachment);
+                attachmentMvpView.addAttachmentView(attachment);
+            } catch (java.io.IOException e) {
+                throw new IllegalStateException("Error storing the message to a temporary file: " + e.getMessage());


Pass the original exception instead of swallowing it.

philipwhiuk · 2016-10-18T13:55:27Z

k9mail/src/main/java/com/fsck/k9/activity/compose/AttachmentPresenter.java

+            } catch (java.io.IOException e) {
+                throw new IllegalStateException("Error storing the message to a temporary file: " + e.getMessage());
+            } catch (MessagingException e) {
+                throw new IllegalStateException("Received the same attachmentViewInfo twice!");


Pass the original exception instead of swallowing it.

at what level should the exception be caught, then? could you propose a solution, please?

I mean throw new IllegalStateException("....",e)

philipwhiuk · 2016-10-18T13:56:08Z

k9mail/src/main/java/com/fsck/k9/activity/compose/AttachmentPresenter.java

+        else {
+            try {
+                File tempFile = File.createTempFile("pre", ".tmp");
+                tempFile.deleteOnExit();


I'm fairly sure we have a better mechanism for temporary files than deleteOnExit. K-9 is unlikely to ever exit. Nor is the JVM.

Also, code style: else { on the same line

deleteOnExit is used throughout the whole K-9 code.
didn't find anything else so this must be it.

philipwhiuk · 2016-10-18T13:57:56Z

k9mail/src/main/java/com/fsck/k9/activity/MessageCompose.java

@@ -467,7 +471,7 @@ public void onTextChanged(CharSequence s, int start, int before, int count) {

        if (!mSourceMessageProcessed) {
            if (mAction == Action.REPLY || mAction == Action.REPLY_ALL ||
-                    mAction == Action.FORWARD || mAction == Action.EDIT_DRAFT) {
+                    mAction == Action.FORWARD || mAction == Action.FORWARD_AS_ATTACHMENT || mAction == Action.EDIT_DRAFT) {


Wrap this line

could you describe in detail what you mean?

The line is too long. Put mAction == Action.EDIT_DRAFT on the next line.

philipwhiuk · 2016-10-18T14:10:01Z

k9mail/src/main/java/com/fsck/k9/activity/compose/AttachmentPresenter.java

        }

        return allPartsAvailable;
    }

+    public boolean loadNonInlineAttachments(MessageViewInfo messageViewInfo) {


Code style generally prefers the public function above the private functions it uses.

will switch order

philipwhiuk · 2016-10-18T14:12:30Z

k9mail/src/main/res/values-de/strings.xml

@@ -73,6 +73,7 @@ Bitte senden Sie Fehlerberichte, Ideen für neue Funktionen und stellen Sie Frag
  <string name="compose_title_reply">Antworten</string>
  <string name="compose_title_reply_all">Allen antworten</string>
  <string name="compose_title_forward">Weiterleiten</string>
+  <string name="compose_title_forward_as_attachment">Weiterleiten als Anhang</string>


I think these will be overwritten by Transifex as an FYI

the0ne · 2016-10-18T14:50:56Z

i moved the exception handling out to processSourceMessage.
seems that's the better place.

philipwhiuk · 2016-10-19T00:09:30Z

This seems to allow viewing of forwarded messages and I have no issue with the changes. Roundcube can parse the forwarded messages we send so it is fine.

But I do have a case where I forwarded an email (which had an attachment itself) as an attachment via Outlook. And that is still displayed as an unparsed attachment. Incidentally Roundcube gets confused here too - it displays the inner attachment as an attachment and the outer forwarded email as content - I think correct behaviour is to display and offer for download the attachment.

In our case it's not possible to save the email forwarded as an attachment in K-9. I think we should be able to save the attachment - similar to how I'm doing with the iCal stuff in #930

I don't know whether that was something you intended to fix as well with this PR.

the0ne · 2016-10-19T04:08:27Z

fixing issues with viewing attachments is out of scope for this fix.
the direction i'm going is the following: https://github.com/the0ne/k-9/tree/report_as_spam

the0ne · 2016-10-19T11:37:07Z

@philipwhiuk would you please also review
https://github.com/the0ne/k-9/tree/report_as_spam
which is based upon this pull?

philipwhiuk · 2016-10-19T12:49:04Z

See #1736 for comments for that @the0ne

the0ne · 2016-10-20T20:37:18Z

@philipwhiuk please also review #1739 for the implementation of #622

Valodim

Looks good, but the 7bit reencoding thing (see below) might turn out to be quite the showstopper here :\

Valodim · 2016-10-26T22:50:50Z

k9mail/src/main/java/com/fsck/k9/activity/MessageCompose.java

+        attachmentPresenter.processMessageToForward(messageViewInfo);
+    }
+
+    private void processMessageToForwardAsAttachment(MessageViewInfo messageViewInfo) throws IOException, MessagingException {


This method is very similar to processMessageToForward, a boolean forwardAsAttachment parameter seems more suitable for the small difference in behavior

Valodim · 2016-10-26T22:51:41Z

k9mail/src/main/java/com/fsck/k9/activity/compose/AttachmentPresenter.java

@@ -162,7 +170,9 @@ public boolean loadNonInlineAttachments(MessageViewInfo messageViewInfo) {
                allPartsAvailable = false;
                continue;
            }
-            addAttachment(attachmentViewInfo);
+            if(loadNonInlineAttachments) {


Valodim · 2016-10-26T22:55:04Z

k9mail/src/main/java/com/fsck/k9/activity/compose/AttachmentPresenter.java

+            attachmentMvpView.showMissingAttachmentsPartialMessageForwardWarning();
+        } else {
+            File tempFile = File.createTempFile("pre", ".tmp");
+            tempFile.deleteOnExit();


deleteOnExit should be considered an almost-noop on android, since apps rarely if ever "exit"

This exact scheme is found throughout the existing code.
Please let me know what else to use.
But I'd prefer to stick with what you used before as it has been in use and tested for quite a while.

Huh, you're right AttachmentContentLoader does this as well. Ugh. We really shouldn't be doing this, but I'm not sure what a good way to deal with this is.. @cketti ?

We should pick one of the strategies outlined here: https://developer.android.com/reference/java/io/File.html#deleteOnExit()

I think the Unix trick is too low level. The list idea may work, but the finally strategy is neatest if we can refactor the code accordingly.

This probably wants an audit issue?

Valodim · 2016-10-26T22:55:29Z

k9mail/src/main/java/com/fsck/k9/activity/compose/AttachmentPresenter.java

+            File tempFile = File.createTempFile("pre", ".tmp");
+            tempFile.deleteOnExit();
+            FileOutputStream fileOutputStream = new FileOutputStream(tempFile);
+            messageViewInfo.message.writeTo(fileOutputStream);


this should not be done on the UI thread but rather in a loader. check out AttachmentContentLoader

hmm. i'm actually using a loader to analyze the attachment after writing.
do you think writing the binary data to the temporary folder is an issue?

At any rate, it definitely should be kept out of the UI thread

hmm.
maybe someone could help out a little?
i understand that one must use the loader for an Attachment object with a link to the MessageViewInfo so that the AttachmentContentLoader can do it's job but i'm not deep enough into K-9 code to do that myself.

Please clarify how this can be improved using an AttachmentContentLoader.
Will it be necessary to add code to SafeContentResolver as well?

Valodim · 2016-10-26T23:00:34Z

k9mail/src/main/java/com/fsck/k9/message/MessageBuilder.java

-                    "attachment;\r\n filename=\"%s\";\r\n size=%d",
-                    attachment.name, attachment.size));
+            if (!MimeUtil.isMessage(attachment.contentType)) {
+                bp.setEncoding(MimeUtility.getEncodingforType(attachment.contentType));


attaching the data as is opens up the whole rabbit hole that is 7bit eencoding. iinm, we would ideally have a magic method MimeMessage Message7BitReencoder.reencode(LocalMessage) that does this, but there are a lot of things that can go wrong with that :\

@cketti what's the plan for this?

@Valodim I actually use this functionality already and I don't see any issues forwarding text mail nor mail with binary attachments. might it be that the encoding issue doesn't apply here?
Please also note that the bp.setEncoding is not called for attachments that are rfc822 messages.
Can you provide any means of creating mail to demonstrate the issue you expect?

This is related to #1034. Although reading how qmail and exim handle this issue (source) we might want to rethink this.

We need to come up with a concept on 8bit messages sooner or later, let's continue the discussion over there

Ok, got your point.
Two workarounds would be:

take the newly created attachment name, strip out anything other than numbers and letters, replace space by underscore, limit length to a max of ~78

use a pre-defined string as the attachment name as thunderbird does, e.g. "forwarded message"

Actually, this is about 8 bit encoding of emails in general, not just the filename. Have you looked at the issue I linked above?

@Valodim not in detail - can you give a short summary?

actually the following code applies:
`

} else if (MimeUtil.isMessage(type)) { return (MimeUtil.ENC_8BIT);

`
and hence the encoding will be set to 8bit.
according to all i read there's no 7bit encoding nowadays, right?

cketti

@the0ne: This branch is a bit of a mess. Please clean it up so it only contains your changes.

the0ne · 2016-12-13T08:53:50Z

after cleanup, will you also answer the questions about 8bit and AttachmentContentLoader above?

the0ne · 2016-12-29T12:53:36Z

@cketti The branch has been cleaned up as requested - please review.

the0ne · 2017-12-29T07:58:40Z

@cketti if you want to have this feature feel free to let me know and i'll think about again fixing the conflicts that have been introduced in the mean time.

i won't again do all the work like with #1736 and #1739 only so that you will say "well, good job but now that it's ready we don't want the feature". what you did there was disrespecting developer's time and that was rude.

so once you decided please either let me know or just close this.

cketti · 2018-01-08T15:56:19Z

@the0ne: Thanks for your work. I used your code as base for PR #3063. Most notable difference is that there happens no (additional) file I/O in the main thread and that we still don't allow external apps to provide attachments with a MIME type of message/*.

the0ne mentioned this pull request Oct 18, 2016

forward as attachment #1343

Closed

philipwhiuk reviewed Oct 18, 2016

View reviewed changes

philipwhiuk suggested changes Oct 18, 2016

View reviewed changes

philipwhiuk approved these changes Oct 19, 2016

View reviewed changes

Valodim reviewed Oct 26, 2016

View reviewed changes

cketti added the status: needs work Issues that are pending further action or development label Dec 12, 2016

cketti requested changes Dec 12, 2016

View reviewed changes

the0ne force-pushed the forward_as_attachment branch from 1db14c3 to 900898c Compare December 13, 2016 16:31

Forward as Attachment

900898c

the0ne mentioned this pull request Dec 13, 2016

Report As Spam functionality #1736

Closed

cketti mentioned this pull request Jan 1, 2017

Redirect mail (Resend/redirect/bounce) rfc5322 #1739

Closed

the0ne added 2 commits February 2, 2017 12:02

Merge branch 'master' into forward_as_attachment

75fafde

re-added action FORWARD_AS_ATTACHMENT

ad40450

cketti self-assigned this Feb 20, 2017

cketti mentioned this pull request Nov 12, 2017

Get rid of 8bit transport encoding #1034

Closed

cketti mentioned this pull request Jan 8, 2018

Forward message as attachment #3063

Merged

cketti closed this Jan 8, 2018

forward a message as attachment #1728

forward a message as attachment #1728

Conversation

the0ne commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipwhiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the0ne commented Oct 18, 2016

philipwhiuk commented Oct 19, 2016 • edited Loading

the0ne commented Oct 19, 2016

the0ne commented Oct 19, 2016

philipwhiuk commented Oct 19, 2016

the0ne commented Oct 20, 2016

Valodim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the0ne Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the0ne Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the0ne Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cketti left a comment

Choose a reason for hiding this comment

the0ne commented Dec 13, 2016 • edited Loading

the0ne commented Dec 29, 2016

the0ne commented Dec 29, 2017

cketti commented Jan 8, 2018

philipwhiuk commented Oct 19, 2016 •

edited

Loading

the0ne Oct 27, 2016 •

edited

Loading

the0ne Nov 4, 2016 •

edited

Loading

the0ne Nov 3, 2016 •

edited

Loading

the0ne commented Dec 13, 2016 •

edited

Loading