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(backend): add window size api #276

Merged
merged 1 commit into from Aug 29, 2023
Merged

Conversation

benjajaja
Copy link
Contributor

@benjajaja benjajaja commented Jun 25, 2023

Depends on crossterm-rs/crossterm#790, and should update crossterm in an independent PR. Related to #215

For image (sixel, iTerm2, Kitty...) support that handles graphics in
terms of Rect so that the image area can be included in layouts.

For example: an image is loaded with a known pixel-size, and drawn, but
the image protocol has no mechanism of knowing the actual cell/character
area that been drawn on. It is then impossible to skip overdrawing the
area.

Returning the window size in pixel-width / pixel-height, together with
colums / rows, it can be possible to account the pixel size of each cell
/ character, and then known the Rect of a given image, and also resize
the image so that it fits exactly in a Rect and avoid artifacts.

image

@codecov
Copy link

codecov bot commented Jun 25, 2023

Codecov Report

Merging #276 (dbf0c5c) into main (ad3413e) will increase coverage by 0.36%.
Report is 6 commits behind head on main.
The diff coverage is 31.32%.

❗ Current head dbf0c5c differs from pull request most recent head 09589b7. Consider uploading reports for the commit 09589b7 to get more accurate results

@@            Coverage Diff             @@
##             main     #276      +/-   ##
==========================================
+ Coverage   89.25%   89.61%   +0.36%     
==========================================
  Files          40       40              
  Lines       10767    10939     +172     
