a-kenji / tui-term

A pseudoterminal widget library for ratatui
MIT License
119 stars 7 forks source link

Looser coupling with vt100 #152

Closed chris-olszewski closed 7 months ago

chris-olszewski commented 7 months ago

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:

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 commented 7 months ago

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.

chris-olszewski commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

Thank you :).