Closed Bug 636112 Opened 13 years ago Closed 13 years ago

Thunderbird should detect the encoding of the attachment text file.

Categories

(MailNews Core :: Composition, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 5.0b1

People

(Reporter: eagle.lu, Assigned: eagle.lu)

Details

Attachments

(2 files, 5 obsolete files)

Steps:
1. Prepare a text file encoded in UTF-8 
2. Send an e-mail with the text file as the attachment to yourself
3. Select the new e-mail in the thread pane

Expected result
  The attachment is shown correctly.

Actual result:
  The attachment is shown inline. But the encoding is incorrect.

Workaround:
  Manually select the encoding to 'UTF-8' in the menu

Since user has no choice to select the encoding format, TB should try to detect it if the attachment can be shown inline.
Attached patch detect charset if it is not set (obsolete) — Splinter Review
Attachment #514746 - Flags: review?(bugzilla)
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core
QA Contact: message-compose → composition
Hardware: x86 → All
Comment on attachment 514746 [details] [diff] [review]
detect charset if it is not set

I've not yet tested this due to the compilation and linking errors mentioned below.

I think this also needs the unit tests - you should be able to base them on the ones in http://mxr.mozilla.org/comm-central/source/mailnews/compose/test/unit/

Also just asking feedback from David to check everything seems to be the right idea here (I believe it is, just double-checking).


>+  CharsetDectionObserver(nsMsgAttachmentHandler* attch)
>+  {
>+    m_attachment = attch;
>+  };

nit: I'd prefer aAttachment for the argument, this is more consistent with elsewhere.

>+  virtual ~CharsetDectionObserver() {};
>+  NS_IMETHOD Notify(const char* aCharset, nsDetectionConfident aConf)
>+  {
>+    m_attachment->m_charset = (char*)aCharset;
>+  };

This function needs to return a value.

>+nsresult
>+nsMsgAttachmentHandler::PickCharset()
>+{
>+  nsCOMPtr<nsILocalFile> tmpFile;
>+  CharsetDectionObserver obs(this);

There's no point in creating these until they are actually used.

>+  if (m_charset || strcmp(m_type,TEXT_PLAIN))
>+    return NS_OK;;

nit: missing space after comma in the strcmp.
nit: double semi-colon.

>+  if (!detector) {
>+    // No universal charset detector, try the default charset detector
>+    const nsAdoptingString& detectorName =
>+    nsContentUtils::GetLocalizedStringPref("intl.charset.detector");

Accessing a static function in nsContentUtils is going to break any non-libxul (although that's going away soon) or shared-mailnews library based build. You should just use nsIPrefBranch direct here.

>+  if (detector)
>+  {
>+    nsCOMPtr<nsIInputStream> inputFile;
>+    nsCOMPtr<nsILineInputStream> lineInputStream;
>+    nsCAutoString buffer;
>+    PRBool isMore = PR_TRUE;
>+    PRBool dontFeed=PR_FALSE;

nit: space either side of equals please.

>+      detector->DoIt((const char*)buffer.get(),buffer.Length(),&dontFeed);

Shouldn't need the cast here - .get() already returns a const char*.

nit: spaces after the commas please.
Attachment #514746 - Flags: review?(bugzilla)
Attachment #514746 - Flags: review-
Attachment #514746 - Flags: feedback?(bienvenu)
Mark, thx a lot.  I'll polish my patch and post a new one.
A very similar one is Bug 238152
Attached patch new patch (obsolete) — Splinter Review
Assignee: nobody → brian.lu
Attachment #514746 - Attachment is obsolete: true
Attachment #514746 - Flags: feedback?(bienvenu)
Attachment #519357 - Flags: review?(bienvenu)
Comment on attachment 519357 [details] [diff] [review]
new patch

from what I can tell, the charset detector doesn't require its input to be lines; in other words, you could just pass in blocks of data (e.g., a few hundred bytes at a time). Is there a particular reason you're using a line input stream? Does the converter try to make lines internally? Other than that, this looks OK, thx for working on this.
Attachment #519357 - Flags: review?(bienvenu) → review+
The reason is that if the attached file is large, I don't want to read all bytes into memory. So I choose read it line by line until a charset is determined.
Keywords: checkin-needed
Comment on attachment 519357 [details] [diff] [review]
new patch

>+    // No universal charset detector, try the default charset detector
>+    const nsAdoptingString& detectorName =
>+    nsContentUtils::GetLocalizedStringPref("intl.charset.detector");

The comment about switching this to nsIPrefBranch hasn't been addressed (I've not checked my other comments).
Attachment #519357 - Flags: review-
Attached patch Fix nsIPrefBranch issue (obsolete) — Splinter Review
Attachment #519357 - Attachment is obsolete: true
Attachment #520608 - Flags: review?(bugzilla)
Comment on attachment 520608 [details] [diff] [review]
Fix  nsIPrefBranch issue

Have you tried taking a look at writing a test?

>+#include "nsContentUtils.h"

I think this is obsolete now.

>+  nsCOMPtr<nsILocalFile> tmpFile;
>+  tmpFile = do_QueryInterface(mTmpFile);

Lets just do this like:

nsCOMPtr<nsILocalFile> tmpFile =
  do_QueryInterface(mTmpFile);


>+  nsCOMPtr<nsICharsetDetector> detector
>+         = do_CreateInstance(NS_CHARSET_DETECTOR_CONTRACTID_BASE
>+                             "universal_charset_detector");

nit: Equals on the end of the previous line and 2-space indent the do_createInstance.

>+    nsCOMPtr<nsIPrefBranch> prefBranch
>+                             =do_GetService(NS_PREFSERVICE_CONTRACTID);
>+    nsAdoptingString detectorName;
>+    if (prefBranch)

Ditto with prefBranch, also I think we should really be doing the NS_ENSURE_SUCCESS(rv, rv); with the pref branch, as it'd be a big surprise if it started failing.
Thx Mark, i'll make a new patch this weekend, I'm learning how to write a test.
Attached patch new patch (obsolete) — Splinter Review
Attachment #520608 - Attachment is obsolete: true
Attachment #520608 - Flags: review?(bugzilla)
Attachment #523840 - Flags: review?(bugzilla)
Attached patch test case (obsolete) — Splinter Review
Attachment #523966 - Flags: review?(bugzilla)
Comment on attachment 523840 [details] [diff] [review]
new patch

Almost there ;-)

>+class CharsetDectionObserver : public nsICharsetDetectionObserver

nit: wrong spelling - should be CharsetDetectionObserver

>+    CharsetDectionObserver obs(this);
...
>+    detector->Init(&obs);
>+    NS_ADDREF(&obs);

Looking at the test output, I noticed it was leaking. You need to change this to nsRefPtr<CharsetDetectionObserver> obs = new CharsetDetectionObserver(this);

You can then safely get rid of the addref that is causing the fake-leak issue.
Attachment #523840 - Flags: review?(bugzilla) → review+
Comment on attachment 523966 [details] [diff] [review]
test case

>+var gAttachedFilePath=null;

nit: space either side of =

>+  const prefs = Cc["@mozilla.org/preferences-service;1"]
>+                  .getService(Ci.nsIPrefBranch);
>+  prefs.setIntPref("mail.strictly_mime.parm_folding", folding);
...
>+  var msgCompose = Cc["@mozilla.org/messengercompose/compose;1"]
>+                     .createInstance(Ci.nsIMsgCompose);

Please use Services.jsm and mailService.js

>+  attachment.url = "file://"+gAttachedFilePath;
>+  attachment.contentType='text/plain';

nit: spaces either side of + and = please.

>+  var attachment_file=do_get_file("data/test-UTF-8.txt");
>+  gAttachedFilePath=attachment_file.path;

nit: spaces either side of = please.
Attachment #523966 - Flags: review?(bugzilla) → review-
Attached patch test case v2Splinter Review
Attachment #523966 - Attachment is obsolete: true
Attachment #524929 - Flags: review?(bugzilla)
Attached patch Using nsCOMPtrSplinter Review
Attachment #526451 - Flags: review?(bugzilla)
Attachment #524929 - Flags: review?(bugzilla) → review+
Comment on attachment 526451 [details] [diff] [review]
Using nsCOMPtr

That's better, thanks. I'll land both these in a moment.
Attachment #526451 - Flags: review?(bugzilla) → review+
Attachment #523840 - Attachment is obsolete: true
Checked in: http://hg.mozilla.org/comm-central/rev/2b9773749c86
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a4
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: