`X.P.Unicode`: `BS` -> `String` to support Unicode descriptions

xmonad / xmonad-contrib

Contributed modules for xmonad

https://xmonad.org

BSD 3-Clause "New" or "Revised" License

588 stars 274 forks source link

`X.P.Unicode`: `BS` -> `String` to support Unicode descriptions #735

Closed PRESFIL closed 2 years ago

PRESFIL commented 2 years ago

This PR changes X.P.Unicode string type from ByteString to String to support Unicode in descriptions. Closes #733.

Description

X.P.Unicode allows you to work with Emoji in Unicode, but does not allow you to use non-Latin characters in the description of these Emojis. X.P.Unicode works fine, but cannot display such characters correctly.

See how to reproduce in #733.

Chose String instead of Text because I thought it was possible to do without an extra dependency for xmonad-contrib. The branch is editable if you know how to add support without dropping the ByteString.

Checklist

[ ] I've read CONTRIBUTING.md
[x] I've considered how to best test these changes (property, unit, manually, ...) and concluded: XXX
[x] I updated the CHANGES.md file

liskin commented 2 years ago

I imagine it might have been done for performance reasons as the list of all unicode codepoints is fairly large and doing it with String can be an order of magnitude slower. Whether that's actually a problem in practice, I have no idea. If it is then depending on text would probably be fine I guess.

PRESFIL commented 2 years ago

OK, I updated CHANGES.md.

PRESFIL commented 2 years ago

On the performance side, I've noticed performance drops in XMonad.Prompt.Man when doing incremental searches. But there are a lot of man pages (5633 lines in prompt).

Or when using XMonad.Prompt.Zsh. But the reason is XMonad.Prompt, it's entirely writen on String base, so rewriting it is a separate issue.

PRESFIL commented 2 years ago

Do I have to update the changes according to master in order for this PR to be merged?

PRESFIL commented 2 years ago

Why not merge it?

slotThe commented 2 years ago

Sorry, seems like everyone was equally busy with other things :)

On the performance side, I've noticed performance drops in XMonad.Prompt.Man when doing incremental searches. But there are a lot of man pages (5633 lines in prompt).

Or when using XMonad.Prompt.Zsh. But the reason is XMonad.Prompt, it's entirely writen on String base, so rewriting it is a separate issue.

You have noticed these performance drops only with this patch applied? Because to me it seems like X.P.Unicode is an extra prompt, not something that interacts with X.P.Man in any way.

PRESFIL commented 2 years ago

I only noticed this in X.P.Man, it also does use String and lots of man pages. It doesn't related with this patch.

I think this is the topic of a separate issue.

slotThe commented 2 years ago

Thanks!

PRESFIL commented 2 years ago

:rocket: