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.

Sign up for GitHub

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

Jump to bottom

feat(buffer): add Buffer::cell, cell_mut and index implementations #1084

Merged
merged 18 commits into from
Aug 6, 2024
Merged

feat(buffer): add Buffer::cell, cell_mut and index implementations #1084

merged 18 commits into from
Aug 6, 2024

Conversation

joshka
Copy link
Member

@joshka joshka commented May 3, 2024 •
edited
Loading

Code which previously called buf.get(x, y) or buf.get_mut(x, y)
should now use index operators, or be transitioned to buff.cell() or
buf.cell_mut() for safe access that avoids panics by returning
Option<&Cell> and Option<&mut Cell>.

The new methods accept Into<Position> instead of x and y
coordinates, which makes them more ergonomic to use.

let mut buffer = Buffer::empty(Rect::new(0, 0, 10, 10));

let cell = buf[(0, 0)];
let cell = buf[Position::new(0, 0)];

let symbol = buf.cell((0, 0)).map(|cell| cell.symbol());
let symbol = buf.cell(Position::new(0, 0)).map(|cell| cell.symbol());

buf[(0, 0)].set_symbol("🐀");
buf[Position::new(0, 0)].set_symbol("🐀");

buf.cell_mut((0, 0)).map(|cell| cell.set_symbol("🐀"));
buf.cell_mut(Position::new(0, 0)).map(|cell| cell.set_symbol("🐀"));

The existing get() and get_mut() methods are marked as deprecated.
These are fairly widely used and we will leave these methods around on
the buffer for a longer time than our normal deprecation approach (2
major release)

Addresses part of: #1011

@joshka
feat(buffer): add Buffer::get_opt, get_mut_opt and index implementations
33edfc3 
Code which previously called `buf.get(x, y)` or `buf.get_mut(x, y)` can
now use index operators, or be transitioned to `buf_get_opt` or
`buf.get_mut_opt` for safe access that avoids panics.

The new methods accept `Into<Position>` instead of `x` and `y`
coordinates, which makes them more ergonomic to use.

```rust
let mut buffer = Buffer::empty(Rect::new(0, 0, 10, 10));

let cell = buf[(0, 0)];
let cell = buf[Position::new(0, 0)];

let symbol = buf.get_opt((0, 0)).map(|cell| cell.symbol());
let symbol = buf.get_opt(Position::new(0, 0)).map(|cell| cell.symbol());

buf[(0, 0)].set_symbol("🐀");
buf[Position::new(0, 0)].set_symbol("🐀");

buf.get_mut_opt((0, 0)).map(|cell| cell.set_symbol("🐀"));
buf.get_mut_opt(Position::new(0, 0)).map(|cell| cell.set_symbol("🐀"));
```
@codecov Codecov
Copy link

codecov bot commented May 3, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.5%. Comparing base (f2fa1ae) to head (9fac1f0).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##            main   #1084     +/-   ##
=======================================
- Coverage   94.5%   94.5%   -0.1%     
=======================================
  Files         65      65             
  Lines      15431   15488     +57     
=======================================
+ Hits       14588   14637     +49     
- Misses       843     851      +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

EdJoPaTo
EdJoPaTo suggested changes May 3, 2024
View reviewed changes
Copy link
Contributor

@EdJoPaTo EdJoPaTo left a comment

Choose a reason for hiding this comment

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

Is there a reason why this was not done with pos_of too?

#1049: https://github.com/ratatui-org/ratatui/blob/30a6fb2e1335d408547f04d0c1a0f527d578e647/src/buffer/buffer.rs#L241-L250

src/buffer/buffer.rs Show resolved Hide resolved
src/buffer/buffer.rs Outdated Show resolved Hide resolved
src/buffer/buffer.rs Show resolved Hide resolved
src/buffer/buffer.rs Show resolved Hide resolved
@joshka
Copy link
Member Author

joshka commented May 3, 2024

Is there a reason why this was not done with pos_of too?

I don't think pos_of should be part of the API, and I'd prefer not to make it worse. (note the new index_of_opt method is private)

@joshka joshka mentioned this pull request May 19, 2024
Closed
kdheepak
kdheepak reviewed May 19, 2024
View reviewed changes
src/buffer/buffer.rs Outdated Show resolved Hide resolved
@joshka joshka mentioned this pull request May 21, 2024
Open
@joshka joshka mentioned this pull request May 21, 2024
Closed
@kdheepak
Copy link
Collaborator

This looks good to me but maybe we can get more reviewers on this?

PR title + description should be updated.

EdJoPaTo
EdJoPaTo reviewed May 22, 2024
View reviewed changes
Copy link
Contributor

@EdJoPaTo EdJoPaTo left a comment

Choose a reason for hiding this comment

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

I like the usage of the Index trait. It is still panicking like before but should have the same performance.

src/buffer/buffer.rs Outdated Show resolved Hide resolved
src/buffer/buffer.rs Outdated Show resolved Hide resolved
src/buffer/buffer.rs Outdated Show resolved Hide resolved
@joshka @EdJoPaTo
Update buffer.rs
9f70408 
Co-authored-by: EdJoPaTo <rfc-conform-git-commit-email@funny-long-domain-label-everyone-hates-as-it-is-too-long.edjopato.de>
@joshka
Copy link
Member Author

joshka commented May 22, 2024

I actually intended to put the deprecation of the existing methods in #1123, but accidentally pushed the commit to this PR. Happy to do it either way (though it would be easier at this point to just keep it in this PR and update the main PR comment.

@joshka joshka requested review from EdJoPaTo and kdheepak May 25, 2024 23:29
EdJoPaTo
EdJoPaTo approved these changes Aug 2, 2024
View reviewed changes
src/buffer/buffer.rs Outdated Show resolved Hide resolved
src/buffer/buffer.rs Outdated Show resolved Hide resolved
@joshka joshka requested a review from a team as a code owner August 6, 2024 07:19
@joshka joshka changed the title feat(buffer): add Buffer::get_opt, get_mut_opt and index implementations feat(buffer): add Buffer::cell, cell_mut and index implementations Aug 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 6, 2024 •
edited
Loading

🐰Bencher

ReportTue, August 6, 2024 at 07:55:38 UTC
ProjectRatatui
Branch1084/merge
Testbedubuntu-latest

⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!

  • Latency (latency)

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds CLI flag.

Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns)
barchart/render/2048➖ (view plot)193,650.00
barchart/render/256➖ (view plot)124,590.00
barchart/render/64➖ (view plot)85,868.00
barchart/render_grouped/2048➖ (view plot)341,480.00
barchart/render_grouped/256➖ (view plot)136,000.00
barchart/render_grouped/64➖ (view plot)123,830.00
barchart/render_horizontal/2048➖ (view plot)163,870.00
barchart/render_horizontal/256➖ (view plot)81,185.00
barchart/render_horizontal/64➖ (view plot)74,361.00
block/render_all_feature/100x50➖ (view plot)10,007.00
block/render_all_feature/200x50➖ (view plot)17,854.00
block/render_all_feature/256x256➖ (view plot)85,222.00
block/render_empty/100x50➖ (view plot)5,634.70
block/render_empty/200x50➖ (view plot)11,033.00
block/render_empty/256x256➖ (view plot)71,559.00
line_render/Center/0➖ (view plot)2.78
line_render/Center/10➖ (view plot)416.28
line_render/Center/3➖ (view plot)211.76
line_render/Center/4➖ (view plot)235.35
line_render/Center/42➖ (view plot)565.39
line_render/Center/6➖ (view plot)253.62
line_render/Center/7➖ (view plot)286.40
line_render/Left/0➖ (view plot)2.79
line_render/Left/10➖ (view plot)367.82
line_render/Left/3➖ (view plot)145.13
line_render/Left/4➖ (view plot)155.36
line_render/Left/42➖ (view plot)565.26
line_render/Left/6➖ (view plot)241.07
line_render/Left/7➖ (view plot)251.96
line_render/Right/0➖ (view plot)2.78
line_render/Right/10➖ (view plot)371.16
line_render/Right/3➖ (view plot)210.61
line_render/Right/4➖ (view plot)248.19
line_render/Right/42➖ (view plot)564.37
line_render/Right/6➖ (view plot)324.00
line_render/Right/7➖ (view plot)365.19
list/render/16384➖ (view plot)1,151,700.00
list/render/2048➖ (view plot)261,800.00
list/render/64➖ (view plot)144,050.00
list/render_scroll_half/16384➖ (view plot)1,150,700.00
list/render_scroll_half/2048➖ (view plot)265,220.00
list/render_scroll_half/64➖ (view plot)96,458.00
paragraph/new/2048➖ (view plot)247,650.00
paragraph/new/64➖ (view plot)6,025.20
paragraph/new/65535➖ (view plot)13,303,000.00
paragraph/render/2048➖ (view plot)435,960.00
paragraph/render/64➖ (view plot)401,200.00
paragraph/render/65535➖ (view plot)1,521,200.00
paragraph/render_scroll_full/2048➖ (view plot)381,030.00
paragraph/render_scroll_full/64➖ (view plot)417,650.00
paragraph/render_scroll_full/65535➖ (view plot)1,489,200.00
paragraph/render_scroll_half/2048➖ (view plot)381,190.00
paragraph/render_scroll_half/64➖ (view plot)425,310.00
paragraph/render_scroll_half/65535➖ (view plot)1,487,000.00
paragraph/render_wrap/2048➖ (view plot)223,110.00
paragraph/render_wrap/64➖ (view plot)198,870.00
paragraph/render_wrap/65535➖ (view plot)1,424,400.00
paragraph/render_wrap_scroll_full/2048➖ (view plot)222,570.00
paragraph/render_wrap_scroll_full/64➖ (view plot)197,150.00
paragraph/render_wrap_scroll_full/65535➖ (view plot)1,421,900.00
sparkline/render/2048➖ (view plot)123,540.00
sparkline/render/256➖ (view plot)118,500.00
sparkline/render/64➖ (view plot)37,847.00

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@joshka joshka merged commit a23ecd9 into main Aug 6, 2024
46 of 47 checks passed
@joshka joshka deleted the jm/buffer_opt branch August 6, 2024 07:40
joshka added a commit to erak/ratatui that referenced this pull request Oct 14, 2024
@joshka @EdJoPaTo
feat(buffer): add Buffer::cell, cell_mut and index implementations (r…
05919a5 
…atatui#1084)

Code which previously called `buf.get(x, y)` or `buf.get_mut(x, y)`
should now use index operators, or be transitioned to `buff.cell()` or
`buf.cell_mut()` for safe access that avoids panics by returning
`Option<&Cell>` and `Option<&mut Cell>`.

The new methods accept `Into<Position>` instead of `x` and `y`
coordinates, which makes them more ergonomic to use.

```rust
let mut buffer = Buffer::empty(Rect::new(0, 0, 10, 10));

let cell = buf[(0, 0)];
let cell = buf[Position::new(0, 0)];

let symbol = buf.cell((0, 0)).map(|cell| cell.symbol());
let symbol = buf.cell(Position::new(0, 0)).map(|cell| cell.symbol());

buf[(0, 0)].set_symbol("🐀");
buf[Position::new(0, 0)].set_symbol("🐀");

buf.cell_mut((0, 0)).map(|cell| cell.set_symbol("🐀"));
buf.cell_mut(Position::new(0, 0)).map(|cell| cell.set_symbol("🐀"));
```

The existing `get()` and `get_mut()` methods are marked as deprecated.
These are fairly widely used and we will leave these methods around on
the buffer for a longer time than our normal deprecation approach (2
major release)

Addresses part of: ratatui#1011

---------

Co-authored-by: EdJoPaTo <rfc-conform-git-commit-email@funny-long-domain-label-everyone-hates-as-it-is-too-long.edjopato.de>
@joshka joshka mentioned this pull request Nov 30, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EdJoPaTo EdJoPaTo EdJoPaTo approved these changes

@orhun orhun Awaiting requested review from orhun

@Valentin271 Valentin271 Awaiting requested review from Valentin271

@kdheepak kdheepak Awaiting requested review from kdheepak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joshka @kdheepak @EdJoPaTo