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(style): Add style with fn #287

Merged
merged 1 commit into from
Jul 8, 2023
Merged

Conversation

a-kenji
Copy link
Contributor

@a-kenji a-kenji commented Jul 1, 2023

This allows the full style to be constructed in a const context.

A const style can be used in the following way:

const DEFAULT_MODIFIER: Modifier = Modifier::BOLD.union(Modifier::ITALIC);
const EMPTY: Modifier = Modifier::empty();
const DEFAULT_STYLE: Style = Style::with(DEFAULT_MODIFIER, EMPTY)
.fg(Color::Red).bg(Color::Black);

@codecov
Copy link

codecov bot commented Jul 1, 2023

Codecov Report

Merging #287 (41304bb) into main (56e44a0) will increase coverage by 0.04%.
The diff coverage is 88.46%.

❗ Current head 41304bb differs from pull request most recent head 6ac1500. Consider uploading reports for the commit 6ac1500 to get more accurate results

@@            Coverage Diff             @@
##             main     #287      +/-   ##
==========================================
+ Coverage   82.92%   82.97%   +0.04%     
==========================================
  Files          37       37              
  Lines        7479     7500      +21     
==========================================
+ Hits         6202     6223      +21     
  Misses       1277     1277              
Impacted Files Coverage Δ
src/widgets/canvas/line.rs 59.67% <50.00%> (ø)
src/widgets/canvas/mod.rs 91.88% <50.00%> (ø)
src/style.rs 86.85% <100.00%> (+1.20%) ⬆️

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 prefer not to have methods that have two parameters of the same type because anyone reading the code unfamiliar with the library will always have to look up what each parameter indicates.

However, this can done without a new method by making add/remove_modifier() const. The implementations only have to change slightly to support this as insert() / remove() on bitflags aren't const-safe:

    pub const fn add_modifier(mut self, modifier: Modifier) -> Style {
        self.sub_modifier = self.sub_modifier.difference(modifier);
        self.add_modifier = self.add_modifier.union(modifier);
        self
    }

    pub const fn remove_modifier(mut self, modifier: Modifier) -> Style {
        self.add_modifier = self.add_modifier.difference(modifier);
        self.sub_modifier = self.sub_modifier.union(modifier);
        self
    }

    #[test]
    fn style_can_be_const() {
        const _RED: Color = Color::Red;
        const _BLACK: Color = Color::Black;
        const _BOLD: Modifier = Modifier::BOLD;
        const _ITALIC: Modifier = Modifier::ITALIC;

        const _RESET: Style = Style::reset();
        const _RED_FG: Style = Style::new().fg(_RED);
        const _BLACK_BG: Style = Style::new().bg(_BLACK);
        const _ADD_BOLD: Style = Style::new().add_modifier(_BOLD);
        const _REMOVE_ITALIC: Style = Style::new().remove_modifier(_ITALIC);
        const _ALL: Style = Style::new()
            .fg(_RED)
            .bg(_BLACK)
            .add_modifier(_BOLD)
            .remove_modifier(_ITALIC);
        assert_eq!(
            _ALL,
            Style::new()
                .fg(Color::Red)
                .bg(Color::Black)
                .add_modifier(Modifier::BOLD)
                .remove_modifier(Modifier::ITALIC)
        );

@joshka
Copy link
Member

joshka commented Jul 1, 2023

Also - please try to give a name to the commit that indicates the user facing benefit (e.g. "make Style support const context" or something similar).

@a-kenji
Copy link
Contributor Author

a-kenji commented Jul 2, 2023

Thank you for the review, this is indeed a much better solution!

Allows Modifiers to be added or removed from `Style` in a const context.
This can be used in the following way:

```
const DEFAULT_MODIFIER: Modifier = Modifier::BOLD.union(Modifier::ITALIC);
const DEFAULT_STYLE: Style = Style::new()
.fg(Color::Red).bg(Color::Black).add_modifier(DEFAULT_MODIFIER);
```
@joshka joshka enabled auto-merge July 8, 2023 10:12
@joshka joshka added this pull request to the merge queue Jul 8, 2023
Merged via the queue into ratatui:main with commit f7c4b44 Jul 8, 2023
@joshka
Copy link
Member

joshka commented Jul 8, 2023

Rebased and approved. Thanks for this :)

a-kenji added a commit to a-kenji/ratatui that referenced this pull request Jul 16, 2023
Allows Modifiers to be added or removed from `Style` in a const context.
This can be used in the following way:

```
const DEFAULT_MODIFIER: Modifier = Modifier::BOLD.union(Modifier::ITALIC);
const DEFAULT_STYLE: Style = Style::new()
.fg(Color::Red).bg(Color::Black).add_modifier(DEFAULT_MODIFIER);
```
@a-kenji a-kenji deleted the add-style-with branch July 17, 2023 11:01
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.

3 participants