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(options): add modify_font option #5265

Closed
wants to merge 9 commits into from

Conversation

akinsho
Copy link

@akinsho akinsho commented Jul 12, 2022

This fixes the issue of not being able to modify a font attributes, namely the underline_thickness, strikethrough_position, underline_position and (eventually) size. It supersedes #4723.

To-do:

  • add validation of the arguments for modify_font
  • Handle specifying postscript font name
  • fix handling of *_position as neither currently move the line down nor up.
  • Handle size... (not entirely sure where to start with this)
    • Units are specified as int but maybe should accept % for size
  • Fix bug with strikethrough positioning

Notes for review:

I'm largely new to python and c so any formatting issues or method usages etc. feel free to correct as I've just focused on getting something working and don't know what the best practices are.

@akinsho
Copy link
Author

akinsho commented Jul 12, 2022

@kovidgoyal would be good to get an early sense check on this PR i.e. the general approach, code style vs conventions etc. This essentially works (give or take some outstanding bug with unsigned ints and strike-through positioning) locally for the use case I described in the original comment I made, i.e. I'm able to change the thickness and height of the underline etc.

I haven't done anything about the size of fonts yet since I'm less sure about how that functionality should work I guess somewhere in the codebase the cell size of each font including fallbacks is being determined, and I need to get the config value over to there. I appreciate you included this in the required feature set but presumably if a general mechanism for specifying font overrides in the config and them flowing through is done then this could maybe be added without needed a refactor i.e. at that point it's just an additional feature

@kovidgoyal
Copy link
Owner

I'll review it when I have some time. cell sizes are calculated only for
the main font. Size adjustment is intended to apply to
fallback/alternate faces anyway, so it doesnt affect cell size
calculation, for which there are already other options.

baseline adjustment probably needs to be per font face as well.
underline and strikethrough adjustment apply only to the main font.

@akinsho
Copy link
Author

akinsho commented Jul 13, 2022

@kovidgoyal thanks for clarifying so if I understand correctly based on

cell sizes are calculated only for
The main font. Size adjustment is intended to apply to
fallback/alternate faces anyway

Cell size is calculated for only the main ("medium") face here

cell_metrics(fg->fonts[fg->medium_font_idx].face, &cell_width, &cell_height, &baseline, &underline_position, &underline_thickness, &strikethrough_position, &strikethrough_thickness);

and currently all the other fonts inherit this calculation?

So when you say size, since you've clarified you don't mean cell size what dimension specifically do you mean you've mentioned the baseline, so you mean altering the baseline for non-main fonts similar to how it is altered in cell_metrics? (presumably alternate fonts can be set higher or lower in the cell separate to the main font, i.e. that's the desired result). Although from what I can see, the baseline is set on the font group, rather than per face.

Apologies if you're having to go over this a few times, I've honestly never dealt with font rendering or anything in this area. I don't think I even know what the outcome of setting adjust size is supposed to actually look like, are these specific fonts now supposed to appear higher or lower in the cell or actually smaller or larger within the cell (I didn't/don't even know if that's possible)

FWIW the rest of it now works, i.e. strikethrough/underline positions and thickness

@kovidgoyal
Copy link
Owner

adjust_font_size should adjust the actual pt size used to render
fallback/alternate fonts into the cell. The cell's size is fixed by the
point size of the main font, which comes from the font_size setting.

Currently what happens is the alternate font's size is set to the same
size as font_size and it is rendered. If the glyph does not fit in the
cell, the size is reduced until it does and the baseline is changed to
match. So with adjust_size the initial
size that the alternate font is rendered at would be adjusted.

And yes changing of baseline can either be automatic as it is currently
when resizing an alternate font or manual if specified.

@akinsho akinsho deleted the add-modify-font branch July 15, 2022 07:12
@akinsho
Copy link
Author

akinsho commented Jul 15, 2022

Thanks for implementing this @kovidgoyal 🙏🏿 and for your patience with my attempt 👍🏿

@kovidgoyal
Copy link
Owner

You're welcome!

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.

2 participants