Conversation
ibrahim-iqbal
left a comment
ibrahim-iqbal
left a comment
There was a problem hiding this comment.
Solid rewrite. detectHtmlCharset runs through a proper priority order (HTTP header → BOM → HTML <meta> → UTF-8 default), which is exactly what feeds and full-content pages need, and the change from source().use { peek { … } } to a straight bytes() + String(bytes, charset) gets rid of the double-decode footgun that was in the old path.
A few small things worth thinking about:
- Memory footprint. Switching from
source()streaming toresponseBody.bytes()loads the entire response into memory. For most feeds and article pages this is fine, but ReadYou's full-content extractor can hit pages in the multi-MB range on image-heavy sites. Not something to change here, but worth keeping an eye on if you ever see OOMs on low-memory devices. - Malformed input.
String(bytes, charset)throws on illegal byte sequences for some charsets (e.g.ISO-2022-JPwith a broken escape). Wrapping the final decode withCharsetDecoder.onMalformedInput(REPLACE).decode(...)would swap those failures for a?glyph instead of a crash. Optional. it.replace(',', ';')intoHttpContentTypeis a nice fix for servers that misspell the header separator. Might be worth a one-line comment saying so, since a future reader is going to wonder why we're rewriting commas in a Content-Type string.- Fixture tests. The added
RssHelperTestcases cover the header + BOM + meta paths — nice. If you feel like adding one more, a test withContent-Type: text/html, charset=windows-1252(comma variant) would lock in thereplace(',', ';')behaviour so a future refactor doesn't quietly regress it.
Overall LGTM on the charset detection side; happy to defer on the memory trade-off since it clearly favours correctness for the current use case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & Why
Fixes #1094
When fetching and parsing full article content (e.g. from
developpez.comor other sites declaring ISO-8859-1 / Windows-1252):é,è,à,ç,ê,î,ô,ù, etc.) were displayed as corrupted replacement symbols (`` or?)."Perplexity a lanc son nouvel abonnement Max, factur 200 dollars..."Root Cause
Content-TypeHeader with Comma: Some legacy/PHP servers returnContent-Type: text/html, charset=iso-8859-1with a comma rather than a semicolon. OkHttp'sMediaType.parse()fails on this format and returnsnullforresponseBody.contentType()?.charset().<meta charset="...">Detection: Onlymeta[http-equiv=content-type]was inspected, completely ignoring HTML5<meta charset="iso-8859-1">.it.peek().readString(Charsets.UTF_8)converted raw ISO-8859-1 bytes (such as0xE9foré) directly into\uFFFD, causing irreversible character loss if meta charset wasn't found in time.Fixes Applied
detectHtmlCharset) inRssHelper.kt:Content-Typeheader (handling both,and;delimiters as well as quotes).<head>for<meta charset="...">(HTML5) and<meta http-equiv="content-type" content="...">.UTF-8.parseFullContent(): Reads the raw response bytes and decodes them directly with the detected charset, preventing lossy intermediate UTF-8 conversions.toHttpContentType(): Normalizes commas to semicolons when reading feed XML content types.RssHelperTest.kt): Added comprehensive tests verifying charset detection from comma-separated headers, HTML5 meta tags, and accurate decoding of French accented text.Tested
./gradlew testGithubReleaseUnitTest->BUILD SUCCESSFUL../gradlew assembleGithubRelease->BUILD SUCCESSFUL.