Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hey,TextResult has a big trouble! #25

Closed
zqq90 opened this issue May 11, 2013 · 11 comments
Closed

Hey,TextResult has a big trouble! #25

zqq90 opened this issue May 11, 2013 · 11 comments
Assignees
Labels

Comments

@zqq90
Copy link
Member

zqq90 commented May 11, 2013

Jodd 3.4.3

My code is like:
return "text:this text contents chinese chars 中文 ";
and i got
this text contents chinese chars ??

i have set all environments (os, jvm,netbeans,tomcat,maven) 's characterEncoding toUTF-8

it may caused by jodd.madvoc.MadvocResponseWrapper.setContentType(), this method will erase responseWrapper.characterEncoding.

waitting for reply ;)

@ghost ghost assigned igr May 11, 2013
@zqq90
Copy link
Member Author

zqq90 commented May 12, 2013

Test Code:

response.setContentType(MimeTypes.MIME_TEXT_PLAIN);
//Haha, characterEncoding is back
response.setCharacterEncoding(madvocConfig.getEncoding());

it works! 😄

@igr
Copy link
Member

igr commented May 12, 2013

Perfect! I always hate when setContentType change encoding :)

I will try to build a testcase on this, so we can have test infrastructure for similar cases for Madvoc.

Thank you! I will soon release a beta snapshot!

@igr igr closed this as completed in a1e1d30 May 12, 2013
@igr
Copy link
Member

igr commented May 12, 2013

Thanx again, i solved it in the same way and also by using OutputStream as I wanted to set the content length as well, in case when server does not provide chunked transfer.

@zqq90
Copy link
Member Author

zqq90 commented May 13, 2013

but what about other Result(s)?
how about
MadvocResponseWrapper.setContentType(..)

if (contentTypeResolver.getEncoding() != null){
    characterEncoding = contentTypeResolver.getEncoding(); 
}

@igr
Copy link
Member

igr commented May 13, 2013

In short, the approach is the following: 1) Madvoc sets the content type and encoding first 2) if the Result wants to modify it, than its his responsibility to do everything right.

I know that MadvocResponseWrapper seems like a single place for fixing this; but that would always append the character encoding to content type; and the purpose of the wrapper is to allow to reset it:) Let me explain. In cases when you return raw types, like for images created dynamically, returned content type would always have character encoding set; and it would look like:

content-type:image/jpeg;charset:UTF-8`

Some browsers (like IE) do not like this, and, being strict, that would not be correct. So we introduced MadvocResponseWrapper to allow raw images to be able to reset charset and allow sending of just:

content-type:image/jpeg

See: https://github.com/oblac/jodd/blob/master/jodd-madvoc/src/main/java/jodd/madvoc/result/RawResult.java#L47

Since no other result is changing the content type, I believe we are fine here. Make sense :)?

@zqq90
Copy link
Member Author

zqq90 commented May 13, 2013

Well, my JsonResult TemplateResult (instead of jsp) even CachingPageInterceptor have the same problem , I may fix them.
btw, do you think about a CachingPageInterceptor for Jodd Madvoc?

@igr
Copy link
Member

igr commented May 13, 2013

Yes, you are probably using Writer to output the content; while thats correct, try using OutputStream in your results as it would force to use the encoding...

This might be annoying, I agree, but I am not sure how to make a difference when character encoding is missing (i.e. being null) on purpose - like in RawResult for binary types. Let me think a bit more about this :)

Sure, I am thinking about caching, for now the only thing I am not sure is how to implement the cache validity check in most pragmatic way? You can send me your idea, no problem with that :)

@zqq90
Copy link
Member Author

zqq90 commented May 13, 2013

it's a piece of cake for me figure out where should reset characterEncoding, however , we should line it out in doc for other guys.

[how about:]
MadvocResponseWrapper override .getCharacterEncoding()
so we can do it like is

String encoding = response.getCharacterEncoding();
response.setContentType(MimeTypes.MIME_TEXT_PLAIN);
response.setCharacterEncoding(encoding);

no

@In(scope = ScopeType.CONTEXT)
MadvocConfig madvocConfig;

@igr
Copy link
Member

igr commented May 13, 2013

Nice 👍 Please let me check first (write a test) for a RawResult and character encodings, to see if this will cover the other use case that actually made me creating MadvocResponseWrapper.

@zqq90
Copy link
Member Author

zqq90 commented May 13, 2013

See: https://github.com/oblac/jodd/blob/master/jodd-madvoc/src/main/java/jodd/madvoc/component/MadvocController.java#L105

and #L106

a litte mistake , use encoding instead of madvocConfig.getEncoding()

@igr
Copy link
Member

igr commented May 13, 2013

Haaa, thank you:exclamation: I've just fixed and push the changes - I hate such mistakes, please let me know if you find anything similar :smile:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants