Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.4%. Comparing base (935a718) to head (86fe49b).
Report is 2 commits behind head on main.

@@          Coverage Diff          @@
##            main   #1226    /-   ##
=====================================
  Coverage   94.4%   94.4%           
=====================================
  Files         62      62           
  Lines      15056   15084    28     
=====================================
  Hits       14225   14253    28     
  Misses       831     831           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

marking it as a breaking change

I don't think this is a breaking change in its current form. It's a bug fix restoring old behavior. Before unicode-width reported these control characters as width 0 which then got filtered out.

https://github.com/ratatui-org/ratatui/blob/935a7187c273e0efc876d094d6247d50e28677a3/src/buffer/buffer.rs#L213-L214

unicode-width changed their behavior (whether it's considered breaking or not is debatable as it's as far as I understand according to Unicode specs, so it's only a fix to be spec compliant) and this restores the behavior of before: filtering them out.
So it's basically preventing a breaking change for users of ratatui.

Are there any that would make you not want to merge this as-is?

Perhaps documenting the behavior somewhere user facing could be a good idea

• This does not include documentation at all and I dont really know how to document something that restores old behaviour.
• Is this the same set of control characters which were width == 0 before? So is it actually restoring the exact old behaviour or is it introducing other bugs now?
• Assign width 1 to control characters unicode-rs/unicode-width#45 suggests following Unicode to render a fallback glyph instead to notify the user of a missing element here being not rendered. Which would be both the more correct approach according to Unicode and the more helpful one to notice that there are more characters than visible.

I think I would like the last one the most. Rendering some fallback symbol instead. But this PR currently does something different.

Is this the same set of control characters which were width == 0 before? So is it actually restoring the exact old behaviour or is it introducing other bugs now?

I read the spec and the two code paths and they are equivalent. 0x00..0x20 and 0x7F..0xA0 are treated as control characters and are length 1.

unicode-rs/unicode-width#45 suggests following Unicode to render a fallback glyph instead to notify the user of a missing element here being not rendered. Which would be both the more correct approach according to Unicode and the more helpful one to notice that there are more characters than visible.

I think I would like the last one the most. Rendering some fallback symbol instead. But this PR currently does something different.

I think that's a good idea also, but I think it would be worth making an application configurable option. So just filtering seems like a good default.

I don't think this is a breaking change in its current form. It's a bug fix restoring old behavior. Before unicode-width reported these control characters as width 0 which then got filtered out.

👍

This does not include documentation at all and I dont really know how to document something that restores old behaviour.

What about a one line mention in Terminal::draw's doc comment?

unicode-rs/unicode-width#45 suggests following Unicode to render a fallback glyph instead to notify the user of a missing element here being not rendered. Which would be both the more correct approach according to Unicode and the more helpful one to notice that there are more characters than visible.

I think I would like the last one the most. Rendering some fallback symbol instead. But this PR currently does something different.

I think that's a good idea also, but I think it would be worth making an application configurable option. So just filtering seems like a good default.

Let's go with restoring the old behavior for now: filtering them out completely and release a bug fix release 0.27.1 with this.

In the long run… Configuring this would require a Buffer configuration which seems annoying? Maybe set_stringn should be moved out of the Buffer then? Anyway, that's a longer run question. Maybe create another issue for this to discuss this further…

This does not include documentation at all and I dont really know how to document something that restores old behaviour.

What about a one line mention in Terminal::draw's doc comment?

It seems far away from actually drawing something to the terminal but maybe that's a technical view… Any good ideas for such a one line?

It seems far away from actually drawing something to the terminal but maybe that's a technical view… Any good ideas for such a one line?

My bad - I was thinking about the setting being needed on Terminal to control the buffer. Probably on set_string / set_stringn would be better for this. Just "This method filters out (does not output) control characters" or something similar.

A lot is depending on set_stringn which doesn't document this either, but I think it's fine.

And while at it… set_line and set_span should probably be deprecated (in their own pr) in favor of Line as a widget in order to simplify the Buffer API to its core functionality.

Anything other needed for a 0.27.1 release?

Anything other needed for a 0.27.1 release?

@orhun is away for a couple of weeks. He usually handles all the release activities (there's a bunch of stuff that's not automated), I think it'll be around the 26th or so.

I'm not sure what the point of the labels accepted or review needed is, as that's something GitHub itself already displays?

Also, why accepted and not just merged then?

I think there should be automated quick released features and bug fixes… so versions like ratatui 0.27.7 could easily be the case… Similar to the weekly alpha release currently which seem a bit weird to me as the Cargo.toml could also just point to the main branch. The point of PR reviews is to ensure that it works correctly when merged, so it should also be releasable? Breaking PRs can be merged at the same time and released together.
That way the current bus factor is reduced and merged stuff ends up quicker with the users?

I'm using labels mostly as an aid to whether I need to look at an issue again at some point. I didn't merge this one in case there was anything you wanted to do before merge. Feel free to merge if there's nothing pressing.

Agreed on the quick releases etc. we're not there right now, but are close. The alpha release was created to address the gap when there was a much longer period between releasing and to make it possible for downstream apps to easily publish crates that relied on unreleased functionality.

However I'm a fan of releasing whenever there's a reasonable amount of stuff to go however. Every few days if needed. When Orhun is back let's discuss making that easier. (Perhaps cut an issue or discussion about it)

I'm using labels mostly as an aid to whether I need to look at an issue again at some point.

This is selectable by queries (and the buttons at the top create them for you)
https://github.com/ratatui-org/ratatui/pulls?q=is:pr is:open -reviewed-by:@me

Agreed on the quick releases etc. we're not there right now, but are close. The alpha release was created to address the gap when there was a much longer period between releasing and to make it possible for downstream apps to easily publish crates that relied on unreleased functionality.

However I'm a fan of releasing whenever there's a reasonable amount of stuff to go however. Every few days if needed. When Orhun is back let's discuss making that easier. (Perhaps cut an issue or discussion about it)

→ #1232