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

UTF8TextConverter broken when used with leadingChars #8175

Closed
jbrichau opened this issue Dec 25, 2020 · 9 comments
Closed

UTF8TextConverter broken when used with leadingChars #8175

jbrichau opened this issue Dec 25, 2020 · 9 comments

Comments

@jbrichau
Copy link

jbrichau commented Dec 25, 2020

Describe the bug
The changes for the deprecation of UTF8TextConverter (202cd0d) have broken the implementation of the converter itself, specifically in the case of strings in other language environments.

The change of Character>>asUnicode in the aforementioned commit disregards the presence of a leadingChar but the same code is still used by the UTF8TextConverter. A asUnicodeForTextConverter method with the implementation that takes the leadingChar into account should keep the TextConverter working.

To Reproduce

	| leading hiraA hiraO hiraAO converter |
	leading := (Smalltalk classNamed: #JapaneseEnvironment) leadingChar.
	hiraA := (Character leadingChar: leading code: 12354) asString. "HIRAGANA LETTER A"
	hiraO := (Character leadingChar: leading code: 12362) asString. "HIRAGANA LETTER O"
	hiraAO := hiraA , hiraO.
	
	converter := [ :arg | String
		streamContents: [ :stream | 
			(UTF8TextConverter new) nextPutAll: arg toStream: stream. ] ].
	
	self assert: (converter value: hiraA) asByteArray equals: #[227 129 130].
	self assert: (converter value: hiraO) asByteArray equals: #[227 129 138].
	self assert: (converter value: hiraAO) asByteArray equals: #[227 129 130 227 129 138]

Expected behavior
Although deprecated, I expect the UTF8TextConverter continue to work as before so we can migrate away from it.

Version information:
Pharo 9.0.0
Build information: Pharo-9.0.0+build.986.sha.759c31d569014695a50f4fc42809ada50b92ea54 (64 Bit)

Expected development cost
I will make a PR

Additional context
Broken test in Grease: SeasideSt/Grease#114

@welcome
Copy link

welcome bot commented Dec 25, 2020

Thanks for opening your first issue! Please check the CONTRIBUTING documents for some tips about which information should be provided. You can find information of how to do a Pull Request here: https://github.com/pharo-project/pharo/wiki/Contribute-a-fix-to-Pharo

GitHub
Pharo is a dynamic reflective pure object-oriented language supporting live programming inspired by Smalltalk. - pharo-project/pharo

@Ducasse
Copy link
Member

Ducasse commented Dec 25, 2020

Thanks Johan.

@Ducasse
Copy link
Member

Ducasse commented Dec 25, 2020

leadingChar is preUnicode and we want to clean our system.

@jbrichau
Copy link
Author

@Ducasse I don't want to revert the deprecation, I just prefer if the deprecated code keeps working while deprecated. Because of the change, I will be cleaning Grease/Seaside in that area as well such that we use the ZnUTF8Encoder in Pharo instead of the encoder built on top of TextConverter, but after some work I noticed I will need some more time to complete it, and therefore, I prefer that the deprecated code keeps working.

But I will try to submit a PR, indeed.

@Ducasse
Copy link
Member

Ducasse commented Dec 25, 2020

Yes I understand and we should have paid attention. But sometimes we feel like in an old dark house where you cannot see the wall before empty all the furnitures.

@jbrichau
Copy link
Author

No worries. I completely understand that. Thanks!

@MarcusDenker
Copy link
Member

revert is here but not yet merged: #8191

@guillep
Copy link
Member

guillep commented Jan 11, 2021

I'm checking it

@guillep
Copy link
Member

guillep commented Jan 29, 2021

This is done

@guillep guillep closed this as completed Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants