feat: support for block border merging - #1729
Conversation
| fn render_left_side(&self, area: Rect, buf: &mut Buffer) { | ||
| if self.borders.contains(Borders::LEFT) { | ||
| for y in area.top()..area.bottom() { | ||
| for y in area.top() + 1..area.bottom() - 1 { |
There was a problem hiding this comment.
Here the first and last draw calls are removed. Indeed they overlap with the border so the merging is done where there should be no merging.
There was a problem hiding this comment.
Add a comment about the +1 / -1. Seems easy that there would be some off by one errors that are difficult to guess about here.
| for y in area.top() + 1..area.bottom() - 1 { | ||
| buf[(area.left(), y)] | ||
| .set_symbol(self.border_set.vertical_left) | ||
| .merge_symbol(self.border_set.vertical_left) |
There was a problem hiding this comment.
Here I replaced set_symbol with the drop-in replacement merge_symbol. The same operation is done but if a border (or something else) has already been drawn something here, the previous and new character will be merged.
| /// This map should be symetric. | ||
| static MERGE_MAP: OnceLock<HashMap<(&'static str, &'static str), &'static str>> = OnceLock::new(); | ||
|
|
||
| macro_rules! insert_merge_rules { |
There was a problem hiding this comment.
A macro because those merging rules are generic to any style (s in the code) of border.
There was a problem hiding this comment.
I'm not sure I'm seeing the need for this to be a macro - isn't s always symbol::line::Set in the calling code below?
There was a problem hiding this comment.
I'm not sure I'm seeing the need for this to be a macro - isn't s always symbol::line::Set in the calling code below?
Indeed, it might be possible to be generic without this macro. This code tries to be generic and to be used with any call tl the set_symbol method 🤔
| /// Map to know how to merge the two given symbols. | ||
| /// If a couple of symbols are not is the map there is nothing to merge. | ||
| /// This map should be symetric. | ||
| static MERGE_MAP: OnceLock<HashMap<(&'static str, &'static str), &'static str>> = OnceLock::new(); |
There was a problem hiding this comment.
Here we use OnceLock which demands no other dependency. A more idiomatic way to do this would be to use once_cell or lazy_static but this adds a dependency.
The behavior wanted is that this hashmap should be initialize once at first call. It is a static hashmap so after that it is O(1) lookup time.
There was a problem hiding this comment.
What about using LazyLock? Introduced in 1.80 (we're fine to bump the MSRV up to at most 1.83 if necessary given stable is 1.85).
It might also be nice if users are able to modify this value so that their own sets use the combination characters. How would the design of this change if you considered that problem?
There was a problem hiding this comment.
What about using LazyLock? Introduced in 1.80 (we're fine to bump the MSRV up to at most 1.83 if necessary given stable is 1.85).
It might also be nice if users are able to modify this value so that their own sets use the combination characters. How would the design of this change if you considered that problem?
Totally agree. I might use LazyLock. Dont know about specific thread safe difference. I used OnceLock for no specific reason.
| } | ||
|
|
||
| /// Lazy initialization of `MERGE_MAP` | ||
| pub fn get_merge_map() -> &'static HashMap<(&'static str, &'static str), &'static str> { |
There was a problem hiding this comment.
This is not a free operation. This also allocates a pretty big hash map. This is a cost that we have to take into account, however the cost is mainly in startup time and in space. In terms of complexity, this is O(1) for a normal application.
joshka
left a comment
There was a problem hiding this comment.
There's another block border merging approach by @thscharler in #1468 which is worth collaborating with them on to work out what's best. I think the ideal solution is somewhere between this and that solution. (I like the simplicity of this solution, the other solution felt a bit complex to me but I haven't dug in to it in earnest to understand whether that's intrinsic or extrinsic complexity (i.e. caused by the problem or caused by the choice of solution). I suspect a bit of both.
| /// Map to know how to merge the two given symbols. | ||
| /// If a couple of symbols are not is the map there is nothing to merge. | ||
| /// This map should be symetric. | ||
| static MERGE_MAP: OnceLock<HashMap<(&'static str, &'static str), &'static str>> = OnceLock::new(); |
There was a problem hiding this comment.
What about using LazyLock? Introduced in 1.80 (we're fine to bump the MSRV up to at most 1.83 if necessary given stable is 1.85).
It might also be nice if users are able to modify this value so that their own sets use the combination characters. How would the design of this change if you considered that problem?
| /// This map should be symetric. | ||
| static MERGE_MAP: OnceLock<HashMap<(&'static str, &'static str), &'static str>> = OnceLock::new(); | ||
|
|
||
| macro_rules! insert_merge_rules { |
There was a problem hiding this comment.
I'm not sure I'm seeing the need for this to be a macro - isn't s always symbol::line::Set in the calling code below?
| fn render_left_side(&self, area: Rect, buf: &mut Buffer) { | ||
| if self.borders.contains(Borders::LEFT) { | ||
| for y in area.top()..area.bottom() { | ||
| for y in area.top() + 1..area.bottom() - 1 { |
There was a problem hiding this comment.
Add a comment about the +1 / -1. Seems easy that there would be some off by one errors that are difficult to guess about here.
| insert_merge_rules!(map, NORMAL); | ||
| insert_merge_rules!(map, ROUNDED); | ||
| insert_merge_rules!(map, THICK); | ||
| insert_merge_rules!(map, DOUBLE); |
There was a problem hiding this comment.
An oversight of missing characters from this is combination characters e.g. ┍ U+250D (Down light + right heavy). These combinations take this from pretty useful to must have. You can then make UIs where the focused block uses a thicker + colored border for instance. Note that the order of overlap likely matters here too (with any first drawn part of the line being overwritten).
There was a problem hiding this comment.
Definitely something interesting for this PR. Those criteria can be added to the hashmap... those features are clearly a step by step process adding the merge of symbols we want.
Only added on the first one of the +1/-1 to avoid redundancy
This can be a problem because we cannot use as is the separation of the symbol::line::Set struct. However this approach is very general and will be applicable easily to any widget drawing some line or special character. The answer is no to your question. Sometimes thoses characters can come from any previous set_symbol call. With my implementation we do not have the information that the previous character has been set by a call to the Block function (and the buffer type is very powerful I think). It is mainly because I only look at the cell symbol and nothing else. |
This approach is actually specific to the block. (what I have seen of it quite quickly). Mine will even be useful for people writing symbol one by one to make their own widget, or for people drawing things with multiple lines. They would have to change the function from |
| exclude = ["assets/*", ".github", "Makefile.toml", "CONTRIBUTING.md", "*.log", "tags"] | ||
| edition = "2021" | ||
| rust-version = "1.74.0" | ||
| rust-version = "1.80.0" |
There was a problem hiding this comment.
change of rust version because of the use of LazyLock.
|
What breaks all the test is the +1/-1 change when drawing the border of the block. Indeed, when the corner is not present before a line will still be drawn until the end. We get : instead of (there is the first character missing at the left). |
orhun
left a comment
There was a problem hiding this comment.
Nice, I like the simplicity of this PR as well
| @@ -0,0 +1,32 @@ | |||
| use crate::symbols::line::*; | |||
|
|
|||
| pub fn merge_border(prev: BorderSymbol, next: BorderSymbol) -> BorderSymbol { | |||
There was a problem hiding this comment.
simpler code for merging because of the data structure introduced.
joshka
left a comment
There was a problem hiding this comment.
An idea for a declarative macro approach:
enum Weight {
None,
Light,
Heavy,
Double,
TripleDash,
QuadrupleDash,
HeavyTripleDash,
HeavyQuadrupleDash,
}
macro_rules! define_symbols {
/// TBD - copilot is getting pretty good at generating these sorts of things from an example of the output btw.
}
define_symbols! {
// symbol => (left, right, up, down) - not sure about this order, possible also match the
// unicode naming order which is bottom/top, left/right, also perhaps splitting out the
// horizontal and vertical weights would work too
"─" => (Lignt, Light, None, None),
"━" => (Heavy, Heavy, None, None),
"│" => (None, None, Light, Light),
"┃" => (None, None, Heavy, Heavy),
"┄" => (TripleDash, TripleDash, None, None),
"┅" => (HeavyTripleDash, HeavyTripleDash, None, None),
"┆" => (None, None, TripleDash, TripleDash),
"┇" => (None, None, HeavyTripleDash, HeavyTripleDash),
"┈" => (QuadrupleDash, QuadrupleDash, None, None),
"┉" => (HeavyQuadrupleDash, HeavyQuadrupleDash, None, None),
"┊" => (None, None, QuadrupleDash, QuadrupleDash),
"┋" => (None, None, HeavyQuadrupleDash, HeavyQuadrupleDash),
"┌" => (None, Light, None, Light),
"┍" => (None, Heavy, None, Heavy),
...
}Should generate whatever hashtable / match statement / combination is needed
| fn try_from(value: &str) -> Result<Self, Self::Error> { | ||
| use LineStyle::*; | ||
| match value { | ||
| "╷" => Ok(BorderSymbol::new(None, None, None, Some(Normal))), |
There was a problem hiding this comment.
From a visual perspective, this fit on the screen better without wrapping as a tuple (alternatively this is a place to turn off automatic formatting).
I wonder if a compile time hashtable would be best here (e.g. phf crate)
You might be able to also add a none / empty vairant to the enum rather than using Option... Not sure about that though:
Regarding ordering these, I'd like to see the order arranged in the same order as the unicode symbol table.
I suspect that there's a way (perhaps with a small amount of macro magic) to write the table once and generate both the forward and backward table)
There was a problem hiding this comment.
- Seems legit to turn off auto formatting.
- Compile time hashtable could be the way, I was trying to avoid that in favor of functions that may be const someday.
- Ordering is an issue indeed. So ordered increasing order from there unicode value ?
- yes some macro may be able to do the backward table for sure
There was a problem hiding this comment.
croissant? 🥐
The macro can generate a match statement if that makes more sense. How this works (as a hashmap or function) is probably orthogonal (see what I did there... ;)) to how it's defined in code.
There was a problem hiding this comment.
"croissant" means "increasing" in french .. 🫣 (yes this has the spelling and pronunciation as the pastry)
There was a problem hiding this comment.
Ha, ok - I thought it was a typo, but couldn't work out what for...
Je n'ai qu'un peu de français
Co-authored-by: Jagoda Estera Ślązak <128227338+j-g00da@users.noreply.github.com>
Co-authored-by: Jagoda Estera Ślązak <128227338+j-g00da@users.noreply.github.com>
Co-authored-by: Jagoda Estera Ślązak <128227338+j-g00da@users.noreply.github.com>
Co-authored-by: Jagoda Estera Ślązak <128227338+j-g00da@users.noreply.github.com>
Co-authored-by: Jagoda Estera Ślązak <128227338+j-g00da@users.noreply.github.com>
Co-authored-by: Jagoda Estera Ślązak <128227338+j-g00da@users.noreply.github.com>
Co-authored-by: Jagoda Estera Ślązak <128227338+j-g00da@users.noreply.github.com>
Ha, sounds like we should do one or more of:
|
orhun
left a comment
There was a problem hiding this comment.
We're pretty close with this... just needs some docs tweaks + documenting LineStyle
What about more test cases? |
|
yup, more tests == more better == more rat |
Co-authored-by: Orhun Parmaksız <orhunparmaksiz@gmail.com>
Co-authored-by: Orhun Parmaksız <orhunparmaksiz@gmail.com>
Co-authored-by: Orhun Parmaksız <orhunparmaksiz@gmail.com>
Suggestions: |
Co-authored-by: Jagoda Estera Ślązak <128227338+j-g00da@users.noreply.github.com>
|
Can we merge this? |
joshka
left a comment
There was a problem hiding this comment.
Apologies for the lag in getting to this review. I wanted to give it some deep review which required really digging in and understanding it in context rather than a shallower one that focuses on does it work etc. Because this affects things in ratatui-core, we want to try to get this as close to perfect as possible so we can avoid needing to bump -core soon after release.
As an over all implementation, I think the idea of making this part of Cell is a great idea. I think it works better over the alternative which only implemented this for Block.
I think MergeStyle (MergeAction) would be clearer with an Overwrite variant rather than using Option to represent this. This is the main blocking comment I have. It should also be non_exhaustive I think. This will make it possible to allow users to add their own merge functionality in the future (e.g. to handle custom border sets).
I think I'd like to start out making LineStyle and BorderSymbol private rather than having them in the public API. Doing so makes the merge functionality defined by the implementation rather than being constrained to follow / align with types which can't be changed without bumping ratatui-core in the future.
By making those two changes, we get this to a point where the internal details / nits etc. are not breaking with respect to semver and this PR becomes not too problematic to merge soon.
--
In particular, I think at some point in the future it makes sense to have a BoxDrawingCharacter type (or something similar), which represents the Unicode metadata about these symbols nicely (and which helps this implementation).
An alternative approach over using LineStyle is to have a (symbol, symbol) => result function / hashmap. As a table, this would be 16kB (128 chars in the block ^ 2), so that's not particularly large (not enough to worry about at least).
Broadly applicable comment:
- leave a newline before doc comments on enum variants and functions. This makes the comment more easy to align with the code that it's commenting (I don't think there's a rustfmt option for this, but I'd turn it on if there was).
There's a few things which seem like they're oddly placed:
- functions which seem like they're good candidates to be methods
- merge implementation in Cell rather than as a method MergeStyle
On the testing side of this, I think it makes sense to have some basic symbol, symbol => symbol type tests of the merging in addition to the checks on block.
For the block tests, I can visualize some simpler block drawings that might help test the block combinations a bit better too, but they're harder to describe than implement.
- A block, surrounded on each corner by 4 blocks (or partial blocks to save space). I think this works out to 6x6 in terms of necessary rendered space.
- 4 combinations of the above (I think this would be renderable in 24x6)
- touching corners exactly
- touching corner and vertical sides
- touching corner and horizontal sides
- crossing vertical and horizontal sides
- for each of the combinations between (plain, thick, double, rounded) add a line (16 options, so this becomes 24x96) or go across then down (96x24) could also work pretty neatly and fit on a screen and be fairly easy to look at and see that it's correct
I'll slap together a quick demo of what I mean here shortly.
@j-g00da if you've got a bunch of motivation to continue this PR, you're welcome to. If you want to add your own commits on top and have them run through as a normal PR, feel free to clone this, add your commits and make a new PR. We'll credit the eventual single commit to @pauladam94, and add a Co author on it.
Otherwise, there's a bunch of things I've mentioned above that could make sense for me to just do that I can update and push to Paul's branch. Let me know how you'd like to proceed?
| let previous = self.symbol.as_str(); | ||
| let next = symbol; | ||
| if let (Ok(s1), Ok(s2), Some(style)) = ( | ||
| BorderSymbol::try_from(previous), | ||
| BorderSymbol::try_from(next), | ||
| style, | ||
| ) { | ||
| if let Ok(merged) = TryInto::<&str>::try_into(&merge_border(&s1, &s2, style)) { | ||
| self.set_symbol(merged); | ||
| return self; | ||
| } | ||
| } | ||
| self.set_symbol(next); | ||
| self |
There was a problem hiding this comment.
I think pushing most of this down into MergeStyle might make sense in terms of where it belongs:
| let previous = self.symbol.as_str(); | |
| let next = symbol; | |
| if let (Ok(s1), Ok(s2), Some(style)) = ( | |
| BorderSymbol::try_from(previous), | |
| BorderSymbol::try_from(next), | |
| style, | |
| ) { | |
| if let Ok(merged) = TryInto::<&str>::try_into(&merge_border(&s1, &s2, style)) { | |
| self.set_symbol(merged); | |
| return self; | |
| } | |
| } | |
| self.set_symbol(next); | |
| self | |
| let symbol = merge_style.map(|ms| ms.merge(self.symbol(), symbol)); | |
| self.set_symbol(symbol) |
Alternatively (thinking about this as I read through the PR a bit), if MergeStyle has an Overwrite variant instead of using Option::None, then this is even simpler:
| let previous = self.symbol.as_str(); | |
| let next = symbol; | |
| if let (Ok(s1), Ok(s2), Some(style)) = ( | |
| BorderSymbol::try_from(previous), | |
| BorderSymbol::try_from(next), | |
| style, | |
| ) { | |
| if let Ok(merged) = TryInto::<&str>::try_into(&merge_border(&s1, &s2, style)) { | |
| self.set_symbol(merged); | |
| return self; | |
| } | |
| } | |
| self.set_symbol(next); | |
| self | |
| self.set_symbol(merge_style.merge(self.symbol(), symbol)) |
| /// Represents a composite border symbol using individual line components. | ||
| pub struct BorderSymbol { | ||
| pub right: LineStyle, | ||
| pub up: LineStyle, | ||
| pub left: LineStyle, | ||
| pub down: LineStyle, | ||
| } |
There was a problem hiding this comment.
This seems like it's about representing a "Box Drawing Character" as its composite parts. I wonder if making that part of the naming here could be useful (e.g. BoxDrawingSymbol). That said, there's a few box drawing symbols in the block that can't be represented with these values (╱ ╲ ╳)
| }; | ||
| } | ||
|
|
||
| define_symbols!( |
There was a problem hiding this comment.
I think it would be best to put these in the same order as the Unicode block.
Make a note about the diagonal characters which aren't handled here. ╱ ╲ ╳
| } | ||
|
|
||
| impl LineStyle { | ||
| fn replace(self, from: &Self, to: &Self) -> Self { |
There was a problem hiding this comment.
Needs a small comment, but could reasonably be inlined once LineStyle implements Copy as the following becomes a single line in BorderSymbol::replace:
self.up = if self.up == from { to } else { from };
| pub fn best_fit(mut self) -> Self { | ||
| use LineStyle::{Double, Plain, Rounded, Thick}; | ||
| // Check if we got a character that can be displayed after change | ||
| if TryInto::<&str>::try_into(&self).is_ok() { |
There was a problem hiding this comment.
This simplifies to self.parse() if we have FromStr implemented.
There was a problem hiding this comment.
I was wrong. That's the other direction that this would apply to (str -> BorderSymbol)
| /// Merges symbols only if an exact composite unicode character exists. | ||
| /// Example: `┐` and `┗` will be merged into `╄` | ||
| #[default] | ||
| Exact, |
There was a problem hiding this comment.
Comment should cover a bit about why this is needed in a narrative form (e.g. There are some symbol combinations that are missing such as rounded corners combining with straight lines and thick lines with double lines.
I'd think in general that BestFit would be a better default, but given that I'm suggesting that Overwrite should be a variant, that makes more sense rather than having a two tier default (None / Some(Exact))
| /// Sets the block's [`MergeStyle`] for overlapping characters. Setting it to `None` | ||
| /// never merges characters. | ||
| #[must_use = "method moves the value of self and returns the modified value"] | ||
| pub const fn merge_style(mut self, merge_style: Option<MergeStyle>) -> Self { |
There was a problem hiding this comment.
This should talk about how this is related to the border and perhaps be named merge_borders or something similar. It definitely should not have style in the name.
|
@joshka I can get back to this on the weekend |
Co-authored by @pauladam94 Fork of #1729 --------- Co-authored-by: pauladam94 <poladam2002@gmail.com> Co-authored-by: Paul Adam <65903440+pauladam94@users.noreply.github.com> Co-authored-by: Orhun Parmaksız <orhunparmaksiz@gmail.com> Co-authored-by: Orhun Parmaksız <orhun@archlinux.org> Co-authored-by: Josh McKinney <joshka@users.noreply.github.com>
|
Implemented in #1874 |

Description
This is a feature that has been proposed for the next release of ratatui (0.30.0) here.
Example of the Result (Before Left - After Right)
Remark
Line and Corner behavior
When the line is drawn for the border block, the last border character is also drawn. This poses a problem because it might merge with the corner in a wrong way.
There is 2 way to fix that :
Future Work
merge_symbolinstead for theset_cellfunction all the time