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)
MailNews Core
Composition
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 5.0b1
People
(Reporter: eagle.lu, Assigned: eagle.lu)
Details
Attachments
(2 files, 5 obsolete files)
4.43 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
4.38 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #514746 -
Flags: review?(bugzilla)
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core
QA Contact: message-compose → composition
Hardware: x86 → All
Comment 2•13 years ago
|
||
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.
Comment 4•13 years ago
|
||
A very similar one is Bug 238152
Assignee: nobody → brian.lu
Attachment #514746 -
Attachment is obsolete: true
Attachment #514746 -
Flags: feedback?(bienvenu)
Attachment #519357 -
Flags: review?(bienvenu)
Comment 6•13 years ago
|
||
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 8•13 years ago
|
||
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-
Updated•13 years ago
|
Keywords: checkin-needed
Attachment #519357 -
Attachment is obsolete: true
Attachment #520608 -
Flags: review?(bugzilla)
Comment 10•13 years ago
|
||
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.
Assignee | ||
Comment 11•13 years ago
|
||
Thx Mark, i'll make a new patch this weekend, I'm learning how to write a test.
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #520608 -
Attachment is obsolete: true
Attachment #520608 -
Flags: review?(bugzilla)
Attachment #523840 -
Flags: review?(bugzilla)
Assignee | ||
Comment 13•13 years ago
|
||
Attachment #523966 -
Flags: review?(bugzilla)
Comment 14•13 years ago
|
||
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 15•13 years ago
|
||
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-
Assignee | ||
Comment 16•13 years ago
|
||
Attachment #523966 -
Attachment is obsolete: true
Attachment #524929 -
Flags: review?(bugzilla)
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #526451 -
Flags: review?(bugzilla)
Updated•13 years ago
|
Attachment #524929 -
Flags: review?(bugzilla) → review+
Comment 18•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #523840 -
Attachment is obsolete: true
Comment 19•13 years ago
|
||
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
Updated•13 years ago
|
status-seamonkey2.1:
--- → ?
Comment 20•13 years ago
|
||
http://hg.mozilla.org/releases/comm-2.0/rev/47c797c65912
status-seamonkey2.1:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•