Add new variants of spawnPipe functions with encoding support #334
Conversation
This file contains 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
The current handle returned by `spawnPipe` doesn't have any encoding and it uses the function fdToHandle which returns a Binary Handle. When spawnPipe is used with a program like xmobar, this can easily lead to errors: λ> h <- spawnPipe "xmobar" λ> hPutStrLn h "\35753Haskell\25110\32773Ghci\33021\27491\30830\26174\31034\27721\23383\24182\19988\35835\21462\27721\23383\21629\21517\30340\25991\26723" <stdin>: hGetLine: invalid argument (invalid byte sequence) One workaround, to avoid this is to use `hSetEncoding`. But from reading GHC's source code - the entire Handle and write operations seems stateful. So doing something like hPutStr and hSetEncoding can theoretically lead to an undefined state as the first write will use latin encoding and the second write will use the new encoding. More details about it are present here: * http://hackage.haskell.org/package/base-4.12.0.0/docs/src/GHC.IO.Handle.Internals.html#writeCharBuffer * http://hackage.haskell.org/package/base-4.12.0.0/docs/src/GHC.IO.Buffer.html#CharBuffer So having these new functions will ensure that we get the handles in the proper encoding in the first place.
|
I'm planning to merge this MR in a week if nobody has any objections to it. |
|
No objections here. |
liskin
added a commit
to liskin/xmonad-contrib
that referenced
this pull request
Feb 16, 2021
…pLog' For many (10+) years, we had a cascade of ugly workarounds: * X.U.Run.spawnPipe returned a binary handle, so the String passed to it must have been encoded by C.B.UTF8.String.encodeString * X.H.DynamicLog.dynamicLogString therefore returned such an encoded String, so one could use it directly in a hPutStrLn, but literal Strings wouldn't work * xmonadPropLog' also expected an encoded String to make it easier to use together with dynamicLogString, again breaking usage with String literals and other normal unicode Strings Then in 1d0eadd Sibi fixed spawnPipe to return a handle usable with normal Strings, which then obviously broke the entire cascade. But, instead of using the opportunity to remove all the ugly workarounds, he decided to add some more on top, so now spawnPipe with dynamicLogString outputs doubly encoded UTF-8 and xmobar has a hack to strip this double encoding (https://github.com/jaor/xmobar/pull/482), which only works when XFT is in use and breaks on some long unicode codepoints. :-( There is a better way: make everything just use normal Strings and only encode when it goes out the wire. This means dynamicLogString can be freely mixed with String literals, manual uses of xmonadPropLog' don't need encodeString, and everything just works nicely. This obviously breaks configs that used some of these pieces in isolation (like mine), but that's a small price to pay. After all, right now all users of spawnPipe/dynamicLogString are getting doubly encoded UTF-8 which might or might not work in xmobar and probably breaks horribly everywhere else, so this fix should be a clear improvement. :-) Fixes: 1d0eadd ("Make spawnPipe to use system's locale encoding") Fixes: xmonad#377 Fixes: https://github.com/jaor/xmobar/issues/476 Related: xmonad#334 Related: https://github.com/jaor/xmobar/pull/482
2 tasks
liskin
added a commit
to liskin/xmonad-contrib
that referenced
this pull request
Feb 16, 2021
…pLog' For many (10+) years, we had a cascade of ugly workarounds: * X.U.Run.spawnPipe returned a binary handle, so the String passed to it must have been encoded by C.B.UTF8.String.encodeString * X.H.DynamicLog.dynamicLogString therefore returned such an encoded String, so one could use it directly in a hPutStrLn, but literal Strings wouldn't work * xmonadPropLog' also expected an encoded String to make it easier to use together with dynamicLogString, again breaking usage with String literals and other normal unicode Strings Then in 1d0eadd Sibi fixed spawnPipe to return a handle usable with normal Strings, which then obviously broke the entire cascade. But, instead of using the opportunity to remove all the ugly workarounds, he decided to add some more on top, so now spawnPipe with dynamicLogString outputs doubly encoded UTF-8 and xmobar has a hack to strip this double encoding (https://github.com/jaor/xmobar/pull/482), which only works when XFT is in use and breaks on some long unicode codepoints. :-( There is a better way: make everything just use normal Strings and only encode when it goes out the wire. This means dynamicLogString can be freely mixed with String literals, manual uses of xmonadPropLog' don't need encodeString, and everything just works nicely. This obviously breaks configs that used some of these pieces in isolation (like mine), but that's a small price to pay. After all, right now all users of spawnPipe/dynamicLogString are getting doubly encoded UTF-8 which might or might not work in xmobar and probably breaks horribly everywhere else, so this fix should be a clear improvement. :-) Fixes: 1d0eadd ("Make spawnPipe to use system's locale encoding") Fixes: xmonad#377 Fixes: https://github.com/jaor/xmobar/issues/476 Related: xmonad#334 Related: https://github.com/jaor/xmobar/pull/482
liskin
added a commit
to liskin/xmonad-contrib
that referenced
this pull request
Feb 16, 2021
…pLog' For many (10+) years, we had a cascade of ugly workarounds: * X.U.Run.spawnPipe returned a binary handle, so the String passed to it must have been encoded by C.B.UTF8.String.encodeString * X.H.DynamicLog.dynamicLogString therefore returned such an encoded String, so one could use it directly in a hPutStrLn, but literal Strings wouldn't work * xmonadPropLog' also expected an encoded String to make it easier to use together with dynamicLogString, again breaking usage with String literals and other normal unicode Strings Then in 1d0eadd Sibi fixed spawnPipe to return a handle usable with normal Strings, which then obviously broke the entire cascade. But, instead of using the opportunity to remove all the ugly workarounds, he decided to add some more on top, so now spawnPipe with dynamicLogString outputs doubly encoded UTF-8 and xmobar has a hack to strip this double encoding (https://github.com/jaor/xmobar/pull/482), which only works when XFT is in use and breaks on some long unicode codepoints. :-( There is a better way: make everything just use normal Strings and only encode when it goes out the wire. This means dynamicLogString can be freely mixed with String literals, manual uses of xmonadPropLog' don't need encodeString, and everything just works nicely. This obviously breaks configs that used some of these pieces in isolation (like mine), but that's a small price to pay. After all, right now all users of spawnPipe/dynamicLogString are getting doubly encoded UTF-8 which might or might not work in xmobar and probably breaks horribly everywhere else, so this fix should be a clear improvement. :-) Fixes: 1d0eadd ("Make spawnPipe to use system's locale encoding") Fixes: xmonad#377 Fixes: https://github.com/jaor/xmobar/issues/476 Related: xmonad#334 Related: https://github.com/jaor/xmobar/pull/482
slotThe
pushed a commit
to liskin/xmonad-contrib
that referenced
this pull request
Mar 20, 2021
…pLog' For many (10+) years, we had a cascade of ugly workarounds: * X.U.Run.spawnPipe returned a binary handle, so the String passed to it must have been encoded by C.B.UTF8.String.encodeString * X.H.DynamicLog.dynamicLogString therefore returned such an encoded String, so one could use it directly in a hPutStrLn, but literal Strings wouldn't work * xmonadPropLog' also expected an encoded String to make it easier to use together with dynamicLogString, again breaking usage with String literals and other normal unicode Strings Then in 1d0eadd Sibi fixed spawnPipe to return a handle usable with normal Strings, which then obviously broke the entire cascade. But, instead of using the opportunity to remove all the ugly workarounds, he decided to add some more on top, so now spawnPipe with dynamicLogString outputs doubly encoded UTF-8 and xmobar has a hack to strip this double encoding (https://github.com/jaor/xmobar/pull/482), which only works when XFT is in use and breaks on some long unicode codepoints. :-( There is a better way: make everything just use normal Strings and only encode when it goes out the wire. This means dynamicLogString can be freely mixed with String literals, manual uses of xmonadPropLog' don't need encodeString, and everything just works nicely. This obviously breaks configs that used some of these pieces in isolation (like mine), but that's a small price to pay. After all, right now all users of spawnPipe/dynamicLogString are getting doubly encoded UTF-8 which might or might not work in xmobar and probably breaks horribly everywhere else, so this fix should be a clear improvement. :-) Fixes: 1d0eadd ("Make spawnPipe to use system's locale encoding") Fixes: xmonad#377 Fixes: https://github.com/jaor/xmobar/issues/476 Related: xmonad#334 Related: https://github.com/jaor/xmobar/pull/482
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The current handle returned by
spawnPipedoesn't have any encodingand it uses the function fdToHandle which returns a Binary
Handle. When spawnPipe is used with a program like xmobar, this can
easily lead to errors:
One workaround is to use
hSetEncoding. But fromreading GHC's source code - the entire Handle and write operations
seems stateful. So doing something like hPutStr and hSetEncoding can
theoretically lead to an undefined state as the first write will use
latin encoding and the second write will use the new encoding. More
details about it are present here:
So having these new functions will ensure that we get the handles in
the proper encoding in the first place.
Checklist
I've read CONTRIBUTING.md
I tested my changes with xmonad-testing
I updated the
CHANGES.mdfile