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

refactor(text): simplify reflow implementation #290

Merged
merged 2 commits into from
Jul 17, 2023

Conversation

joshka
Copy link
Member

@joshka joshka commented Jul 2, 2023

This is a PR that extracts the refactor part of #259, as well as the code for Line->StyledGrapheme. It's intent is to make the review of the actual wrapping code easier. It contains the following commits from 259:

refactor(text): split text::* into separate files (47f8c81)

  • cf4f0ed: refactor(text): reorganize text module items into their own files
  • does not include wrap.rs

feat(text): expose graphemes on Line (ec411bb)

  • add Line::styled()
  • add Line::styled_graphemes()
  • add StyledGrapheme::new()

which contains

  • 54aac91: feat(text): add Line::styled() and Line.styled_graphemes()
  • c1f3cf8: chore: add doc comment description to StyledGrapheme
  • 66706c6: fix: remediate lifetime and Cow issues in Line

and adds:

Review approach

The new code is called out above - no need to review the entire PR, just the bits called out.
Pinging @lthoerner for 👀
The rest of #289 will be rebased on top of this, but this part should be merged quickly to avoid more drift.

@joshka
Copy link
Member Author

joshka commented Jul 2, 2023

Formatting issue is unrelated to this - seems like a slight change in the current nightly toolchain

@codecov
Copy link

codecov bot commented Jul 2, 2023

Codecov Report

Merging #290 (72a9128) into main (b347201) will decrease coverage by 2.12%.
The diff coverage is 72.28%.

❗ Current head 72a9128 differs from pull request most recent head 8dbc44c. Consider uploading reports for the commit 8dbc44c to get more accurate results

@@            Coverage Diff             @@
##             main     #290      +/-   ##
==========================================
- Coverage   85.00%   82.88%   -2.12%     
==========================================
  Files          38       39       +1     
  Lines        8507     7532     -975     
==========================================
- Hits         7231     6243     -988     
- Misses       1276     1289      +13     
Impacted Files Coverage Δ
src/text/grapheme.rs 36.36% <36.36%> (ø)
src/widgets/canvas/line.rs 59.67% <50.00%> (ø)
src/widgets/canvas/mod.rs 91.88% <50.00%> (ø)
src/text/text.rs 52.05% <52.05%> (ø)
src/text/line.rs 96.55% <86.04%> (-3.45%) ⬇️
src/text/span.rs 100.00% <100.00%> (ø)

... and 20 files with indirect coverage changes

@joshka joshka mentioned this pull request Jul 2, 2023
6 tasks
@joshka joshka changed the title reflow rebase refactor: reflow Jul 3, 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.

Needs to be rebased, otherwise lgtm, thanks @joshka for extracting this and thanks @lthoerner for taking this much needed task on !

@joshka
Copy link
Member Author

joshka commented Jul 15, 2023

This is @lthoerner's work. I just git-monkied it. Will rebase and merge soon. Likely after the release to give it some time to bake.

- add `Line::styled()`
- add `Line::styled_graphemes()`
- add `StyledGrapheme::new()`
@joshka
Copy link
Member Author

joshka commented Jul 15, 2023

rebased on main - requires re-approval as there was a conflict in the rebase in comments in text.rs for Span (which are then changed in the second commit), and in the set_style() method in text.rs for Span.

@orhun
Copy link
Sponsor Member

orhun commented Jul 16, 2023

I recommend updating the commit message to something like refactor(text): simplify reflow implementation or something like that before merging.

@joshka joshka changed the title refactor: reflow refactor(text): simplify reflow implementation Jul 17, 2023
@joshka joshka added this pull request to the merge queue Jul 17, 2023
Merged via the queue into ratatui-org:main with commit fb6d4b2 Jul 17, 2023
15 of 17 checks passed
@joshka joshka deleted the reflow-rebase branch July 17, 2023 06:28
@lthoerner
Copy link
Contributor

I'm very confused what's going on with this. Did we only merge the text module refactor? As far as I am aware, the wrap implementation does not seem to be included in the merge, nor was it completed by me prior to going on hiatus from the project. I implemented only truncation and char-boundary wrapping.

@joshka
Copy link
Member Author

joshka commented Jul 18, 2023

yes - the goal on this was to cut down the size of the diff in #259 to something more reviewable. I'd want to take a second step that's similar - find some parts of the rest that are easy to see are useful steps along the way to the implementation that help someone review a part at a time rather than 2000+ lines of churn. https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/ is a helpful article on why.

@lthoerner
Copy link
Contributor

Should we update the changelog and the title of the PR then?

@joshka
Copy link
Member Author

joshka commented Jul 20, 2023

The changelog is automatically updated as part of the release process generally.
This PR is merged so changing anything on it won't have any meaningful effect.

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