==========================================
+ Hits         9610     9803     +193     
+ Misses       1157     1136      -21     
Files Changed Coverage Δ
src/backend/crossterm.rs 0.00% <0.00%> (ø)
src/backend/mod.rs 82.92% <ø> (ø)
src/backend/termion.rs 26.36% <0.00%> (-1.10%) ⬇️
src/backend/termwiz.rs 0.00% <0.00%> (ø)
src/backend/test.rs 95.25% <0.00%> (+52.01%) ⬆️
src/terminal.rs 58.30% <80.00%> (ø)
src/layout.rs 98.12% <85.71%> (-0.27%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Rewrite the commit message title in the convention commit style using imperative tone (e.g. feat(backend): add window_size_in_pixels() method).

This is pretty close to being mergable (or will be once the crossterm PR is merged), so can you please rebase and squash your commits into a single commit and setup commit signing? See https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits for info.

Cargo.toml Outdated Show resolved Hide resolved
examples/window_size.rs Outdated Show resolved Hide resolved
src/backend/crossterm.rs Outdated Show resolved Hide resolved
src/backend/mod.rs Outdated Show resolved Hide resolved
src/backend/termwiz.rs Outdated Show resolved Hide resolved
src/backend/test.rs Outdated Show resolved Hide resolved
@joshka
Copy link
Member

joshka commented Jun 26, 2023

Backend is woefully under-tested. I wonder if there's some way to fix that.

benjajaja added a commit to benjajaja/ratatui-image that referenced this pull request Jul 22, 2023
* `sixel-bytes` encodes to `String` instead of file.
* `serde` for `BackendType` for e.g. user configs.
* `Send + Sync` for threads/tokio.
* `rustix::termios` to get window/font size until ratatui/crossterm get `window_size()`:
  [`ratatui::Backend::window_size()`](ratatui-org/ratatui#276) needs [`crossterm::terminal::window_size()`](crossterm-rs/crossterm#790).
benjajaja added a commit to benjajaja/ratatui-image that referenced this pull request Jul 22, 2023
* `sixel-bytes` encodes to `String` instead of file.
* `serde` for `BackendType` for e.g. user configs.
* `Send + Sync` for threads/tokio.
* `rustix::termios` to get window/font size until ratatui/crossterm get `window_size()`:
  [`ratatui::Backend::window_size()`](ratatui-org/ratatui#276) needs [`crossterm::terminal::window_size()`](crossterm-rs/crossterm#790).
benjajaja added a commit to benjajaja/ratatui-image that referenced this pull request Jul 22, 2023
* `sixel-bytes` encodes to `String` instead of file.
* `serde` for `BackendType` for e.g. user configs.
* `Send + Sync` for threads/tokio.
* `rustix::termios` to get window/font size until ratatui/crossterm get `window_size()`:
  [`ratatui::Backend::window_size()`](ratatui-org/ratatui#276) needs [`crossterm::terminal::window_size()`](crossterm-rs/crossterm#790).
@benjajaja benjajaja force-pushed the window_size branch 2 times, most recently from 6aaf325 to 7cacfcc Compare August 6, 2023 20:32
@benjajaja benjajaja marked this pull request as ready for review August 6, 2023 21:44
@benjajaja benjajaja requested a review from joshka August 6, 2023 21:46
@benjajaja
Copy link
Contributor Author

benjajaja commented Aug 6, 2023

Regarding tests, I don't think it's really feasible to cover this. window_size() really depends on being run as a graphical application, i.e. xorg, wayland, maybe even fbdev, but AFAIK near impossible in docker / github runners.

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

I think there was a mention of this not yet working with windows on crossterm if I read the linked issue right. Can you make sure to document this in the crossterm docs for the window_size method?

src/backend/mod.rs Outdated Show resolved Hide resolved
@joshka
Copy link
Member

joshka commented Aug 7, 2023

BTW, another PR upgraded crossterm 0.27 and brought in the changes to the error bits. Can you rebase this on main please?

src/terminal.rs Outdated Show resolved Hide resolved
@benjajaja
Copy link
Contributor Author

I tried adding some test coverage for insert_before, but that did require to implement clear_region() and append_line() for TestBackend for which I don't know exactly how to implement the different "clear types", e.g. AfterCursor etc. In general, it seemed a bit pointless to test insert_before against TestBackend.

@benjajaja benjajaja requested a review from joshka August 19, 2023 08:55
@joshka joshka added this to the v0.23.0 milestone Aug 21, 2023
Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

I think perhaps make this non-breaking with the existing size() and resize() continuing to take Rects? We can come back to that decision at a later date however.

Aiming for a 0.23.0 release after the weekend, so this should be in that if you can make the changes in time.

src/layout.rs Outdated Show resolved Hide resolved
src/backend/crossterm.rs Outdated Show resolved Hide resolved
src/backend/mod.rs Outdated Show resolved Hide resolved
src/backend/mod.rs Outdated Show resolved Hide resolved
src/layout.rs Outdated Show resolved Hide resolved
@benjajaja benjajaja force-pushed the window_size branch 3 times, most recently from 5001d14 to 0a4b51f Compare August 26, 2023 09:13
@benjajaja
Copy link
Contributor Author

@joshka I toned this down to only adding the new window_resize() method, still introducing the new Size struct but not using it in src/terminal.rs so no breaking changes. Also dropped the truncation in Size copied from Rect, as its main purpose is to keep area (and maybe indirectly Buffer::content) in bounds, and there are no more conversions between Size and Rect now.

It's cool if we can get this into the release, but it's not critical like the cell skipping, as the window pixel size can be queried outside of ratatui.


Some thoughts on Size vs Rect in Terminal:

In Terminal, there is a last_known_size that may have non-zero x,y if the viewport is Viewport::Fixed(Rect). However it seems like the field is used exclusively by Viewport::Inline(u16).

In general, I think it could be healthy to cut down a few public methods in Terminal: autoresize, resize, size (is just a wrapper to backend.size()), and area of CompletedFrame returned by draw.

@benjajaja benjajaja requested a review from joshka August 26, 2023 11:10
Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

This looks good now. Thanks again. This seems like something we could merge before release. I'll flip it to the other maintainers to take a look.

Comments are non-blocking

src/backend/test.rs Show resolved Hide resolved
src/backend/mod.rs Outdated Show resolved Hide resolved
For image (sixel, iTerm2, Kitty...) support that handles graphics in
terms of `Rect` so that the image area can be included in layouts.

For example: an image is loaded with a known pixel-size, and drawn, but
the image protocol has no mechanism of knowing the actual cell/character
area that been drawn on. It is then impossible to skip overdrawing the
area.

Returning the window size in pixel-width / pixel-height, together with
colums / rows, it can be possible to account the pixel size of each cell
/ character, and then known the `Rect` of a given image, and also resize
the image so that it fits exactly in a `Rect`.

Crossterm and termwiz also both return both sizes from one syscall,
while termion does two.

Add a `Size` struct for the cases where a `Rect`'s `x`/`y` is unused
(always zero).

`Size` is not "clipped" for `area < u16::max_value()` like `Rect`. This
is why there are `From` implementations between the two.
@mindoodoo mindoodoo changed the title window size feat(backend): add window size api Aug 29, 2023
@mindoodoo mindoodoo modified the milestones: v0.23.0, v0.24.0 Aug 29, 2023
Copy link
Member

@mindoodoo mindoodoo left a comment

Choose a reason for hiding this comment

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

Looks good ! Thanks for this !

I'll have to take a look at the image crate soon ! Exciting stuff.

@joshka what do you think about the scope of the conventional commit PR title ? let me know if anything else would be more suited. Thanks for all the review on this !

@joshka
Copy link
Member

joshka commented Aug 29, 2023

@joshka what do you think about the scope of the conventional commit PR title ? let me know if anything else would be more suited. Thanks for all the review on this !

I think it's probably ok as is. Merging this now. Thanks again @benjajaja. This is going to lead to some fun apps.

@joshka joshka added this pull request to the merge queue Aug 29, 2023
Merged via the queue into ratatui-org:main with commit d077903 Aug 29, 2023
29 of 30 checks passed
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

4 participants