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

[BasicUI] Fix character encoding of returned page #2746

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

lolodomo
Copy link
Contributor

@lolodomo lolodomo commented Sep 7, 2024

Fix #2681

Concerns only the settings page and the page listing all sitemaps

Signed-off-by: Laurent Garnier lg.hc@free.fr

Fix openhab#2681

Concerns only the settings page and the page listing all sitemaps

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo lolodomo added bug Something isn't working basic ui Basic UI labels Sep 7, 2024
@lolodomo lolodomo requested a review from a team as a code owner September 7, 2024 14:57
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see it fixed. 👍
Maybe also update the ManifestServlet similarly?

@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 7, 2024

Maybe also update the ManifestServlet similarly?

Oh yes

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 7, 2024

Done

Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Might also be an idea to check other add-ons for similar issues.

@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 7, 2024

For the full explanation, res.setContentType with text/html;charset=UTF-8 as parameter is also setting the character encoding which is then considered when calling res.getWriter().
An alternative is to call res.setCharacterEncoding.
Whenres.setContentType was not first called, res.getWriter() was considering the default charset "iso-8859-1".

@dilyanpalauzov
Copy link

dilyanpalauzov commented Sep 7, 2024

Where does charset=ISO-8959-1/Latin-1 come from? Can that place be enforced to send UTF-8 instead? Then there will be no need to set explicitly UTF-8 on many places.

@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 7, 2024

After the merge, I will cherry-pick this fix in 4.2.x.

@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 7, 2024

Where does charset=ISO-8959-1/Latin-1 come from? Can that place be enforced to send UTF-8 instead? Then there will be no need to set explicitly UTF-8 on many places.

I just explained it in my previous message.
It is the behaviour defined by javax.servlet.ServletResponse

@dilyanpalauzov
Copy link

After the merge, I will cherry-pick this fix in 4.2.x.

I do not know where is discussed what to backport, but I propose also openhab/openhab-core#4312 for including in 4.2.x.

@florian-h05
Copy link
Contributor

I do not know where is discussed what to backport, but I propose also openhab/openhab-core#4312 for including in 4.2.x.

This is usually discussed inside the PR, and in case of your linked PR it seems it is already backported.

@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 7, 2024

but I propose also openhab/openhab-core#4312 for including in 4.2.x.

It was back-ported and should be available in 4.2.1.

@dilyanpalauzov
Copy link

but I propose also openhab/openhab-core#4312 for including in 4.2.x.

It was back-ported and should be available in 4.2.1.

Ah, it is in https://github.com/kaikreuzer/openhab-core/commits/4.2.x/ . I have not found it at https://github.com/openhab/openhab-core/commits/4.2.x/ .

@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 7, 2024

You just have it included in the release notes of 4.2.1:
https://github.com/openhab/openhab-distro/releases/tag/4.2.1

@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 7, 2024

Might also be an idea to check other add-ons for similar issues.

I have done a quick check in addons repository (30 files) and most of the time a character encoding is set before calling response.getWriter(). It can happen it is not set before but maybe the developer knows in that case that iso-8859-1 is an appropriate default character encoding ? I see it not set especially in the cases of XML or JSON responses.

@wborn
Copy link
Member

wborn commented Sep 7, 2024

Ah, it is in https://github.com/kaikreuzer/openhab-core/commits/4.2.x/ . I have not found it at https://github.com/openhab/openhab-core/commits/4.2.x/ .

Thanks for noticing!
Looks like several others were missing too so I've cherry-picked those as well and removed them from the release notes.

@lolodomo lolodomo changed the title [BasicUI] Fix setting content type of returned page [BasicUI] Fix character encoding of returned page Sep 7, 2024
@lolodomo
Copy link
Contributor Author

@kaikreuzer : any chance that you review this small PR fixing an annoying bug ?

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing - thanks!

@kaikreuzer kaikreuzer merged commit 727b876 into openhab:main Sep 17, 2024
5 checks passed
@kaikreuzer kaikreuzer added this to the 4.3 milestone Sep 17, 2024
@lolodomo lolodomo deleted the basicui_contenttype branch September 17, 2024 19:49
@lsiepel
Copy link

lsiepel commented Sep 18, 2024

After the merge, I will cherry-pick this fix in 4.2.x.

^^

lolodomo added a commit that referenced this pull request Sep 21, 2024
Fix #2681

Concerns only the settings page and the page listing all sitemaps

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo lolodomo added the patch A PR that has been cherry-picked to a patch release branch label Sep 21, 2024
@lolodomo
Copy link
Contributor Author

Backported to branch 4.2.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
basic ui Basic UI bug Something isn't working patch A PR that has been cherry-picked to a patch release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BasicUI] Wrong encoding of translated strings
6 participants