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: Hide default font name for SystemFontProvider, change default font #2385

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

Jklawreszuk
Copy link
Collaborator

@Jklawreszuk Jklawreszuk commented Jul 13, 2024

PR Details

My PR tries to hide default default font name for SystemFontProvider. I also changed default font name from Courier New to Arial for sake of consistency - (Almost) all samples are using Arial font anyway.
In addition, Arial is a sans-serif font - these types of fonts are more popular and easier to read.

  • - This also fixes samples / templates for systems where Arial font is not available (i.e Linux)

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (kinda, because this changes font :P)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

@Jklawreszuk Jklawreszuk marked this pull request as draft July 13, 2024 08:10
@Jklawreszuk Jklawreszuk marked this pull request as ready for review July 13, 2024 08:23
@Eideren
Copy link
Collaborator

Eideren commented Jul 13, 2024

So, this PR is mostly to ensure that by default samples use a font that is compatible with the system it was created on, right ?

This also fixes samples / templates for systems where Arial font is not available (i.e Linux)

You meant to say that Courrier is not available in Linux, right otherwise your changes to make the default font for font assets Arial doesn't make a lot of sense ?

@Jklawreszuk
Copy link
Collaborator Author

So, this PR is mostly to ensure that by default samples use a font that is compatible with the system it was created on, right ?

@Eideren Correct 😅 On most systems it will be the Arial font but on Linux does not. On most distros most common one would be Liberation (i.e. LibreOffice use it as default font). Thus, I would prefer to void direct font calling for samples and default font.

You meant to say that Courrier is not available in Linux, right otherwise your changes to make the default font for font assets Arial doesn't make a lot of sense ?

Both Arial and Courier New technically are available on any OS (no one stops you from installing MS fonts, Example: MS Fonts for Alpine Linux). But that's too anoying to install another dependency and most distributions offer Microsoft font replacements (the replacement for Times New Roman is Liberation Serif , for Arial is Liberations Sans etc)

@Eideren
Copy link
Collaborator

Eideren commented Jul 14, 2024

Right, so not specifying any font would automatically pick one of the fonts available on the current OS ?

@Jklawreszuk
Copy link
Collaborator Author

@Eideren If I understand correctly not specifying the parameters will result in not overwriting the FontName property value of SystemFontProvider and the method GetDefaultFontNamewill be called from the construtor instead - SystemFontProvider line 23

@Jklawreszuk
Copy link
Collaborator Author

Jklawreszuk commented Jul 15, 2024

I tested it by running Gamestudio and indeed it sets to Arial as expected.
I have one more observation : It is possible to set back to Arial font explicitly and but I think that a matter for another PR to detect "default" font in the GameStudio editor (i.e you can choose to set font name Arial or Default (Arial / Liberation))

@Eideren Eideren merged commit b40ad46 into stride3d:master Jul 15, 2024
3 checks passed
@Eideren
Copy link
Collaborator

Eideren commented Jul 15, 2024

Thanks for the clarifications

@Jklawreszuk Jklawreszuk deleted the hide-default-font branch September 8, 2024 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants