Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
glyph: Add working braille custom glyphs #919
glyph: Add working braille custom glyphs #919
Changes from 8 commits
d9baaf7
52bbc7d
4e447d4
e63b542
c96973c
eeca274
a7ffc35
0f37957
2111540
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm finding this naming a bit confusing;
cell_width
is used in many places in wezterm to refer to the width of the terminal cell and I'd assumed that was true here and getting confused about how the dots fit in the cell, but then I saw that this is actually half that size!Could you make a pass through to clarify these?
I think
square_length
is the key metric being computed here, so I'd suggest simplifying these into something like this:I think this makes it a bit easier to visualize the positioning when thinking about the cell overall and how that gets divided up into an interior grid.
I think it's probably a good idea to have a separate
square_width
andsquare_height
because different fonts have different aspect ratios, which makes it potentially problematic to use a metric derived from the X in the Y direction.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was thinking that
cell_width
isn't great naming either, sorry for the confusion 👀As for the suggested vars definition, I actually find it harder to see how it works overall..
I'd be ok to rename the badly named var to sth like
dot_area_width
maybe (or you have better naming?), but calculating thetopleft_offset_x
like you suggest isn't great IMO, because as soon as you want to have a slightly bigger square, you're out of luck, and have to rethink all your formulas. However doing this with my use of variables I believe is as easy as changing thesquare_length
value, and the rest will adapt.I don't think it's worth removing the intermediate variables, as in a release build llvm will probably find ways to optimize things.. And it's not as if the code was a hot-path for the rendering, it's only called a few times to build the cache.
If you really want I can do what you suggest (or you can change the code yourself afterward 👀), but know that I personally wouldn't want to maintain these simplified definitions if it was my project.
I don't agree, if the dots were circles, would you like to have ovals? I don't think so..
And it's the same for sharp dots, I think these should be actual squares, not ~rectangles.
For the fonts with different aspect ratios, do you have something in mind that wouldn't look great? Maybe we can find an optimal
square_length
with some min/max of some other values?Also, did you see my PR comment here: #919 (comment) ?