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

Fix encoding issues in spawnPipe/dynamicLogString/xmonadPropLog #471

Merged
merged 2 commits into from Mar 20, 2021

Conversation

liskin
Copy link
Member

@liskin liskin commented Feb 16, 2021

Description

X.U.Run: Clean up spawnPipes a bit

This makes it easier to see the differences between these functions,
makes it less likely someone will change one and not the others, etc.
More importantly, the documentation doesn't contain circular references
any more. :-)

Also, let's just use hSetEncoding. The concern of this being stateful
and theoretically having something written in the wrong encoding is
pointless: nobody has the handle until we return it from spawnPipe'.

(This also means that spawnPipeWithNoEncoding is now a text handle that
possibly does newline translation, just with char8 encoding. There
should be no difference in practice.)

Fixes: 8b2bd3a ("Add new variants of spawnPipe functions with encoding support")

X.H.DynamicLog: Move UTF8 encoding from dynamicLogString to xmonadPropLog'

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: #377
Fixes: https://github.com/jaor/xmobar/issues/476
Related: #334
Related: https://github.com/jaor/xmobar/pull/482

Checklist

  • I've read CONTRIBUTING.md

  • n/a I tested my changes with xmonad-testing

  • I updated the CHANGES.md file

  • n/a I updated the XMonad.Doc.Extending file (if appropriate)

Copy link
Member

@psibi psibi left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing this in a much better way!

@PRESFIL
Copy link
Contributor

PRESFIL commented Feb 17, 2021

Working! See answer.

@TheMC47
Copy link
Member

TheMC47 commented Feb 20, 2021

This also solves another issue I had, where xmobar couldn't correctly parse a combination of fn and raw tags with a special font. Awesome 🎉

@liskin
Copy link
Member Author

liskin commented Feb 20, 2021

Oh yeah, that's a good point possibly worth mentioning somewhere. xmobarRaw wouldn't work without this.

@PRESFIL
Copy link
Contributor

PRESFIL commented Feb 28, 2021

Any updates on this PR?

@liskin
Copy link
Member Author

liskin commented Mar 5, 2021

@PRESFIL I've been busy hacking on other projects, and I'd like to at least have a look at #465 before merging this, so I'd expect this to be merged hopefully next week-ish.

This makes it easier to see the differences between these functions,
makes it less likely someone will change one and not the others, etc.
More importantly, the documentation doesn't contain circular references
any more. :-)

Also, let's just use hSetEncoding. The concern of this being stateful
and theoretically having something written in the wrong encoding is
pointless: nobody has the handle until we return it from `spawnPipe'`.

(This also means that spawnPipeWithNoEncoding is now a text handle that
possibly does newline translation, just with char8 encoding. There
should be no difference in practice.)

Fixes: 8b2bd3a ("Add new variants of spawnPipe functions with encoding support")
…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 slotThe force-pushed the pr/spawnpipe-dynamiclog-encoding branch from 0b74f05 to 63e31cc Compare March 20, 2021 17:11
@slotThe
Copy link
Member

slotThe commented Mar 20, 2021

Rebased and merged, see #481 for reasons to do this before #465

@slotThe slotThe merged commit e3a13a5 into xmonad:master Mar 20, 2021
@liskin liskin deleted the pr/spawnpipe-dynamiclog-encoding branch March 20, 2021 23:13
liskin added a commit to liskin/xmobar that referenced this pull request Apr 27, 2021
This reverts commits c6669e2 (partially; (changelog entry kept),
73e0293, 78efa59.

These commits were introduced as a workaround for a double UTF-8
encoding bug in xmonad-contrib which itself was meant to be a
fix/workaround for another issue. We have now identified the underlying
issue and fixed it right at the root, so this entire cascade of hacky
(and wrong) workarounds can be safely reverted. Thankfully, none of the
xmonad-contrib hacks made it into a release, so there's no backward
compat to worry about.

Should anyone be interested in the details,
xmonad/xmonad-contrib@63e31cc
provides a summary and links to all the related issues and PRs.

Related: https://github.com/jaor/xmobar/pull/482
Related: https://github.com/jaor/xmobar/issues/476
Related: xmonad/xmonad-contrib@63e31cc
Related: xmonad/xmonad-contrib#471
Related: xmonad/xmonad-contrib#377
liskin added a commit to liskin/xmobar that referenced this pull request Apr 27, 2021
This reverts commits c6669e2 (partially; changelog entry kept),
73e0293, 78efa59.

These commits were introduced as a workaround for a double UTF-8
encoding bug in xmonad-contrib which itself was meant to be a
fix/workaround for another issue. We have now identified the underlying
issue and fixed it right at the root, so this entire cascade of hacky
(and wrong) workarounds can be safely reverted. Thankfully, none of the
xmonad-contrib hacks made it into a release, so there's no backward
compat to worry about.

Should anyone be interested in the details,
xmonad/xmonad-contrib@63e31cc
provides a summary and links to all the related issues and PRs.

Related: https://github.com/jaor/xmobar/pull/482
Related: https://github.com/jaor/xmobar/issues/476
Related: xmonad/xmonad-contrib@63e31cc
Related: xmonad/xmonad-contrib#471
Related: xmonad/xmonad-contrib#377
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XMonad.Hooks.DynamicLog corrupt non-ASC characters The fonts are not being rendered properly
5 participants