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

Add metrics to attributes #257

Merged
merged 6 commits into from
Jun 7, 2024
Merged

Add metrics to attributes #257

merged 6 commits into from
Jun 7, 2024

Conversation

jackpot51
Copy link
Member

@jackpot51 jackpot51 commented Jun 6, 2024 •
edited
Loading

Rewrite of #235, fixes #64

@jackpot51 jackpot51 self-assigned this Jun 6, 2024
alice-i-cecile
alice-i-cecile reviewed Jun 6, 2024
View reviewed changes
src/attrs.rs Outdated Show resolved Hide resolved
nicoburns
nicoburns suggested changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@nicoburns nicoburns left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Haven't had chance to review the code yet, but there is bug when creating new lines:

Screenshot 2024-06-06 at 23 41 06

(newly created lines end up with the "font size 64" line-height even though there is no text on that line with that height)

@jackpot51
Copy link
Member Author

Right now it takes the maximum of either the buffer line height or the glyph line height for the whole run. I could probably only reference the glyph line height but I didn't want to run into empty lines being 0 height.

@jackpot51
Copy link
Member Author

@nicoburns should be resolved in the latest commit.

alice-i-cecile
alice-i-cecile approved these changes Jun 6, 2024
View reviewed changes
Copy link

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This seems reasonable from a code quality perspective, and I can follow the logic :)

@jackpot51 jackpot51 marked this pull request as ready for review June 6, 2024 22:57
@jackpot51 jackpot51 requested a review from nicoburns June 6, 2024 23:30
@jackpot51
Copy link
Member Author

This is working well enough for me that I will merge. Any issues I missed can be reported against master.

@jackpot51 jackpot51 merged commit c7100dc into main Jun 7, 2024
2 of 3 checks passed
@jackpot51 jackpot51 deleted the font-size-redux branch June 7, 2024 01:38
@tigregalis tigregalis mentioned this pull request Jun 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@nicoburns nicoburns Awaiting requested review from nicoburns

Assignees

@jackpot51 jackpot51

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Spans of font sizes
3 participants
@jackpot51 @nicoburns @alice-i-cecile