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

Refactor FontMetrics #1485

Closed
rvilarl opened this issue Dec 10, 2022 · 2 comments · Fixed by #1486
Closed

Refactor FontMetrics #1485

rvilarl opened this issue Dec 10, 2022 · 2 comments · Fixed by #1486

Comments

@rvilarl
Copy link
Collaborator

rvilarl commented Dec 10, 2022

FontMetrics has two issues that I would like to resolve:

  • the different values are not consistenly located inside or outside the glyphs section.
  • a lot of any

I will submit a PR including all the changes and smaller PRs (part of the previous one to facilitate the review)

@AaronDavidNewman
Copy link
Collaborator

AaronDavidNewman commented Dec 13, 2022

a couple of pretty unrelated observations:

  1. Shouldn't Font always have a FontData - no ? (?)

  2. Many classes, e.g. StaveSection have a TEXT_FONT that is never used and can be removed.

  3. Just some random thoughts about the defaults:

The way I usually handle defaults in Smoosic, and I think it might work well for font defaults:

export enum FONT_FAMILY_DEFAULTS {
SANS_DEFAULT: 'Arial, , sans-serif ', SERIF_DEFAULT: 'Times New Roman, serif'
};

export interface FontInfo {
  family?: string;
  size?: number | string;
  weight?: string | number;
  style?: string;
  static get  defaults() {
  return {  family: FONT_FAMILY_DEFAULTS.SANS_DEFAULT, size: 10, weight: FONT_WEIGHT.NORMAL, style: FONT_STYLE.NORMAL };
  }
}

I think having a Font.SIZE is a little misleading since it's just a default for the size of a 'FontInfo'. And music fonts have a different default size. I realize this is a lot of trivial changes, but 90% of the time we want FontInfo.defaults whenever a text font is needed for a modifier. We might also want to have a defaultSans and defaultSerif (default) font, with FontInfo.defaults returning FontInfo.defaultSans.

  1. I always find Font.ts confusing the way it mixes text and music fonts. I don't know the solution because many nouns and verbs apply to both, so maybe what we have is the best it can be. But maybe we should comment this - I find I need to relearn this every time I look at this code.
  • FontInfo - only applies to text fonts
  • FontModule - only applies to music fonts(?)
  • FontData - applies to both music and text fonts
  • FontMetrics - applies only to music fonts.
  • FontGlyph - applies to both music fonts and text fonts
  • Font - a union of text and music font utilities, methods and data - a Font object could represent either a text font or a music font.

When we add 'FontKerning', that would apply only to text fonts. If we decide to read music fonts directly from font files, I don't think any of this will change.

@rvilarl
Copy link
Collaborator Author

rvilarl commented Dec 13, 2022

@AaronDavidNewman I have created a new issue with your comment. I agree that the mix today is not clear.

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 a pull request may close this issue.

2 participants