feat(backend): add window size api #276

benjajaja · 2023-06-25T09:56:25Z

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.

codecov · 2023-06-25T09:59:57Z

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

joshka

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

examples/window_size.rs

src/backend/crossterm.rs

src/backend/mod.rs

src/backend/termwiz.rs

src/backend/test.rs

joshka · 2023-06-26T00:43:07Z

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

* `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/ratatui#276) needs [`crossterm::terminal::window_size()`](crossterm-rs/crossterm#790).

benjajaja · 2023-08-06T21:47:45Z

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.

joshka

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

joshka · 2023-08-07T21:47:34Z

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

benjajaja · 2023-08-19T08:54:42Z

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.

joshka

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

src/backend/crossterm.rs

src/backend/mod.rs

src/layout.rs

benjajaja · 2023-08-26T11:10:05Z

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

joshka

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

src/backend/mod.rs

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

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 · 2023-08-29T04:45:58Z

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

benjajaja force-pushed the window_size branch from 4893685 to 3c30c22 Compare June 25, 2023 09:59

joshka requested changes Jun 26, 2023

View reviewed changes

benjajaja force-pushed the window_size branch 2 times, most recently from 6aaf325 to 7cacfcc Compare August 6, 2023 20:32

benjajaja marked this pull request as ready for review August 6, 2023 21:44

benjajaja requested review from orhun, mindoodoo, sayanarijit, sophacles and kdheepak as code owners August 6, 2023 21:44

benjajaja requested a review from joshka August 6, 2023 21:46

joshka requested changes Aug 7, 2023

View reviewed changes

src/backend/mod.rs Outdated Show resolved Hide resolved

benjajaja force-pushed the window_size branch from 7cacfcc to 1ec1ca9 Compare August 7, 2023 07:11

benjajaja force-pushed the window_size branch 4 times, most recently from 8601a65 to 5bb8543 Compare August 12, 2023 19:26

benjajaja force-pushed the window_size branch from 5bb8543 to c7faa78 Compare August 19, 2023 08:40

benjajaja commented Aug 19, 2023

View reviewed changes

src/terminal.rs Outdated Show resolved Hide resolved

benjajaja requested a review from joshka August 19, 2023 08:55

joshka added this to the v0.23.0 milestone Aug 21, 2023

joshka requested changes Aug 23, 2023

View reviewed changes

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 force-pushed the window_size branch 3 times, most recently from 5001d14 to 0a4b51f Compare August 26, 2023 09:13

benjajaja requested a review from joshka August 26, 2023 11:10

joshka approved these changes Aug 26, 2023

View reviewed changes

src/backend/test.rs Show resolved Hide resolved

src/backend/mod.rs Outdated Show resolved Hide resolved

benjajaja force-pushed the window_size branch from 0a4b51f to a9caa12 Compare August 27, 2023 12:50

benjajaja force-pushed the window_size branch from a9caa12 to 09589b7 Compare August 27, 2023 12:54

mindoodoo changed the title ~~window size~~ feat(backend): add window size api Aug 29, 2023

mindoodoo modified the milestones: v0.23.0, v0.24.0 Aug 29, 2023

mindoodoo approved these changes Aug 29, 2023

View reviewed changes

joshka added this pull request to the merge queue Aug 29, 2023

Merged via the queue into ratatui:main with commit d077903 Aug 29, 2023
29 of 30 checks passed

benjajaja mentioned this pull request Oct 12, 2023

Add support for previewing images in room scrollback ulyssa/iamb#108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backend): add window size api #276

feat(backend): add window size api #276

benjajaja commented Jun 25, 2023 •

edited

Loading

codecov bot commented Jun 25, 2023 •

edited

Loading

joshka left a comment

joshka commented Jun 26, 2023

benjajaja commented Aug 6, 2023 •

edited

Loading

joshka left a comment

joshka commented Aug 7, 2023

benjajaja commented Aug 19, 2023

joshka left a comment

benjajaja commented Aug 26, 2023

joshka left a comment •

edited

Loading

mindoodoo left a comment

joshka commented Aug 29, 2023

feat(backend): add window size api #276

feat(backend): add window size api #276

Conversation

benjajaja commented Jun 25, 2023 • edited Loading

codecov bot commented Jun 25, 2023 • edited Loading

Codecov Report

joshka left a comment

Choose a reason for hiding this comment

joshka commented Jun 26, 2023

benjajaja commented Aug 6, 2023 • edited Loading

joshka left a comment

Choose a reason for hiding this comment

joshka commented Aug 7, 2023

benjajaja commented Aug 19, 2023

joshka left a comment

Choose a reason for hiding this comment

benjajaja commented Aug 26, 2023

joshka left a comment • edited Loading

Choose a reason for hiding this comment

mindoodoo left a comment

Choose a reason for hiding this comment

joshka commented Aug 29, 2023

benjajaja commented Jun 25, 2023 •

edited

Loading

codecov bot commented Jun 25, 2023 •

edited

Loading

benjajaja commented Aug 6, 2023 •

edited

Loading

joshka left a comment •

edited

Loading