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

replace 'Courier New' with 'monospace' in Linux font list #34947

Merged
merged 1 commit into from Oct 2, 2017

Conversation

Projects
None yet
5 participants
@andyli
Contributor

andyli commented Sep 25, 2017

A workaround better than #33319.

Close #5742.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Sep 25, 2017

@andyli, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alexandrudima and @rebornix to be potential reviewers.

mention-bot commented Sep 25, 2017

@andyli, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alexandrudima and @rebornix to be potential reviewers.

@jhasse

This comment has been minimized.

Show comment
Hide comment
@jhasse

jhasse Sep 26, 2017

Contributor

I've tested it on Fedora 26 and it works as intended: 'monospace' results in Liberation Mono just as 'Courier New' did. The problem is that if Courier New is really installed, it will look terrible (I guess because it was designed with Windows' hinting algorithm in mind, not FreeType's), see #5742 (comment). @Tyriar mentioned that monospace doesn't help, but according to #5742 (comment) that seems to be a Chromium bug and should be fixed by adding single-quotes around it just like this PR does :)

Contributor

jhasse commented Sep 26, 2017

I've tested it on Fedora 26 and it works as intended: 'monospace' results in Liberation Mono just as 'Courier New' did. The problem is that if Courier New is really installed, it will look terrible (I guess because it was designed with Windows' hinting algorithm in mind, not FreeType's), see #5742 (comment). @Tyriar mentioned that monospace doesn't help, but according to #5742 (comment) that seems to be a Chromium bug and should be fixed by adding single-quotes around it just like this PR does :)

@Tyriar Tyriar self-requested a review Sep 26, 2017

@alexandrudima alexandrudima self-requested a review Sep 27, 2017

@alexandrudima

LGTM

@alexandrudima

This comment has been minimized.

Show comment
Hide comment
@alexandrudima

alexandrudima Sep 27, 2017

Member

@Tyriar I see you would also like to test things out. This PR looks good to me. Please feel free to merge once you had a chance to review.

Member

alexandrudima commented Sep 27, 2017

@Tyriar I see you would also like to test things out. This PR looks good to me. Please feel free to merge once you had a chance to review.

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Sep 27, 2017

Member

Yes I wanted to test this on a few distros to verify, targeting October 😃

Member

Tyriar commented Sep 27, 2017

Yes I wanted to test this on a few distros to verify, targeting October 😃

@Tyriar Tyriar added this to the October 2017 milestone Sep 27, 2017

@Tyriar Tyriar merged commit cdd2de7 into Microsoft:master Oct 2, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.

@andyli andyli deleted the andyli:linux-monospace branch Oct 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment