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

OfTrueTypeFont constructor+misc #7740

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

artificiel
Copy link
Contributor

@artificiel artificiel commented Nov 1, 2023

the constructor allows .h use like this:

ofTrueTypeFont font_titre_ {"fonts/Univers LT Std/UniversLTStd-BoldCn.otf", 20, true, true, true};

other tweaks allow positioning from glm::vec2, and additional (0) default values for getStringBoundingBox.

[edit] also aligned the ofTrueTypeFontSettings constructor to ofTrueTypeFont's. while the idea of Settings class is evidently to provide explicitly named parameters (and fine-grained) configs, a given Setting should be as configurable as the class' most complex available setup(). It is arguable that the ofTrueTypeFont's setup is too complicated and should be reduced to enforce the Settings, but until that happens they should at least be coherent.

@dimitre
Copy link
Member

dimitre commented Nov 20, 2023

One suggestion for future PRs is splitting different subjects in different PRs, so they can be reviewed, discussed and merged faster.
it was even recommended to me to have separate PRs for functionality change and ident fix

@artificiel
Copy link
Contributor Author

I don't mind splitting but at the same time got the opposite suggestion from @ofTheo vs my jumbo OSC PR #7658.

what I'm aiming for is limiting individual commits to single code changes with a name that indicates clearly the change, and especially keep formatting is separate commits so the individual functional changes can be audited separately without being obfuscated in whitespace changes. so for instance in the above we see 4 code changes which are self-contained and well labeled.

would it be preferable to have 4 distinct PR's? (and also consider the trickiness that they modify the same file, sometimes with sequential dependency, hence enhancing the risk of merge conflicts?)

a strategy seen elsewhere is "a PR must close a single issue, and the issue must exist prior to the PR" (which implies the mandate has been clarified). this is in the spirit of discussing a future change before spending time on implementation. for some things it makes sense, but other minor changes (like this one) seems like an overhead for everyone. before GitHub integrates very well the discussion process within the PR, discussions can occur in there too, with a head start "proposal".

@artificiel artificiel mentioned this pull request Dec 12, 2023
36 tasks
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