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

Revert "Font Issue on M1 Mac Big Sur" #119

Merged
merged 1 commit into from
Jan 13, 2022

Conversation

kouyk
Copy link
Contributor

@kouyk kouyk commented Dec 28, 2021

Reverts #106 to resolve #111.

The original PR aimed to side step the garbled fonts on M1 Mac devices by embedding another font of our choice. One upside and unintended consequence is that fonts are consistent across the various OS regardless of font availability, helping to avoid any potential UI bugs such as text being cut off prematurely etc. However, this does bring about unnecessary complication as more resources are being loaded into the application, such as the one surfaced here, mandating the use of even more workarounds such as the one referenced in #111.

The M1 MacOS issue was eventually resolved through the use of an alternative OpenJDK build that happened to have native M1 binaries as well. Given that the issue originated from OpenJDK rather than AB3 itself, it no longer make sense for the "fix" to be applied to AB3, which seems to cause more trouble than it is worth. Given that most students are fresh to JavaFX, the additional complication does not seem warranted.

@canihasreview
Copy link

canihasreview bot commented Dec 28, 2021

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

@codecov-commenter
Copy link

codecov-commenter commented Dec 28, 2021

Codecov Report

Merging #119 (8971ba4) into master (0f5f654) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #119   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f5f654...8971ba4. Read the comment docs.

@damithc
Copy link
Contributor

damithc commented Jan 2, 2022

@kouyk This is an automatic git revert, correct? And this requires us to release a new version, right?

@kouyk
Copy link
Contributor Author

kouyk commented Jan 2, 2022

@kouyk This is an automatic git revert, correct? And this requires us to release a new version, right?

Yes, that's right. I reverted through github, I might need to do some manual fixes to resolve the conflicts now that the help window PR has been merged in.

This reverts commit 9a46153, which was aimed
at sidestepping se-edu#105. A better solution has been
identified which made the current fix redundant while introducing path issues
that are not obvious, see se-edu#111 for more details.
@kouyk kouyk force-pushed the revert-106-105-font-issue-on-m1-mac branch from 2ec9dbc to 8971ba4 Compare January 2, 2022 09:06
@canihasreview
Copy link

canihasreview bot commented Jan 2, 2022

v1

@kouyk submitted v1 for review.

(📚 Archive)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/119/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

@damithc damithc merged commit 8d5b66e into se-edu:master Jan 13, 2022
@damithc
Copy link
Contributor

damithc commented Jan 13, 2022

Thanks very much for the fix @kouyk
I'll do a new release

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.

Loading of embedded fonts via CSS causes runtime issues
3 participants