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

Looser coupling with vt100 #152

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

chris-olszewski
Copy link
Contributor

Addresses #150

Adds a generic Screen and Cell traits that opens the possibility of using tui-term with libraries other than vt100.

This is a breaking change in a few ways:

  • PseudoTerminal now contains a generic.
  • Automatic dereferencing no longer works for the constructor e.g. PseudoTerminal::new(&parser.screen()) will fail. This could be avoided with a impl<'a> Screen for &'a vt100::Screen, but wasn't sure this was desired. Will add it if desired.

Please let me know if there's additional tests that would be beneficial.

Alternatives:
We could try to avoid leaking the underlying Screen type by storing a &dyn Screen, but that would require reworking the interface to not have an associated type.

@a-kenji a-kenji changed the base branch from release to development March 2, 2024 10:03
@a-kenji
Copy link
Owner

a-kenji commented Mar 2, 2024

Thank you for the pr!
I think it is already in a good shape.

Automatic dereferencing no longer works for the constructor

If needed we can add it. Maybe it makes sense to keep backwards compatibility? Though again I wouldn't focus too much on that yet.

Please let me know if there's additional tests that would be beneficial.

The changes are clear, and the generics are tested by the impl.
I don't see the need for additional tests.

We could try to avoid leaking the underlying Screen type by storing a &dyn Screen

I am happy with the current state, but I do invite you to sketch that a little more out in a comment, if you want - so we can look at it in a little more detail.
Otherwise I would be fine with the PR as is, as I already see it as an improvement.

@a-kenji a-kenji self-requested a review March 2, 2024 10:20
Copy link
Owner

@a-kenji a-kenji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,
pending the doc comments.

And if desired a discussion about: &dyn Screen

src/widget.rs Show resolved Hide resolved
@chris-olszewski
Copy link
Contributor Author

I am happy with the current state, but I do invite you to sketch that a little more out in a comment

To avoid needing a Cell associated type I think it would require moving the Cell to a methods up to Screen e.g.

trait Screen {
  // vt100 implementation would be `screen.cell(row, col).map(|cell| cell.has_contents())`
  fn cell_has_contents(&self, row: u16, col: u16) -> Option<bool>;
  fn apply_cell(&self, row: u16, col: u16, cell: &mut ratatui::buffer::Cell);
  ...
}

Feels less than ideal since that would require the Screen implementation to fetch the screen type twice with the current rendering implementation. That double fetch could be avoided if the implementation of apply_cell took ownership this logic, but I'm unsure if that's desired.

cell_has_contents would then only be used for rendering the cursor.

@a-kenji
Copy link
Owner

a-kenji commented Mar 6, 2024

Thank you for that sketch and explanation!
I have thought about it and I am also unsure if that would be desired.

Unless you have more input or ideas I would merge this PR, if you are happy with it.
If you want I can cut a release after the merge.

@chris-olszewski
Copy link
Contributor Author

I have thought about it and I am also unsure if that would be desired

I feel similarly

Unless you have more input or ideas I would merge this PR

I'm happy with the state of the PR

@a-kenji
Copy link
Owner

a-kenji commented Mar 6, 2024

Thank you :).

@a-kenji a-kenji merged commit 96da15f into a-kenji:development Mar 6, 2024
16 checks passed
chris-olszewski added a commit to vercel/turborepo that referenced this pull request Mar 12, 2024
### Description

In order to properly persist a task's output we need to display the full
terminal contents including whatever might be in the scrollback buffer.
This PR adds an alternative screen where the row index includes the
scrollback rows. The `Screen` trait added in
a-kenji/tui-term#152 will be implemented for
`EntireScreen` allowing for us to use this structure with the
`PseudoTerminal` widget.

### Testing Instructions

Added some basic unit tests to verify that scrollback gets printed.


Closes TURBO-2592
chris-olszewski added a commit to vercel/turborepo that referenced this pull request Mar 14, 2024
### Description

a-kenji/tui-term#152 allowed for the widget to
be used with any library that implements the `Screen` trait. This PR
switches us to use our vendored vt100 crate as the backend for the
terminal widget.

If desired should be easy to review each commit individually.

### Testing Instructions

`cargo run -p turborepo-ui --example pane` and verify that colors looks
nice


Closes TURBO-2623
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.

2 participants