Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow Lines to be wrapped individually wrapped #1091

Closed
wants to merge 1 commit into from

Conversation

kxxt
Copy link
Contributor

@kxxt kxxt commented May 9, 2024

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
Copy link
Member

joshka commented May 9, 2024

As a heads up, you might want to digest #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
Copy link
Contributor Author

kxxt commented May 9, 2024

@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.

@kxxt kxxt closed this May 9, 2024
@joshka
Copy link
Member

joshka commented May 9, 2024

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
Copy link
Contributor Author

kxxt commented May 9, 2024

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
Copy link
Member

joshka commented May 9, 2024

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 #174 (another deep problem to solve).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants