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

Blurry Text for PT Run improvements #6049

Merged
merged 3 commits into from
Aug 21, 2020

Conversation

crutkas
Copy link
Member

@crutkas crutkas commented Aug 19, 2020

Summary of the Pull Request

Heavily based off work from #5126 by @AnuthaDev, this has a few additional tweaks to crisp up a few things and got hint text again. They are the driving force here, not me.

Will create a test installer for people who are affected to try it and will also test on a VM which I'm still in the process of spinning up.

PR Checklist

Info on Pull Request

What does this include?

Validation Steps Performed

Current on master
image

With Adjustments
image

Current on master
image

With adjustments
image

and in a later tweak ...
image

@AnuthaDev
Copy link

Finally!

@crutkas crutkas marked this pull request as ready for review August 19, 2020 22:00
Copy link
Contributor

@niels9001 niels9001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the characters seem to be a bit sharp sometimes to me, but it's a minor thing. But, that might be related to WPF font-rendering, so let's get this in. Hopefully at some point WinUI could solve this.

@crutkas
Copy link
Member Author

crutkas commented Aug 20, 2020

Some of the characters seem to be a bit sharp sometimes to me, but it's a minor thing. But, that might be related to WPF font-rendering, so let's get this in. Hopefully at some point WinUI could solve this.

@niels9001, part of me wonders if this should be a setting versus on by default.

@niels9001
Copy link
Contributor

be a sett

Hmm.. I'm not sure if a lot of people would notice.. Maybe get this in, if issues arise make it a setting?

FontSize="24"
Foreground="{DynamicResource TextControlPlaceholderForeground}"
/>

Copy link

@AnuthaDev AnuthaDev Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe remove this line...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the green line 85?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@crutkas
Copy link
Member Author

crutkas commented Aug 20, 2020

@AnuthaDev did you see my comment on your PR, #5126? want to be sure credit is properly attributed

Copy link
Contributor

@arjunbalgovind arjunbalgovind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested the changes on a full HD monitor and a VM at 1366x768. The font is clear on low res monitors. On my full HD monitor, as @niels9001 mentioned there is some sharpness in some characters. I think it's probably more noticeable if you have a large monitor. But I think it's not very different so it should be fine till migration to WinUI. Here is a screenshot of the two
Original:
maps old
With PR changes:
maps new

@crutkas
Copy link
Member Author

crutkas commented Aug 20, 2020

@AnuthaDev pending how you want to move forward, we're ready to get this put in.

@AnuthaDev
Copy link

@AnuthaDev did you see my comment on your PR, #5126?

I was asleep😅

Want to be sure credit is properly attributed

Ahh, it's alright, you have already mentioned me in the PR which is enough for me 😊. I just want it to land asap so I can finally use launcher without my eyes hurting.

Please merge this PR, I am closing #5126

@AnuthaDev
Copy link

@AnuthaDev pending how you want to move forward, we're ready to get this put in.

I don't want to delay the improvements. Please merge this PR.

@crutkas crutkas merged commit cc0b808 into master Aug 21, 2020
@crutkas crutkas deleted the dev/crutkas/blurryFixOffAnuthaDevPr branch August 21, 2020 17:20
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.

5 participants