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

Implemented DPI process bug fix #717

Merged
merged 6 commits into from
Aug 15, 2019
Merged

Implemented DPI process bug fix #717

merged 6 commits into from
Aug 15, 2019

Conversation

everettraven
Copy link
Contributor

Changed the app.py create method for toga_winforms to check the current OS Version and build number to make sure we are using the most up to date DPI setting API for the user. Came across this issue when updating windows to the 1903 update (Build #: 18362)

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

shcore.SetProcessDpiAwareness(True)
elif win_version >= 10 and win_build >= 15063:
user32.SetProcessDpiAwarenessContext(True)

Copy link
Contributor

Choose a reason for hiding this comment

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

At column 1: (W293) blank line contains whitespace

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks good (and works for my testing); a couple of minor tweaks inline.

win_version = Environment.OSVersion.Version.Major
win_build = Environment.OSVersion.Version.Build
Copy link
Member

Choose a reason for hiding this comment

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

This is a really small thing, but could we change win_version to be Environment.OSVersion.Version, and then get access to all the structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went ahead and changed it to that.

user32.SetProcessDPIAware(True)
elif win_version >= 8 and win_build < 15063:
Copy link
Member

Choose a reason for hiding this comment

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

Are you able to provide any reference to documentation for why these magic numbers exist? An inline docstring will help explain the magic to whoever comes along later.

Copy link
Member

Choose a reason for hiding this comment

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

Actually - this specific check looks a little off - a windows 8 build with a high build number will break on this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct that this check is wrong as the windows 8 major version number is actually a 6 and not an 8. In regards to build number it should only pull the update build number, which for Windows 8 the highest build number is 9600. I will fix this tomorrow when I have the chance. Source for information: https://www.lifewire.com/windows-version-numbers-2625171

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and implemented much better checks and loads of comments documenting what is what and why.

@@ -21,8 +21,23 @@ def __init__(self, interface):
def create(self):
self.native = WinForms.Application

if win_version >= 6:
user32.SetProcessDPIAware(True)
# Check the version of windows and make sure we are setting the DPI mode
Copy link
Contributor

Choose a reason for hiding this comment

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

At column 81: (W291) trailing whitespace

# Windows Versioning Check Sources : https://www.lifewire.com/windows-version-numbers-2625171
# and https://docs.microsoft.com/en-us/windows/release-information/
if win_version.Major >= 6: # Checks for Windows Vista or later
# Represents Windows 8.1 up to Windows 10 before Build 1703 which should use
Copy link
Contributor

Choose a reason for hiding this comment

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

At column 89: (W291) trailing whitespace

if win_version.Major >= 6: # Checks for Windows Vista or later
# Represents Windows 8.1 up to Windows 10 before Build 1703 which should use
# SetProcessDpiAwareness(True)
if (win_version.Major == 6 and win_version.Minor == 3) or (win_version.Major == 10 and win_version.Build < 15063):
Copy link
Contributor

Choose a reason for hiding this comment

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

At column 120: (E501) line too long (126 > 119 characters)

if win_version.Major >= 6: # Checks for Windows Vista or later
# Represents Windows 8.1 up to Windows 10 before Build 1703 which should use
# SetProcessDpiAwareness(True)
if (win_version.Major == 6 and win_version.Minor == 3) or (win_version.Major == 10 and win_version.Build < 15063):
Copy link
Contributor

Choose a reason for hiding this comment

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

At column 127: (W291) trailing whitespace

# SetProcessDpiAwareness(True)
if (win_version.Major == 6 and win_version.Minor == 3) or (win_version.Major == 10 and win_version.Build < 15063):
shcore.SetProcessDpiAwareness(True)
# Represents Windows 10 Build 1703 and beyond which should use
Copy link
Contributor

Choose a reason for hiding this comment

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

At column 75: (W291) trailing whitespace

if win_version.Major >= 6: # Checks for Windows Vista or later
# Represents Windows 8.1 up to Windows 10 before Build 1703 which should use
# SetProcessDpiAwareness(True)
if ((win_version.Major == 6 and win_version.Minor == 3) or
Copy link
Contributor

Choose a reason for hiding this comment

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

At column 71: (W291) trailing whitespace

# Represents Windows 8.1 up to Windows 10 before Build 1703 which should use
# SetProcessDpiAwareness(True)
if ((win_version.Major == 6 and win_version.Minor == 3) or
(win_version.Major == 10 and win_version.Build < 15063)):
Copy link
Contributor

Choose a reason for hiding this comment

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

At column 5: (E129) visually indented line with same indent as next logical line

# Represents Windows 8.1 up to Windows 10 before Build 1703 which should use
# SetProcessDpiAwareness(True)
if ((win_version.Major == 6 and win_version.Minor == 3) or
(win_version.Major == 10 and win_version.Build < 15063)):
Copy link
Contributor

Choose a reason for hiding this comment

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

At column 74: (W291) trailing whitespace

# with the most up to date API
# Windows Versioning Check Sources : https://www.lifewire.com/windows-version-numbers-2625171
# and https://docs.microsoft.com/en-us/windows/release-information/
if win_version.Major >= 6: # Checks for Windows Vista or later
Copy link
Contributor

Choose a reason for hiding this comment

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

At column 35: (E261) at least two spaces before inline comment

if win_version.Major >= 6: # Checks for Windows Vista or later
# Represents Windows 8.1 up to Windows 10 before Build 1703 which should use
# SetProcessDpiAwareness(True)
if ((win_version.Major == 6 and win_version.Minor == 3) or
Copy link
Contributor

Choose a reason for hiding this comment

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

At column 71: (W291) trailing whitespace

# Represents Windows 8.1 up to Windows 10 before Build 1703 which should use
# SetProcessDpiAwareness(True)
if ((win_version.Major == 6 and win_version.Minor == 3) or
(win_version.Major == 10 and win_version.Build < 15063)):
Copy link
Contributor

Choose a reason for hiding this comment

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

At column 13: (E128) continuation line under-indented for visual indent

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Awesome - Nicely done, and thanks for the contribution!

If you're still in this DPI hole, you might want to investigate why the font sizes in tutorial1 aren't adapting to DPI sizing... but that's not related to this patch.

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.

3 participants