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

Split scrollable viewports #121

Closed
wants to merge 34 commits into from
Closed

Conversation

gibbz00
Copy link

@gibbz00 gibbz00 commented Mar 28, 2023

Basically pt. 2 of #119.

asciicast

Most of the same things apply as described in the first PR.

Implementation and usage

Viewport can now be split by using the Terminal::new_split(constraints) constructor where the constraints are those from layout.rs. Each constraint results in one viewport, meaning that two imply one split. The viewports can then be scrolled by calling terminal.split_viewport_scroll(|index| -> (x_step, y_step)). This takes a closure that returns the scroll for a given split viewport index. The index is set by the index of the respective constraint used when calling the terminal constructor.

let mut terminal = Terminal::new_split(
    backend,
    vec![Constraint::Length(3), Constraint::Min(0)],
    Direction::Vertical,
)?;

/// ...draw UI and what not.

terminal.split_viewport_scroll(|index| match index {
    1 => (0, 1),
    _ => (0, 0),
})?
.unwrap_or(())

unwrap_or(()) represents the action of skipping viewport over scroll attempts. This includes both split viewport overlapping (disallowed), and when attempting to scroll beyond the buffer. viewport_scroll() explains this in more detail.

Also, calling split_viewport() will only update the region with the updated scroll. All other regions are left untouched.

Introduces the enum `Extend` with the variants `Viewport`, `Overflow`, `None`

(Also moved some variable initialization closer to their first use.)
* also adds some temporary explanatory comments
* Remove offset property from buffer.
* Move diffing to Terminal so that only the part of the buffer that
  the viewport overlays is diffed.
* Use interior mutability for backend.buffer.
* buffer.merge() is now O(n) and not O(2n^2).
* protect against overflow behavior when calculating `Rect.size()`.
* do not expose the buffer directly from the terminal with `current_buffer_mut`.
* added a fill widget, like the Clear widget but a bit more general.
* don't call reset() between each call.
* decouple drawing and setting cursor position. Creating runtime issues with borrow_mut and crossterm.
  Better to use terminal.set_position()
Backend buffers are flushed in their entirety, diffing two frontend becomes
adds a lot of complexity that gets buggy very quickly with very questionable
performance improvements, if not the opposite.
Removes the Frame struct and the closure requirement. Reasons:
1) Required a lot of duplicate associated methods in Frame such as
```
  Frame::get_cursor() -> _ {
    self.terminal.get_cursor()
  }
```
2) Had the limitation of not being able to use ? in said draw closures.

3) Makes the terminal API similar to how others write buffers are
controlled with write and flush. Gives in the end the user a better
understanding of what operations do what.
```
  terminal.render_widget(..)
  terminal.flush()
```
vs.
```
  terminal.draw(|f| {
    frame.render_widget(..)
  })
```
(No need to manually think about clearing and resizing.)

4) Makes it possible implement a cleaner `set_cursor` workaround.
Only used for redundant testing, at least in the library. Those
wishing to get an insight of the current state of the buffer can use
`terminal.get_buffer()` instead.
* Removes the copy derive from `Rect`, makes it way to easy implicitly
"clone" the `Rect` and faulty code in doing so.
* adds more stringent viewport overflow checks
* make sure regions are properly cleared
* add clean up split viewport demo (testing with hjkl keys.)
In order to use internal viewport regions other that the first when
the user does not have access to the layouts.
@gibbz00 gibbz00 marked this pull request as draft March 29, 2023 10:32
Allows users to take control for more fine-grained flushing.
Makes it easier to clear a specific region when the user does not have
direct access to the region itself, only the viewport index.
@mindoodoo mindoodoo added the Type: Enhancement New feature or request label Mar 29, 2023
@mindoodoo
Copy link
Member

Yooooo, sorry for the delay in getting back to you.

We had a chat with the maintainers a couple days ago where we discussed some open PRs, including your two open PR.
First of all, the consensus was that your feature was pretty sick, although we couldn't come up with many use cases off the top of our heads, it does seem quite promising and we're sure that if the scrollable viewport made it into the crate people would find very cool uses for it.

However, we can't really review the PRs under their current form. There are way more files modified than needed for this feature, and a lot of the modifications are also irrelevant to the feature you're adding. With that in mind, could you please create a new PR only containing code that is relevant to the feature ?

@gibbz00
Copy link
Author

gibbz00 commented Apr 19, 2023

Hii 😁

Apologies on my end, I was the one that was supposed to reach out when time allowed.

Thanks nonetheless for looking into it. The reason I developed it was because I was writing a markdown reader. The idea was that there would be a fixed tab line at the top, and where the markdown view used many of the built-in widgets. (Tables, paragraphs etc...) Scrolling up and down required for all widgets to move in unison. I've now made the repo public if you're interested in watching how Ratatui works in the context of the two PRs. https://github.com/gibbz00/grow. It is by no means a finished state, but it does achieve this concept if I remember correctly.

I completely understand the difficulty in reviewing the drafts in their current state. Making them reviewable that can realistically only be achieved by creating a series of split up PRs each containing a small subset of cherry-picked/reworked commits.

The reason as to why I haven't heard back, is that I've lately been fully occupied with work. I can make an attempt at beginning to clean up the drafts on Sunday, but use otherwise these drafts as sample references for things that could be changed in the framework.

All the best,

@mindoodoo
Copy link
Member

Yeah no rush, whenever you get around to it we'll be ready to merge it !

You mentioned splitting it up in multiple PRs again, feel free to chat us on discord in order to potentially discuss what to split in what PRs. The goal being to avoid ending up in a situation where we have to ask you for more major edits.

I'll close both draft PRs for the time being, if you need more input on this feature I think you should potentially create an issue where other users might also be able to give you their input on the feature.

We appreciate the work you poured into this and are looking forward to seing the revamped PR(s) 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants