search

ratatui-org / ratatui

Rust library that's all about cooking up terminal user interfaces (TUIs) 👨‍🍳🐀
https://ratatui.rs
MIT License
8.96k stars 274 forks source link

feat: allow `Line`s to be wrapped individually wrapped #1091

Closed kxxt closed 2 months ago

kxxt commented 2 months ago

I am implementing #201 because I need this feature.

This PR is still a WIP as I haven't added tests. Early feedback is always welcome!

How the individual line wrapping should interact with the paragraph's wrapping is a big headache. For now this PR only handles wrapping for lines without considering the interaction with paragraphs, which means that when rendering paragraph, `Line's' wrap is ignored.

It seems I also need to update Text to make it work in the List.

joshka commented 2 months ago

As a heads up, you might want to digest https://github.com/ratatui-org/ratatui/issues/293 for context before tackling just a part of this.

We're effectively rewriting Line::render_ref in #1089 to fix the unicode truncation bug #1032, so you're aiming at a moving target right now and will have to rewrite your changes with that in mind.

My biggest concern about introducing a change like this is that it's important to do it in a way that doesn't break users of the new API many times as the new functionality is introduced. This means gating things behind unstable flags and if there are breaking changes, doing them in a way that gives many several major versions worth of compiler deprecation warnings.

Ideally, we'd implement something fully new here, and fully deprecate the reflow module. That likely means a new WrapOptions type rather than using the existing type. It also means that we shouldn't make the new functionality dependent in any way on the reflow module.

Lastly, taking a brief look at the implementation suggests that it adds significant complexity to the render method (29 Loc -> 64 LoC). From an implementation perspective, it's important that these particular methods are simple, obvious, maintainable, with easily reasoned about structure and performance. The number of levels of indentation is usually a good indicator of this. The PR's structure changes things to the following nesting:

fn ...
    if ...
        while ...
            for ...
                if ...
  else
      if ...
      else ...
      for ...

I wouldn't review this PR without simplifying it significantly. Finding the right simplifications on this particular method is a pretty hard thing to do even for us core devs. Take a read of the various iterations of #1089 for some insight on this.

kxxt commented 2 months ago

@joshka Thanks for your feedback! As I dig deeper I find that it's much more complex to make it work because I also need to modify ListItem and Text. Given that I need this feature now and it will take weeks(or even monthes) for me to finish this PR and get it reviewed and merged. I think it makes more sense for me to create a tailored widget that suites my own need rather than getting it merged into ratatui. So I am closing this PR for now.

joshka commented 2 months ago

I think it makes more sense for me to create a tailored widget

I'm not sure what the underlying problem you're solving is, but reading between the lines (no pun intended), it sounds like it's probably that you have a non wrappable part and a wrappable part in a list. Perhaps using https://crates.io/crates/tui-widget-list and having a small widget that has a non-wrappable line and a wrapped paragraph (or pre-wrapped text using textwrap) could be an approach that works for you.

kxxt commented 2 months ago

I think it makes more sense for me to create a tailored widget

I'm not sure what the underlying problem you're solving is, but reading between the lines (no pun intended), it sounds like it's probably that you have a non wrappable part and a wrappable part in a list. Perhaps using https://crates.io/crates/tui-widget-list and having a small widget that has a non-wrappable line and a wrapped paragraph (or pre-wrapped text using textwrap) could be an approach that works for you.

Basically I am building something similar to tui-widget-list but not the same. The list I am building might have items taller than the view port so I want it scrolls pixel by pixel instead of item by item. Initially I want to implement it by creating a wrapper component of the built-in list with a scrollview but that would need ListItem to implement text wrapping. Now I guess I should put a Vec of Paragraphs inside a scrollview and manually manage the selection. Pre-wrapping the text isn't really a choice for me because I need to keep the styles, which means I need a solution at the StyledGraphemes level.

joshka commented 2 months ago

The list I am building might have items taller than the view port so I want it scrolls pixel by pixel instead of item by item

Makes sense - I think we'd probably also want that behavior in the main list eventually, but it's a difficult one to get right without breaking all existing code. And it has a lot of overlap with the concepts in https://github.com/ratatui-org/ratatui/issues/174 (another deep problem to solve).