Fix unicode text width calculation #1271
Comments
|
Not sure where the problem is. Rendering ignores graphemes of width 0 and control characters. Only after that the width is relevant for rendering. At that point The only thing I see here are multiple places of truth, but that should be fixed by reducing the amount of render functions to ideally 1. Currently, |
|
Option 2, but it we don't need an extension. Text/Line/Span::width are just methods. They should return the width that will be rendered, not the width including control characters. |
I guess my concern is that there a number of usages of
If we only touch Span, I'm worried it'll be a game of wack-a-mole with the other uses. |
The other three meaningful places in a simple search are:
|
|
Off-topic:
Scrollbar has a bunch of |
I think I'd lean to maybe not there, as these seem still useful as the scrollbar sets will likely still be unstyled, and it's easier to call 4 methods to style the track/thumb/begin/end than to create a whole new object for this. Especially if you just want one of them styled. But that's a weak opinion that would be contradicted by showing that the interface is easier with spans. |
|
Ok, so to summarize everybody would be ok with just fixing |
|
Coming back to this after #1385 - related discussion;
To summarize, newlines are treated as 0 length in However, that's not why our CI is currently failing. It's due to unicode-rs/unicode-width@afab363 Simple repro: assert_eq!("👩".width(), 2); // Woman
assert_eq!("🔬".width(), 2); // Microscope
assert_eq!("👩🔬".width(), 4); // Woman scientist -> should be 4 but it expect 2Zero width joiner ( So, how should we proceed with this? I see two options:
Thoughts? |
- assert_eq!("👩🔬".width(), 4);
+ assert_eq!("👩🔬".width(), 2); This new behaviour is what we want, right? I would vote for upgrading to v0.2 but pinning the dependency, and manually upgrading every now and then. Is it possible to pin a version like that? It would be nice to not have a minor version upgrade break unexpectedly because we can put the time and effort into solving the problem when we get a chance to plan for it. |
I don't know if we want it, there is #925 and #75 which are related but I might still be lacking some context.
Sounds reasonable, but note that we need to update our tests accordingly and this will probably be a breaking change.
Yup, I think it's possible. |
|
The problem with a pin is that it means that any code that also uses unicode-width will need to also be pinned (I htink). This might lead to a situation where an app calls our code and some other code that calls unicode width expecting certain behavior. I might be wrong about that though. It's worth testing to see what happens there. |
|
Unicode defines Emojis as width == 2. Joined Emojis are width 2. Most Terminals display them by printing the participating emojis rather than the joined one, so it's not a bug of ratatui. (Unicode allows for this case, but its not the one someone should aim for) Also, unicode-width states:
Basically: When the implementation details of unicode-width are relevant, then the code is likely wrong. |
|
So does that give us to freedom to just follow |
I was not wrong - pinning to a specific point revisions negatively affects our dependents who might make different choices about which point version to pin: zhiburt/tabled#423 (comment)
Moving to 0.2.0 would make it possible to have two versions, but would still run into the same problem if we then pinned the patch version. I think this is probably reasonable though, and we should just make it clear why we're doing this. That perspective is that (for the docs):
So I also agree that we should move to 0.2.0 and pin at the latest patch version, we should manually check the changes in each patch version to understand their impact on rendering widgets. Separately we should look at the usages of unicode-width in ratatui and make sure that we understand when this usage should be avoided because it will cause incorrect calculations. |
|
If we vendor a version of unicode-width, that would solve the problem, right? I'm not saying we should do that though because it seems extreme. It would have been nice if there was an alternative to pinning that that basically did the equivalent of "vendor". |
|
I don't like the vendoring approach in general as it has its own set of problems. Not sure that we need to go too deep on the problems of pinning until they rear their head again. Likely it would be better worth our time to spend it on understanding the changes that impact how unicode-width affects rendering. |
See #1249 (comment) and #1270 (comment)
UnicodeWidthStr::widthcounts newlines as one unit which is not what we want for terminal rendering. We need to figure out how we want to fix this. Some options:widthwith our own extension (maybe calledrenderable_width) which does the right thing for ratatui.widthsays, e.g. render a space for a newline.I don't really have a preference between 2 and 3, but I think most people prefer 2?
cc: @joshka @EdJoPaTo
The text was updated successfully, but these errors were encountered: