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

docs(examples): simplify the barchart example #1079

Merged
merged 23 commits into from
Aug 6, 2024

Conversation

joshka
Copy link
Member

@joshka joshka commented May 2, 2024

The barchart example has been split into two examples: barchart and
barchart-grouped. The barchart example now shows a simple barchart
with random data, while the barchart-grouped example shows a grouped
barchart with fake revenue data.

This simplifies the examples a bit so they don't cover too much at once.

  • Simplify the rendering functions
  • Fix several clippy lints that were marked as allowed

@joshka
Copy link
Member Author

joshka commented May 15, 2024

I made a bunch of changes:

  • removed the prelude
  • revamped the way the data for the companies was stored to simplify the conversion into bars (i.e. it's stored as months containing revenues for each company rather than companies containing revenues for each month
  • moved a bunch of things around
  • tightened up pretty much everything
  • added comments where they would normally be needed.

@joshka joshka requested a review from EdJoPaTo May 15, 2024 07:53
@joshka
Copy link
Member Author

joshka commented May 15, 2024

The idea behind this is to do similar changes to all the examples, and to make them take this form.

@joshka
Copy link
Member Author

joshka commented May 16, 2024

fix: tweaks

  • Move constants closer to their usage
  • add an exit field to app
  • extract methods from the main loop to make the flow clearer
  • move update handling to use a field on app instead of a variable in the loop
  • rename last_tick to last_update to make it clearer what it controls

@EdJoPaTo
Copy link
Contributor

Given that there are other Repos with widget examples… how about a ratatui feature for an example base? Other widgets in other repos can also use them as a common base to demonstrate their widgets then.

@joshka
Copy link
Member Author

joshka commented May 16, 2024

Given that there are other Repos with widget examples… how about a ratatui feature for an example base? Other widgets in other repos can also use them as a common base to demonstrate their widgets then.

An example is just an app.
Adding code to make writing examples easy is adding code to make writing apps easy.
There shouldn't be an "example base", there should be functions that make using common parts of ratatui easy. Those changes are more difficult than cleaning up existing code to make it easier for new users to understand.

@EdJoPaTo
Copy link
Contributor

Given that there are other Repos with widget examples… how about a ratatui feature for an example base? Other widgets in other repos can also use them as a common base to demonstrate their widgets then.

An example is just an app. Adding code to make writing examples easy is adding code to make writing apps easy. There shouldn't be an "example base", there should be functions that make using common parts of ratatui easy. Those changes are more difficult than cleaning up existing code to make it easier for new users to understand.

This creates a common example base. My point is to make this common.rs reusable for other repos too, not just examples within the ratatui repo. That way they can also be more consistent.

@joshka
Copy link
Member Author

joshka commented May 16, 2024

I think it might be better to incorporate that file in each example instead as it makes each example self contained as a single file.

Publishing public code locks you into supporting it and not making changes that would break code that uses it. I'd prefer this code to be copied instead of referenced.

@joshka joshka requested a review from EdJoPaTo May 25, 2024 23:26
joshka added 8 commits May 29, 2024 04:47
- Simplify the rendering functions
- Fix several clippy lints that were marked as allowed
- Extract a common.rs for terminal initialization and error handling
Move constants closer to their usage
add an exit field to app
extract methods from the main loop to make the flow clearer
move update handling to use a field on app instead of a variable in the loop
rename last_tick to last_update to make it clearer what it controls
@joshka joshka force-pushed the jm/simplify-barchart-example branch from f241d60 to 77f8183 Compare May 29, 2024 11:59
joshka and others added 2 commits May 29, 2024 05:31
Co-authored-by: EdJoPaTo <rfc-conform-git-commit-email@funny-long-domain-label-everyone-hates-as-it-is-too-long.edjopato.de>
@joshka joshka requested a review from EdJoPaTo May 30, 2024 00:15
@joshka joshka force-pushed the jm/simplify-barchart-example branch from 57375fa to b2ab9a7 Compare July 17, 2024 17:48
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.5%. Comparing base (a23ecd9) to head (3a0bd95).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1079   +/-   ##
=====================================
  Coverage   94.5%   94.5%           
=====================================
  Files         65      65           
  Lines      15488   15488           
=====================================
  Hits       14637   14637           
  Misses       851     851           

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

Copy link
Collaborator

@kdheepak kdheepak left a comment

Choose a reason for hiding this comment

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

Looks much nicer to me!

@joshka
Copy link
Member Author

joshka commented Jul 17, 2024

In the latest I simplified this a bunch more.

  • Removed the Widget implementation from App and replaced it with just a plain render method that accepts Frame. This means that there's no need to split into two impl App blocks (which confused at least one newer rustacean)
  • renamed common to terminal
  • moved the color_eyre initialization into the terminal::init method
  • removed all the animation (the animation demonstrates how to handle updates in ratatui apps, rather than just the widget)
  • moved things around in layout / color etc.
  • removed the legend (to avoid implying that the legend was part of the widget)

@EdJoPaTo aside from the color_eyre parts on this (which I'd suggest we agree to disagree on and move past), does this seem reasonable? I'd like to move forward with generally bringing each example up to scratch here using this pattrn. Once we work out #1180 and #1181, the terminal init and error handling parts can easily be replaced with whatever that comes up with, but those are a longer conversations that are currently stalled. Those concerns shouldn't block making the examples be consistent and useful for users.

@joshka
Copy link
Member Author

joshka commented Jul 17, 2024

Small number tweak to avoid the 12.5M label looking weird

barchart

@joshka joshka added Type: Documentation Improvements or additions to documentation Status: Review Needed PR needs a review / Issue needs buy-in from other maintainers / users labels Jul 17, 2024
@joshka joshka force-pushed the jm/simplify-barchart-example branch from 0d6850d to 26df82c Compare July 17, 2024 19:44
@joshka
Copy link
Member Author

joshka commented Jul 17, 2024

Split in two based on conversation with @kdheepak
barchart
barchart-grouped

The `barchart` example has been split into two examples: `barchart` and
`barchart-grouped`. The `barchart` example now shows a simple barchart
with random data, while the `barchart-grouped` example shows a grouped
barchart with fake revenue data.

This simplifies the examples a bit so they don't cover too much at once.
@joshka joshka force-pushed the jm/simplify-barchart-example branch 2 times, most recently from 5dd5f60 to d666054 Compare July 17, 2024 19:56
@joshka joshka force-pushed the jm/simplify-barchart-example branch from d666054 to 02a5dd0 Compare July 17, 2024 20:28
@joshka joshka requested a review from a team as a code owner August 6, 2024 07:53
@joshka
Copy link
Member Author

joshka commented Aug 6, 2024

Merging this as it's better, as it simplifies the main logic. Whether it's "best" is up for debate, but it's a demo of approximately what #1289 would look like (but with the methods in an example module)

Copy link
Contributor

github-actions bot commented Aug 6, 2024

🐰Bencher

ReportTue, August 6, 2024 at 08:18:07 UTC
ProjectRatatui
Branch1079/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)192,800.00
barchart/render/256➖ (view plot)124,560.00
barchart/render/64➖ (view plot)85,846.00
barchart/render_grouped/2048➖ (view plot)341,270.00
barchart/render_grouped/256➖ (view plot)135,130.00
barchart/render_grouped/64➖ (view plot)123,080.00
barchart/render_horizontal/2048➖ (view plot)162,810.00
barchart/render_horizontal/256➖ (view plot)80,155.00
barchart/render_horizontal/64➖ (view plot)74,311.00
block/render_all_feature/100x50➖ (view plot)10,173.00
block/render_all_feature/200x50➖ (view plot)17,885.00
block/render_all_feature/256x256➖ (view plot)85,300.00
block/render_empty/100x50➖ (view plot)5,717.00
block/render_empty/200x50➖ (view plot)11,095.00
block/render_empty/256x256➖ (view plot)71,570.00
line_render/Center/0➖ (view plot)2.78
line_render/Center/10➖ (view plot)420.69
line_render/Center/3➖ (view plot)213.10
line_render/Center/4➖ (view plot)237.04
line_render/Center/42➖ (view plot)571.58
line_render/Center/6➖ (view plot)257.72
line_render/Center/7➖ (view plot)291.14
line_render/Left/0➖ (view plot)2.78
line_render/Left/10➖ (view plot)374.14
line_render/Left/3➖ (view plot)145.36
line_render/Left/4➖ (view plot)156.71
line_render/Left/42➖ (view plot)571.38
line_render/Left/6➖ (view plot)243.90
line_render/Left/7➖ (view plot)255.88
line_render/Right/0➖ (view plot)2.78
line_render/Right/10➖ (view plot)373.85
line_render/Right/3➖ (view plot)211.36
line_render/Right/4➖ (view plot)249.42
line_render/Right/42➖ (view plot)571.95
line_render/Right/6➖ (view plot)329.36
line_render/Right/7➖ (view plot)365.75
list/render/16384➖ (view plot)1,161,400.00
list/render/2048➖ (view plot)263,170.00
list/render/64➖ (view plot)143,750.00
list/render_scroll_half/16384➖ (view plot)1,154,500.00
list/render_scroll_half/2048➖ (view plot)267,640.00
list/render_scroll_half/64➖ (view plot)98,230.00
paragraph/new/2048➖ (view plot)250,920.00
paragraph/new/64➖ (view plot)6,169.80
paragraph/new/65535➖ (view plot)14,989,000.00
paragraph/render/2048➖ (view plot)440,800.00
paragraph/render/64➖ (view plot)400,540.00
paragraph/render/65535➖ (view plot)1,410,300.00
paragraph/render_scroll_full/2048➖ (view plot)390,620.00
paragraph/render_scroll_full/64➖ (view plot)430,750.00
paragraph/render_scroll_full/65535➖ (view plot)1,374,000.00
paragraph/render_scroll_half/2048➖ (view plot)392,630.00
paragraph/render_scroll_half/64➖ (view plot)439,210.00
paragraph/render_scroll_half/65535➖ (view plot)1,375,700.00
paragraph/render_wrap/2048➖ (view plot)240,880.00
paragraph/render_wrap/64➖ (view plot)191,260.00
paragraph/render_wrap/65535➖ (view plot)1,419,900.00
paragraph/render_wrap_scroll_full/2048➖ (view plot)240,290.00
paragraph/render_wrap_scroll_full/64➖ (view plot)191,680.00
paragraph/render_wrap_scroll_full/65535➖ (view plot)1,409,500.00
sparkline/render/2048➖ (view plot)121,590.00
sparkline/render/256➖ (view plot)118,800.00
sparkline/render/64➖ (view plot)37,855.00

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

@joshka joshka merged commit fe4eeab into main Aug 6, 2024
38 of 39 checks passed
@joshka joshka deleted the jm/simplify-barchart-example branch August 6, 2024 08:11
joshka added a commit to erak/ratatui that referenced this pull request Oct 14, 2024
The `barchart` example has been split into two examples: `barchart` and
`barchart-grouped`. The `barchart` example now shows a simple barchart
with random data, while the `barchart-grouped` example shows a grouped
barchart with fake revenue data.

This simplifies the examples a bit so they don't cover too much at once.

- Simplify the rendering functions
- Fix several clippy lints that were marked as allowed

---------

Co-authored-by: EdJoPaTo <rfc-conform-git-commit-email@funny-long-domain-label-everyone-hates-as-it-is-too-long.edjopato.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed PR needs a review / Issue needs buy-in from other maintainers / users Type: Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants