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(block): support for having more than one title #232

Merged
merged 1 commit into from
Jun 19, 2023

Conversation

samyosm
Copy link
Contributor

@samyosm samyosm commented Jun 9, 2023

To be able to create TUIs such as that of btop, I believe it to be necessary to be able to have multiple titles for a single block. This pull request adds the ability to have multiple titles per block. Each title can have a different alignment thought multiple titles can have the same alignment.

For backwards compatibility sake, I let the old title system and just added a new one that takes precedence over the old one in cases it has been set.

In the new system, you declare your titles in the following ways:

Block::default()
    .title("old system"),
    .titles(vec![
        Title::default()
            .content("Title 1")
            .alignment(Alignment::Right),
        Title::default()
            .content("Title 2")
            .alignment(Alignment::Right)
            .position(Position::Bottom),
        Title::default()
            .content("Title 3")
            .alignment(Alignment::Center)
            .position(Position::Bottom),
        Title::default()
            .content("Cen")
            .alignment(Alignment::Center)
            .position(Position::Bottom),
        Title::default()
            .content("Center 1")
            .alignment(Alignment::Center),
        Title::default().content("Cen").alignment(Alignment::Center),
    ])
    .borders(Borders::ALL);

where the content is just a Line like in the old system.

Note: I haven't added tests, documentation, or bothered with code style.

@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #232 (3172e37) into main (e869869) will increase coverage by 0.12%.
The diff coverage is 95.17%.

❗ Current head 3172e37 differs from pull request most recent head e5390a0. Consider uploading reports for the commit e5390a0 to get more accurate results

@@            Coverage Diff             @@
##             main     #232      +/-   ##
==========================================
+ Coverage   82.38%   82.50%   +0.12%     
==========================================
  Files          35       36       +1     
  Lines        7197     7301     +104     
==========================================
+ Hits         5929     6024      +95     
- Misses       1268     1277       +9     
Impacted Files Coverage Δ
src/widgets/mod.rs 66.66% <ø> (ø)
src/widgets/block.rs 83.29% <94.01%> (+0.96%) ⬆️
src/layout.rs 87.73% <100.00%> (ø)
src/title.rs 100.00% <100.00%> (ø)

@joshka
Copy link
Member

joshka commented Jun 9, 2023

Can you explain the need for this a bit more. How would you explain the feature to someone that hasn’t seen btop? I took a look at the screenshot and can’t identify how multiple titles is useful.

@samyosm
Copy link
Contributor Author

samyosm commented Jun 9, 2023

I believe it would allow for a better user experience. For example, similarly to Btop, the titles can be useful for indicating to users keyboard shortcuts they can make use of in a block; one might also want to add additional information in a block's border such as a status bar with word counts, current location, and such. Here are other uses I found:

  1. Hierarchical information representation. E.g. a main title and a subtitle;
  2. Navigation. E.g. arrows on each side of a block;
  3. Step-by-step instructions or tutorials. I.e. something like a wizard;
  4. Tabs; and,
  5. Filtering and sorting options.

@joshka
Copy link
Member

joshka commented Jun 9, 2023

Sorry, I think I wasn't clear. Help me understand how the desired state is desirable. I'm looking for a description of the feature that tells me the effect of this change. How does it render? What are the features that it implements and how do they interact (multiple titles, alignment, on_bottom)? How does it interact with the current features of the block / borders?

For on_bottom: bool, idiomatic rust would suggest making this a position enum (Position::top/bottom) rather than a bool.

@samyosm
Copy link
Contributor Author

samyosm commented Jun 9, 2023

Oops, sorry for the misunderstanding, I'm new to contributing. For on_bottom, I copied what was there before in the Block widget thinking that's how this library wanted it. I'll try to make the change with the position enum approach.

For the rest:

Novelties

I introduced a new title struct in a separate file similar to what has been done with Text.

How does it render?

If titles is None, it goes with the old title system.

Otherwise, it uses a very similar rendering approach to the old system inside the Block render function:

  1. It calculates the necessary dimensions and offsets based on borders and titles to determine the available space for the titles to be rendered.
  2. It iterates over each title and determines the position and alignment within the block.
  3. Based on the alignment and position, it calculates the x-coordinate for rendering each title.
  4. It determines the y-coordinate based on the position of the title (top or bottom of the block).
  5. It uses the calculated coordinates and the content of the title to set the appropriate line in the buffer (buf.set_line) for rendering.

Being so similar to the prior system, I doubt there will be any differences between using the new system over the old one. This is why I believe we can deprecate the previous system.

@joshka
Copy link
Member

joshka commented Jun 10, 2023

Got it
Allow the block to render multiple titles which each can be positioned either top or bottom, aligned (Left / Center / Right), and individually styled.

I wonder if we can change title() to accept Into<Title> instead of Into<Line> (and then impl From<Line> for Title). Then we could allow title() to be called multiple times. e.g.:

Block::default()
    .title(Title::default()
        .content("Left")
        .alignment(Alignment::Left)
        .style(Style::default().fg(Color::Yellow))
    .title(Title::default().content("Center").alignment(Alignment::Center))
    .title(Title::default().content("Right").alignment(Alignment::Right))
    .title(Title::default().content("Left").alignment(Alignment::Left).position(Position::Bottom))
    .title(Title::default().content("Center").alignment(Alignment::Center)).position(Position::Bottom)
    .title(Title::default().content("Right").alignment(Alignment::Right)).position(Position::Bottom)

Thoughts?

@samyosm
Copy link
Contributor Author

samyosm commented Jun 10, 2023

Yes, true. I believe that to be a viable option. I'll work on it sometimes soon.

@samyosm
Copy link
Contributor Author

samyosm commented Jun 10, 2023

I implemented the suggestion you made but had to remove the old system's title_alignment and title_on_bottom functions. As such, I rewrote some demo examples and some tests.

I'm now uncertain whether or not to bring them back for backward compatibility. As reading them would mean people won't have to make any changes to their current library. We could just mark them deprecated.

Please let me know your opinion on this.

@joshka
Copy link
Member

joshka commented Jun 10, 2023

I think leave them in if possible and mark them deprecated.
Perhaps make the field used for the default for the alignment?

@samyosm
Copy link
Contributor Author

samyosm commented Jun 10, 2023

I added tests, and brought back title_alignment and title_on_bottom. All that is left is adding documentation and deprecating title_alignment and title_on_bottom. But I'm uncertain how to do both: never documented anything in rust, and deapracting would seem to need a version since I don't know where to get.

https://github.com/tui-rs-revival/ratatui/blob/9ecc4a15df5731fdf6956320cbb6737d7b01ec08/src/widgets/block.rs#L145-L148

Backward compatibility-wise, people shouldn't have to change anything. Thought I haven't tested this.

@joshka
Copy link
Member

joshka commented Jun 10, 2023

The examples are generally good for checking the backwards compatibility as they show what a user will see if they are using the deprecated methods. Assume the next major version for a deprecation (v0.22.0).

@samyosm
Copy link
Contributor Author

samyosm commented Jun 10, 2023

All done I believe.

@joshka joshka self-requested a review June 11, 2023 02:32
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'm generally approving the idea. Grouping the feedback from this review:

  • consider whether to make the existing block methods act as defaults for each title. This makes it consistent with other widgets that do this (like Paragraph::style() which acts as a default)
  • make sure the docs cover the edge cases (overlapping), note the defaults, and indicate how the titles will be rendered. (it might be useful to show a couple of example outputs in the docs to make it easy to visualize this)

examples/inline.rs Outdated Show resolved Hide resolved
src/title.rs Outdated Show resolved Hide resolved
src/title.rs Outdated Show resolved Hide resolved
src/title.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Show resolved Hide resolved
tests/widgets_block.rs Show resolved Hide resolved
@joshka
Copy link
Member

joshka commented Jun 11, 2023

A lot of my comments come down to picking an API that:

  • avoids breaking changes when possible and reasonable
  • is the simplest least surprising interface
  • avoids repetition

I think that

Block::new()
  .title("A")
  .title("B")
  .title_style(Style::default()...)
  .title_position(Position::Bottom)

is fine

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.

Still looking for some changes on this to make it easy to see how it works.

The main change needed is to render any title style, position or alignment as an override in the render rather than by modifying the stored value.

src/title.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
@joshka
Copy link
Member

joshka commented Jun 12, 2023

P.s. Thanks for sticking with it on this review.

I think the next change you make will probably be ready to merge. When you push can you please squash the changes into a single commit and update the commit message to use the conventional commit format with a header and a body that describes the feature enough that it works well for the changelog.

I didn't mention in the review, but would like to see it some tests that show how overlapping titles are handled (e.g. what happens when there is only 10 characters wide and there are 3 titles (left, center, right aligned) and they are each 4 characters wide...?)

@samyosm
Copy link
Contributor Author

samyosm commented Jun 13, 2023

I tried using Commitizen for the commit, message but it would seem not to have worked.

@samyosm samyosm changed the title feat(Block): Multiple titles feat(block): support for having more than one title Jun 13, 2023
@samyosm samyosm force-pushed the multiple-titles branch 3 times, most recently from 44e5dc6 to eec56ce Compare June 13, 2023 05:29
@samyosm
Copy link
Contributor Author

samyosm commented Jun 14, 2023

I believe everything to now be ready.

test.txt Outdated Show resolved Hide resolved
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.

Please rebase on main (there's an updated typos.toml that has a fix for the ratatui typo) and fix up the typos (use typos --write-changes?

src/widgets/block.rs Outdated Show resolved Hide resolved
tests/widgets_block.rs Show resolved Hide resolved
@samyosm
Copy link
Contributor Author

samyosm commented Jun 18, 2023

Wait, did I mess something up while rebasing? I'm new to this and I'm not sure.

@joshka
Copy link
Member

joshka commented Jun 18, 2023

Yes maybe, but it's not the end of the world.
If you want to get back to exactly where you were, take a look at the output of git reflog There will a line that is just before a line that says rebase (start): checkout main. You can write git reset --hard <the SHA1 hash on that line> and you're exactly where you were.

I'd recommend taking the following steps:

git switch main

# assuming the this repo is your upstream remote:
git pull upstream main 

git switch multiple-titles

# this is optional - it just creates a branch that helps you easily go back to where you were
git branch mutliple-titles-before-rebase 

# assuming that you've already reset the branch back to where you want to merge
git rebase -i <some commit hash that is not your code>
# edit each line that is your code except the first and mark it as s or squash. close the file
# this will result in the code that you've written being a single commit
# now rebase on main

git rebase main
git push origin multiple-titles --force

@samyosm samyosm force-pushed the multiple-titles branch 2 times, most recently from f824305 to 5bab461 Compare June 18, 2023 14:09
@samyosm
Copy link
Contributor Author

samyosm commented Jun 18, 2023

Thanks a lot, that really helped. And, sorry for the extra work.

@joshka joshka added this pull request to the merge queue Jun 19, 2023
Merged via the queue into ratatui-org:main with commit a04b190 Jun 19, 2023
8 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

3 participants