Closed Bug 1509709 Opened 4 years ago Closed 4 years ago

Control showing text attachments inline in Thunderbird message display: Implement pref mail.inline_attachments.text = false

Categories

(MailNews Core :: MIME, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 67.0

People

(Reporter: aceman, Assigned: aceman)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

If the user sets 'display attachments' inline, TB shows images and also text/* (.g. plain text, HTML, CSV) attachments inline. For images it makes sense. For text attachments it is questionable. E.g. for long text files (like logs) or CSV data dumps sent within enterprises, showing them inline only inflates the displayed message height, but the user does not want to view them immediately.

We have some possibilities here:
1. do not display attachments with text/* mimetype inline ever
2. only display text/* inline if below some file size (say 10KB).
3. have a pref whether the user wants to display those
Only 3 is probably doable
See also Bug 564051.
Especially Bug 564051 comment 20
Thanks, yes it seems to be the same problem. IF the text attachment is very large it may slow down TB for something the user will surely not read inline.
Blocks: 564051
Attached patch 1509709.patch (obsolete) — Splinter Review

This works for me.

Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #9039434 - Flags: ui-review?(richard.marti)
Attachment #9039434 - Flags: review?(mkmelin+mozilla)

OK, test mailnews/mime/test/unit/test_message_attachment.js needs update for this, but we can test behaviour with both values of the pref now.

Comment on attachment 9039434 [details] [diff] [review]
1509709.patch

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

::: mailnews/mime/src/mimei.cpp
@@ +828,5 @@
>     But Content-Disposition is ignored for all containers except `message'.
>     (including multipart/mixed, and multipart/digest.)  It's not clear if
>     this is to spec, but from a usability standpoint, I think it's necessary.
>     */
> +printf("ACE: 4: show=%i, quote=%i, force=%i\n", opts->show_attachment_inline_p, opts->quote_attachment_inline_p, forceInline);

Please remove debug or is this left for review purposes?
Attached patch 1509709.patch v1.1 (obsolete) — Splinter Review

Sorry, leftover debugging. Now with fixed and improved tests.

So with this patch and having the "show attachments" inline option set, attachmed images will still be displayed inline in the message body, but text attachments (e.g. .txt or .csv) will not be. I have intentionally kept HTML attachments displaying as those probably aren't used as data exchange with megabyte-sized attachment.

Attachment #9039434 - Attachment is obsolete: true
Attachment #9039434 - Flags: ui-review?(richard.marti)
Attachment #9039434 - Flags: review?(mkmelin+mozilla)
Attachment #9039542 - Flags: ui-review?(richard.marti)
Attachment #9039542 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9039542 [details] [diff] [review]
1509709.patch v1.1

Maybe this could be extended to attached messages in a new bug. They could also be very long and it can irritate when the message text and the attached message text are shown together.
Attachment #9039542 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 9039542 [details] [diff] [review]
1509709.patch v1.1

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

Why not make html attachments attachments too? They mess up the display of the main message pretty badly (which is another bug).
Attached patch 1509709.patch v1.2 (obsolete) — Splinter Review

Sure, we can do that until HTML pages are displayed properly (e.g. confined in some iframe to not affect the msg body).

Attachment #9039542 - Attachment is obsolete: true
Attachment #9039542 - Flags: review?(mkmelin+mozilla)
Attachment #9040996 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9040996 [details] [diff] [review]
1509709.patch v1.2

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

Yep, LGTM! r=mkmelin

::: mailnews/mailnews.js
@@ +12,5 @@
>  pref("mailnews.logComposePerformance", false);
>  
>  pref("mail.wrap_long_lines",                true);
>  pref("mail.inline_attachments",             true);
> +pref("mail.inline_attachments.text",        false);

Please add inline documentation about what it does
Attachment #9040996 - Flags: review?(mkmelin+mozilla) → review+

Yes, thanks.

Attachment #9040996 - Attachment is obsolete: true
Attachment #9042731 - Flags: review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/43c9c600cf84
add a pref to disallow showing text attachments inline. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0

FYI, I mention this bug here: Bug 184869 comment 16. I don't see that the fix for this bug is causing a problem. With or without this fix, the two text sub-parts of Bug 184869 don't display.

See Also: → 1628441
Regressions: 1628441
Summary: limit showing text attachments inline in Thunderbird message display → Control showing text attachments inline in Thunderbird message display: Implement pref mail.inline_attachments.text = false
See Also: 1628441
You need to log in before you can comment on or make changes to this bug.