go-runewidth icon indicating copy to clipboard operation
go-runewidth copied to clipboard

Upgraded rivo/uniseg to latest version, switched StringWidth/Truncate to speedier version

Open rivo opened this issue 2 years ago • 11 comments

The rivo/uniseg package has received a major update which also includes methods for grapheme cluster parsing that are much faster than the previously used Graphemes class.

I've upgraded your package accordingly and updated the relevant code to use these faster methods. It would be great if you could merge these changes.

Thank you!

ps. I noticed that some automatic checks did not complete successfully because they are still running on Go 1.15. Would you like me to look into upgrading them to the current version (1.18)?

rivo avatar Jul 27 '22 11:07 rivo

Codecov Report

Merging #63 (0a21090) into master (f9d5553) will increase coverage by 0.25%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   93.56%   93.82%   +0.25%     
==========================================
  Files           3        3              
  Lines         171      178       +7     
==========================================
+ Hits          160      167       +7     
  Misses          6        6              
  Partials        5        5              
Impacted Files Coverage Δ
runewidth.go 95.10% <100.00%> (+0.25%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov-commenter avatar Sep 11 '22 11:09 codecov-commenter

I upgraded again to the latest uniseg version. Also implemented the "Quick Fix" solution outlined in https://github.com/mattn/go-runewidth/issues/59#issuecomment-1242923961.

Also updated CI to use current Go versions. Everything passes now.

rivo avatar Sep 11 '22 11:09 rivo

Do you still need information or any action from me here?

rivo avatar Sep 18 '22 16:09 rivo

👍 to getting this merged

derekperkins avatar Nov 05 '22 17:11 derekperkins

@mattn is there anything blocking this from merging and cutting a new release?

derekperkins avatar Dec 05 '22 18:12 derekperkins

@rivo Thanks for the patch. I can confirm that this fixes a display issue of my program.

junegunn avatar Jan 04 '24 04:01 junegunn

Hi @mattn just checking if there's anything specific blocking this merge?

@rivo Are you using this branch yourself in another project? Is your fork something I could safely use myself?

jesseduffield avatar Jun 23 '24 10:06 jesseduffield

@jesseduffield I'm using this package but not this branch. After tons and tons of hours and effort getting into the details of Unicode and terminals, including writing my own version of a character width package (https://github.com/rivo/uniseg?tab=readme-ov-file#monospace-width), I realize that which package to use really depends on what you're trying to achieve. go-runewidth will be more or less compatible with most terminals out there. Most of them use the widespread wcwidth implementation which, unfortunately, is old and buggy. Many emojis don't render properly, spacing is wrong is lots of cases. And similar issues have been found here, in go-runewidth. But if your application runs in these terminals, you'll probably want to make the same "mistakes". It doesn't help if a character is really 2 spaces wide but the terminal will always advance by 1 space. So go-runewidth is your best bet in doing what the terminal is doing.

On the other hand, the uniseg package attempts to give you the "real" width of a character. If your application has control over where a character is placed, I would suggest using uniseg instead. For example, if you write your own text editor GUI (e.g. something like VSCode) where you don't rely on terminals, I would not suggest using go-runewidth but uniseg instead.

Regarding this branch, I don't know what the plans are. This whole thing is a complicated topic and I totally understand if @mattn doesn't have the time to get into the details. Looks like there's not much happening in this project anymore anyway.

Maybe I should add wcwidth compatibility to uniseg at some point. Then people could simply switch over.

rivo avatar Jun 23 '24 10:06 rivo

I got some errors in this branch.

./runewidth_go118.go:188:28: undefined: uniseg.FirstGraphemeClusterInString
./runewidth_go118.go:214:33: undefined: uniseg.FirstGraphemeClusterInString
./runewidth_go118.go:244:33: undefined: uniseg.FirstGraphemeClusterInString

mattn avatar Jun 23 '24 12:06 mattn

@mattn I haven't spent time on this PR anymore as it seemed like you didn't want to follow up on it / merge it. I can fix these issues but only if you indicate that you will merge it. Otherwise, it would be a wasted effort. Please let me know.

Also, your go.mod still lists Go 1.9 as a requirement. uniseg requires 1.18 due to the use of generics. Would you consider bumping your Go version to 1.18 as well? It would make things much easier.

rivo avatar Jun 23 '24 16:06 rivo

@rivo thanks for that detailed explanation, that's really helpful

jesseduffield avatar Jun 23 '24 23:06 jesseduffield