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

IDLE: Turn on DPI awareness on Windows #77837

Closed
terryjreedy opened this issue May 26, 2018 · 38 comments
Closed

IDLE: Turn on DPI awareness on Windows #77837

terryjreedy opened this issue May 26, 2018 · 38 comments
Assignees
Labels
3.7 3.8 expert-IDLE type-feature

Comments

@terryjreedy
Copy link
Member

@terryjreedy terryjreedy commented May 26, 2018

BPO 33656
Nosy @terryjreedy, @pfmoore, @ronaldoussoren, @taleinat, @tjguk, @zware, @serhiy-storchaka, @zooba, @efahl, @miss-islington
PRs
  • #7137
  • #7638
  • #7639
  • #7640
  • #7641
  • #7642
  • #7643
  • #7644
  • #7646
  • #7647
  • #7648
  • #9858
  • #9862
  • #9863
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/terryjreedy'
    closed_at = <Date 2018-10-14.00:41:17.759>
    created_at = <Date 2018-05-26.23:14:50.803>
    labels = ['3.8', 'expert-IDLE', 'type-feature', '3.7']
    title = 'IDLE: Turn on DPI awareness on Windows'
    updated_at = <Date 2018-10-14.00:41:17.758>
    user = 'https://github.com/terryjreedy'

    bugs.python.org fields:

    activity = <Date 2018-10-14.00:41:17.758>
    actor = 'terry.reedy'
    assignee = 'terry.reedy'
    closed = True
    closed_date = <Date 2018-10-14.00:41:17.759>
    closer = 'terry.reedy'
    components = ['IDLE']
    creation = <Date 2018-05-26.23:14:50.803>
    creator = 'terry.reedy'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33656
    keywords = ['patch']
    message_count = 38.0
    messages = ['317775', '317776', '318040', '318053', '318056', '318068', '318069', '319130', '319148', '319150', '319152', '319159', '319184', '319185', '319192', '319326', '319327', '319328', '319329', '319330', '319331', '319332', '319334', '319336', '319340', '319345', '319346', '319348', '319362', '319403', '319407', '319411', '319426', '327676', '327680', '327681', '327683', '327685']
    nosy_count = 10.0
    nosy_names = ['terry.reedy', 'paul.moore', 'ronaldoussoren', 'taleinat', 'tim.golden', 'zach.ware', 'serhiy.storchaka', 'steve.dower', 'eric.fahlgren', 'miss-islington']
    pr_nums = ['7137', '7638', '7639', '7640', '7641', '7642', '7643', '7644', '7646', '7647', '7648', '9858', '9862', '9863']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue33656'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented May 26, 2018

    On IDLE-dev today, Elisha Paine, from Ranelagh School in England, wrote:
    '''
    I love IDLE (so simple and lightweight compared with other IDEs) and was just wondering if you could add the following code into pyshell.py (as I have done on mine) because, as you will be able to tell, it sets DPI awareness (and on my set-up makes the text a lot clearer)

    import ctypes
    try: ctypes.windll.shcore.SetProcessDpiAwareness (True)
    except: pass
    '''
    On my Win 10 system with a 2560 x 1440 27" monitor, the text is noticeably sharper, some colors are brighter, and some characters are better formed.  The main effect seems to be from properly lining up vertical and horizontal lines with the pixels.  Lines that are supposed to be 1 or 2 pixels wide are just that, instead of bleeding over onto additional rows or columns.

    By comparing IDLE's shell with Windows consoles with the same font and text, both for Command Prompt and Python x.y consoles, I determined that the Windows consoles have DPI awareness on. The text is longer with it off and some character shapes, such as for 2 and 3, don't match, whereas text length and shape matches perfectly with it on.

    At least some other apps also have DPO awareness on. With the patch, the Open and Save As dialogs opened opened by from IDLE, via the tkinter/tk function, match those opened by Firefox. Without the patch, not.

    I like the improvement and with 3.7.0rc1 delayed, would like to get it in now. My question for you Windows experts is 1. Is the above exactly the right thing? and 2. Should it be unconditional, or is there a possible downside? Do consoles always have DPI awareness, or is it conditional on the monitor?

    I will make a PR as soon as I post this and get an issue number.

    @terryjreedy terryjreedy self-assigned this May 26, 2018
    @terryjreedy terryjreedy added expert-IDLE type-feature labels May 26, 2018
    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented May 26, 2018

    I changed the bare except: to 'except AttributeError:'. If ctypes.windll.shcore.SetProcessDpiAwareness exists, can calling it with True ever fail?

    @ronaldoussoren
    Copy link
    Contributor

    @ronaldoussoren ronaldoussoren commented May 29, 2018

    I'm not a Windows expert, but looking that the API description [1] the patch is not entirely correct.

    The API takes an integer with 3 valid values, the patch effectively passes the value 1 (which means the application claims to be DPI aware, but won't scale itself when the DPI changes for some reason).

    The API can fail, but that shouldn't cause an exception as ctypes won't automaticly convert error return values to exceptions.

    P.S. The pull request seems to contain an unrelated change as well.

    [1] https://msdn.microsoft.com/en-us/library/windows/desktop/dn302122(v=vs.85).aspx

    @zware
    Copy link
    Member

    @zware zware commented May 29, 2018

    Does Tcl/Tk have a function for this, and/or should we expose this as a function in Tkinter? Do other platforms (macOS, X/Wayland) have similar functions?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented May 29, 2018

    Perhaps the argument should be 2 for supporting multi-display configuration. Currently I can't do this, but will test this recipe with Hi-DPI monitor and multiple displays after 1-2 weeks.

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented May 29, 2018

    Thanks for the comments:

    Ronald: I agree that an int should be passed. I understand your ctype comment as saying that an attribute error before making the call should be the only possible exception.

    I should have explained the 'unrelated change' to the one entry box. With the default size of 20, it no longer fits properly after the change. Adding an explicit size was the easiest fix for something that is only present for test and demonstration purposes. A better fix, which is not needed for this issue, would probably be to add a 'expand=True' or something, somewhere else.

    I checked all the other panes on the settings dialog and the other major dialog boxes and found no problems.

    Zack: I had the same question. Now that I know that IDLE as a tk app *can* look 'sharp', the current fuzzyness is annoying. I would not be surprised if it contributes to some of the negative opinions about IDLE.

    Tk tries to look 'native' and this seems to be the main thing missing. To me, the only obvious reason for tk to not use this setting as default
    is that is can mess up spacing on current windows. But to me, it definitely should be available.

    I don't know if other systems have such a setting, or any need for such. I will ask an someone who uses IDLE both in Windows and Ubuntu on the same machine to compare.

    Serhiy: I have a space monitor and I believe my graphics card will allow me to attach one, so I may give it a try.

    The linked article recommends "that you set the process-default DPI awareness via application manifest." I don't currently know what this
    means. It appears that making one call before any tkinter calls is the right thing to do if any call is to be made.

    The call does not appear to be available on Windows 7 or 8.0.

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented May 29, 2018

    Eryk Sun commented on the PR.

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 9, 2018

    Elisha wrote and I quoted: '[the call] sets DPI awareness'. After reading https://msdn.microsoft.com/en-us/library/windows/desktop/dn280512(v=vs.85).aspx PROCESS_DPI_AWARENESS, what the call sets is the system's idea of what DPI awareness the app/process has. As I understand,
    0 means 'App assumes DPI = 96, so if it is not, system must adjust.'
    1 means 'App reads DPI at startup (somehow) and adjust to that value, but does not adapt to monitor change, so system must.'
    2 means 'App reads DPI at startup and again after monitor change (somehow), so system should leave it alone.'
    The correct value to pass is the one that best describes what the app does. I believe that 0 is wrong and 1 more likely than 2.

    My own experience and Elisha's report that IDLE looks better (at any resolution setting) matches others. For instance,
    https://stackoverflow.com/questions/41315873/attempting-to-resolve-blurred-tkinter-text-scaling-on-windows-10-high-dpi-disp
    Three people describe fuzzy tk text as a well-known problem, fixed by SetProcessDpiAwareness(1).

    https://wiki.tcl.tk/8484 tk has a scaling function that gets and sets a scaling factor, dpi/72 = pixels per printer point. "The initial value for the scaling factor is set when the application starts, based on properties of the installed monitor." This also suggests to me also that default 0 for awareness is wrong value. Users (apps) can changes this, but I see no suggestion that monitor changes are tracked.

    @efahl
    Copy link
    Mannequin

    @efahl efahl mannequin commented Jun 9, 2018

    https://msdn.microsoft.com/en-us/library/windows/desktop/mt748620(v=vs.85).aspx gives the syntax for adding dpiAwareness to the Windows manifest.

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 9, 2018

    As reported on bpo-26698, which was originally opened as an IDLE issue, but which I turned into a tkinter issue, one can also sharpen text buy opening python(w).exe properties, Compatibility tab, clicking [Change high DPI settings], and then checking [] Override high DPI scaling behavior while leaving the default 'Application'. The effect is the same as 'SetProcessDpiAwareness (1)' -- or (2) if monitor is not changed.

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 9, 2018

    Eric, thanks for the additional reference. From Window's viewpoint, Python, not IDLE, is the application. IDLE is just input data. I should not touch Python's manifest, and changing it would have to consider other possible gui module input data.

    This page says it applies only to Win10 desktop. The page for SetProcessDpiAwareness says it applies to 8.1+. That still excludes Vista and Win7, but is better than just Win10.

    @efahl
    Copy link
    Mannequin

    @efahl efahl mannequin commented Jun 9, 2018

    So maybe add the dpiAware and dpiAwareness settings to the manifest for just Windows' pythonw.exe and leave the python.exe console interpreter alone? I'm going to guess that the pythonw.exe manifest already has some settings related to its unique status that align with this.

    @zooba
    Copy link
    Member

    @zooba zooba commented Jun 9, 2018

    I'm okay to add it to the manifest for both in 3.8, along with a What's New entry.

    High DPI screens are very common though, and adding it to existing releases won't allow existing apps or frameworks to account for the change. I don't want non-DPI aware apps to suddenly become unreadably small with a minor runtime update. Backporting Python-specific docs is fine.

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 9, 2018

    Steve, I would like to add the SetProcessDpiAwareness(1) call to IDLE tomorrow, for 3.7.0 and 3.6.6. Do I need to make it an avoidable option? Can it hurt people on some machines? It seems to me that if tk is doing dpi scaling, then it should always be correct to tell Windows that it is doing so.

    @zooba
    Copy link
    Member

    @zooba zooba commented Jun 10, 2018

    Yep, that should be fine.

    If you want to add it to the winapi module rather than use ctypes that's fine too.

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 11, 2018

    New changeset 800415e by Terry Jan Reedy in branch 'master':
    bpo-33656: On Windows, add API call saying that tk scales for DPI (GH-7137)
    800415e

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 11, 2018

    New changeset 8a05f55 by Terry Jan Reedy in branch 'master':
    bpo-33656: Add entry to What's New 3.7. (GH-7638)
    8a05f55

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Jun 11, 2018

    New changeset 144a867 by Miss Islington (bot) in branch '3.7':
    bpo-33656: On Windows, add API call saying that tk scales for DPI (GH-7137)
    144a867

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Jun 11, 2018

    New changeset 0404d35 by Miss Islington (bot) in branch '3.6':
    bpo-33656: On Windows, add API call saying that tk scales for DPI (GH-7137)
    0404d35

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 11, 2018

    Thanks everyone for the help. I think this is the right patch, but as with many IDLE patches, it is hard to be sure until it is in use. At least there is no question here of tk Windows-Linux-MacOS differences. I checked that the Win 7 buildbots are OK with the PR.

    Serhiy: My interpretation of the tk scaling doc is that 1, not 2, is the right argument. But we can test to be sure. I meant 'spare monitor', not 'space monitor'. But I need to find one with a substantially different DPI.

    Steve: changing the binaries' manifests is a separate issue, not limited to IDLE or even tkinter. And 3.8 enhancements do nothing for IDLE on 3.6 and 3.7. Ditto for a new WinAPI.

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 11, 2018

    Reopening, temporarily, to add change requested by Eryk Sun after the merge.

    @terryjreedy terryjreedy reopened this Jun 11, 2018
    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 11, 2018

    New changeset fd88f31 by Terry Jan Reedy in branch 'master':
    bpo-33656: Add enum name for argument of Windows call. (GH-7642)
    fd88f31

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Jun 11, 2018

    New changeset d26277a by Miss Islington (bot) in branch '3.7':
    bpo-33656: Add enum name for argument of Windows call. (GH-7642)
    d26277a

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Jun 11, 2018

    New changeset afa1ea5 by Miss Islington (bot) in branch '3.6':
    bpo-33656: Add enum name for argument of Windows call. (GH-7642)
    afa1ea5

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 11, 2018

    Since there should be time before .rcs are cut, I changed the idlelib NEWS.txt entry to mention color changes, based on editing with the change in place.

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 11, 2018

    New changeset 4b704f2 by Terry Jan Reedy in branch 'master':
    bpo-33656: Mention color in idlelib/NEWS.txt entry. (bpo-7646)
    4b704f2

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 11, 2018

    New changeset 6530577 by Terry Jan Reedy (Miss Islington (bot)) in branch '3.7':
    bpo-33656: Mention color in idlelib/NEWS.txt entry. (GH-7646) (GH-7647)
    6530577

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Jun 11, 2018

    New changeset 2023eaf by Miss Islington (bot) in branch '3.6':
    bpo-33656: Mention color in idlelib/NEWS.txt entry. (GH-7646)
    2023eaf

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jun 12, 2018

    I can't test on multiple monitors, but I tested on a single HiDPI monitor. With 1 it looks much better than with 0. With 2 it looks the same as with 1 on HiDPI, but doesn't scale well after changing the resolution of the display. I suppose there are the same problems on multi-monitor configuration. Thus the value 1 is the correct one.

    ctypes is optional. It would be better to catch an ImportError from importing it.

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 12, 2018

    I confirmed that IDLE did not import ctypes before this issue. Serhiy, I presume that the new code can be skipped, you are suggesting replacing

    import ctypes
    <use ctypes....>

    with

    try:
    import ctypes
    except ImportError:
    pass # The ctypes-using fix is not essential.
    else:
    <use ctypes...>

    But, in what sense is ctypes optional *on Windows* more than most other modules? I don't see anything in the ctypes doc. It is normal to depend on stdlib modules being present and not wrap imports.

    Steve: does the Windows installer have an option to omit ctypes? I don't remember one.

    Anyone: do you know of any CPython Windows distributions that we care about that omit ctypes?

    Which of the following best describes the situation?

    1. This is a theoretical concern that does not justify adding noise to the code.
    2. This is a real concern, but so rare that the fix can be deferred to 3.6.7 and 3.7.1.
    3. This is likely common enough that we should ask Ned to cherry-pick the patch into 3.6.6 and 3.7.1.

    @terryjreedy terryjreedy reopened this Jun 12, 2018
    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Jun 12, 2018

    I just ran the 3.7.0rc1 installer and here is no option to omit ctypes.

    @zooba
    Copy link
    Member

    @zooba zooba commented Jun 12, 2018

    There's no option to omit ctypes, though I am aware of some custom builds of Python that exclude it. I don't think you need to worry about those being used for IDLE (but of course the general policy of the stdlib not relying on ctypes remains).

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jun 13, 2018

    I don't know whether this is a theoretical or a real concern, but I think this fix can wait for 3.7.1.

    @taleinat
    Copy link
    Contributor

    @taleinat taleinat commented Oct 13, 2018

    Ping? Do we want to get this in for 3.7.1 and 3.6.7?

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Oct 13, 2018

    There is still no rush as guarding the ctypes import on Windows builds that can run IDLE may never be needed. But I grepped the stdlib .py code and uuid is the only .py module that imports ctypes, and all are directly or indirectly guarded in the code or test_uuid. So I moved the import inside the existing try block and added ImportError to those caught.

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy terryjreedy commented Oct 14, 2018

    New changeset d274afb by Terry Jan Reedy in branch 'master':
    bpo-33656: Move pyshell ctypes import inside try block. (GH-9858)
    d274afb

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Oct 14, 2018

    New changeset 77e0abe by Miss Islington (bot) in branch '3.7':
    bpo-33656: Move pyshell ctypes import inside try block. (GH-9858)
    77e0abe

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Oct 14, 2018

    New changeset 6829930 by Miss Islington (bot) in branch '3.6':
    bpo-33656: Move pyshell ctypes import inside try block. (GH-9858)
    6829930

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 3.8 expert-IDLE type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants