bpo-37039: Make IDLE's Zoom Height adjust to users' screens #13678

taleinat · 2019-05-30T09:59:59Z

When "zoom height" is first activated, this implementation measures the window manager's size for maximized windows by maximizing IDLE, and from that point uses that size. Subsequent activations will use the cached value for the dimensions of maximized windows.

This avoids hard-coded values which aren't always correct, e.g. because docks/taskbars can be resized, moved or hidden and window-manager themes can be customized.

This does cause a one-time visual artifact on macOS, where on the first activation, the window is maximized, un-maximized, and then set to the maximum height. Subsequent activation uses cached information, so this doesn't happen again.

https://bugs.python.org/issue37039

aroberge · 2019-05-30T10:19:38Z

On my computer (Windows 10, resolution: 3000 x 2000, scaling of UI elements set to 200%), clicking on Zoom Height makes Idle go full screen. Clicking on the same menu (now called Restore Height) does absolutely nothing: Idle stays in full screen mode.

If I run the program on a secondary monitor (4k), the height only changes, but nowhere enough to be equal to the height of the screen.

csabella · 2019-05-30T11:53:04Z

On Windows 10, I'm seeing the same thing as @aroberge, but there's some more. I'll enumerate the steps.

Start IDLE.
Zoom Height -- window now takes up full screen.
Restore Height -- nothing happens.
Use Maximize/Minimize from title bar -- window goes back to original size and location.
Zoom Height -- window expands past bottom of screen, but top of the screen doesn't move. The status bar (and bottom of the editor frame) cannot be seen.
Restore Height -- restores to original size and location.
Moving the window around (that is, where the top of the screen is above or below the original), the window always 'snaps' to the same top, which is where is was originally opened.

Another path:

Start IDLE.
Maximize window using the title bar icon.
Menu says 'Zoom Height', but selecting it does nothing.
Continue from step 4 above.

This seems like a different bug/enhancement (to not allow zooming when the window is maximized), but I wanted to mention it since I was checking the behavior.

Thanks!

1. Tk's wm_geometry() doesn't report the actual position for maximized windows, but thankfully does report their actual dimensions. 2. The title bar height is different for normal vs. maximized windows.

Also clean up code and add comments where needed.

taleinat · 2019-06-04T20:32:13Z

I've fixed three different issues on Windows 10, including those brought up here. I've also verified that it still works as intended on macOS (10.14.4).

@aroberge, @csabella, would you mind giving this another go?

aroberge · 2019-06-04T21:02:00Z

It works correctly on my system.

It is a bit surprising to see the window taking the full screen for a second and then having its width reduced again, but it only does that when changing the zoom status for the first time. Moving the window to a second monitor with a different resolution after having clicked on zoom height at least once results in the zoom height feature not working properly as it remembers the settings from the first window. However, if one first moves the window to a second monitor and then clicks on the button, it works.

taleinat · 2019-06-05T05:13:00Z

Moving the window to a second monitor with a different resolution after having clicked on zoom height at least once results in the zoom height feature not working properly as it remembers the settings from the first window.

This is true and very unfortunate. Many people use multiple monitors with different resolutions these days, so this should be considered a common scenario.

To avoid this, we could check the screen dimensions and only cache values for screens with identical dimensions. We'll also need to properly handle screens with identical resolutions but different scaling settings.

terryjreedy · 2019-06-06T04:50:57Z

The test failures were all for test_enum. False positives with respect to this patch. Since test_idle passed on my machine, no need to retest with close/open -- wait until another commit.

terryjreedy · 2019-06-06T05:15:15Z

Testing and commenting without looking at the code yet. There seem to be three issues, starting with what I think is the highest priority.

Initial full-screen flash: really annoying. If we cannot find something we have missed so far (I don't expect this), it might be better to pause it a bit. Idea: maximize the root window with text such as "Calibrating zoom". Or do this on startup as a splash screen with text such as "IDLE is measuring\nyour screen." The latter might require pulling startup code from pyshell into a separate startup file that does not immediately import all of idlelib. I don't currently know where the startup time goes, and if the time between creating root and displaying a shell or editor screen could be made long enough with pyshell as startup.

After this, Zoom and Restore work as they should. Thanks Tal. I think reducing the flash annoyance would be enough to merge.

User maximizes the window and then 'zooms'. Nothing happens as the window is already zoomed and zoom/restore do not change width. 'Zoom' is not changed to 'Restore'. Perhaps a comparison needs to remove '='? Perhaps when no calibration is stored, max can be stored and 'zoom' flipped?
Moving between a window between multiple screens. Given multiple screens, how common is this? We should be able to identify screens sufficiently by size and scaling. It does suggest that a startup check is not enough, and that both zoom and scale calibration need to be per window. Also, that user should be able to initiate recalibration for a particular window.

taleinat · 2019-06-06T10:38:23Z

The newest change calculates and caches the needed info once per screen resolution.

This will still misbehave slightly in truly rare edge-cases, such as changing the size or position of the taskbar/dock.

taleinat · 2019-06-06T10:56:08Z

Initial full-screen flash: really annoying. If we cannot find something we have missed so far (I don't expect this), it might be better to pause it a bit. Idea: maximize the root window with text such as "Calibrating zoom". Or do this on startup as a splash screen with text such as "IDLE is measuring\nyour screen." The latter might require pulling startup code from pyshell into a separate startup file that does not immediately import all of idlelib. I don't currently know where the startup time goes, and if the time between creating root and displaying a shell or editor screen could be made long enough with pyshell as startup.

IMO the number of users who actually use "zoom height" is small, so I don't think this justifies annoying 100% of the users with a maximized (i.e. almost full-screen) splash screen.

I've been working around and through the limitations of Tk as far as possible, and have reached the limit of what is possible, to the best of my understanding and research. To do better would require using OS APIs directly, e.g. as done by the screeninfo library, but IMO this is not warranted in this case.

User maximizes the window and then 'zooms'. Nothing happens as the window is already zoomed and zoom/restore do not change width. 'Zoom' is not changed to 'Restore'. Perhaps a comparison needs to remove '='? Perhaps when no calibration is stored, max can be stored and 'zoom' flipped?

I've currently made "zoom height" be a no-op when the window isn't in the "normal" state, e.g. when it is maximized. I can handle the case where the screen is already maximized if you find the current "do nothing" behavior surprising. Or I can just add a .bell() call to make it clear that the command failed.

Moving between a window between multiple screens. Given multiple screens, how common is this?

These days, with widespread use of laptops, it is very common: People connect and disconnect to/from external screens, using them instead of/in addition to the laptop screen. Also, multi-monitor desktop setups are increasingly common for developers, and often they do not have the same resolution, e.g. having one monitor situated in a "portrait" orientation.

... We should be able to identify screens sufficiently by size and scaling. It does suggest that a startup check is not enough, and that both zoom and scale calibration need to be per window.

The latest commit in the PR handles this properly. (Note that as far as Tk is concerned, scaling simply means a different resolution.)

terryjreedy

I addressed my points 1 and 2 in the comments. Without a multiscreen setup, I cannot comment much on 3. I am willing to have zoomheight be imperfect if someone changes their taskbar or screen settings. I will add this also to the doc, something like "After the first zoom on a screen, zoomheight assumes that taskbar and screen settings remain the same."

I have not yet looked at the rest of the code of zoom_height and that of get_max..., but I expect this should get into 3.7.4b1 on 6/17.

Lib/idlelib/zoomheight.py

terryjreedy · 2019-06-06T17:07:45Z

Lib/idlelib/zoomheight.py

+        if self.top.wm_state() != 'normal':
+            # Can't zoom/restore window height for windows not in the 'normal'
+            # state, e.g. maximized and full-screen windows.
+            return None


I now agree that this is correct. Previously, touching the window when maximized messed up the behavior of the OS WM Restore button (at least on Windows, which is enough). Besides which, restoring only the height and not the width would be wrong and restoring both would be redundant. So add top.beep().

This should be added to the doc and blurb. I plan to edit both when code is settled.

terryjreedy · 2019-06-06T17:16:39Z

Lib/idlelib/zoomheight.py

-    newgeom = '' if height >= newheight else f"{width}x{newheight}+{x}+{newy}"
-    top.wm_geometry(newgeom)
-    return newgeom != ""
+    return width, height, x, y


Just return map... and unpack where used.

terryjreedy · 2019-06-06T17:25:22Z

Lib/idlelib/zoomheight.py

+        return self._max_height_and_y_coords[screen_dimensions]
+
+
+def get_window_geometry(top):
    geom = top.wm_geometry()
    m = re.match(r"(\d+)x(\d+)\+(-?\d+)\+(-?\d+)", geom)
    if not m:
        top.bell()
        return


By PEP 8, this should be return None because of explicit return below. But unpacking None at the call site will crash IDLE anyway. So we might as well remove the check here.

https://www.tcl.tk/man/tcl8.6/TkCmd/tkvars.htm says
"The following variables are only guaranteed to exist in wish executables; the Tk library does not define them itself but many Tk environments do.
geometry
If set, contains the user-supplied geometry specification to use for the main Tk window. "

idlelib, including tests, is loaded with unguarded geometry calls, so IDLE and the test suite would fail on any tk without geometry. So there must be none on systems Python and IDLE currently run on.

Note that this part of the code is unchanged...

If the existing norm is not guarding against geometry() returning bad values, I'll remove the check.

bedevere-bot · 2019-06-06T17:40:09Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be poked with soft cushions!

taleinat · 2019-06-06T21:39:35Z

I have made the requested changes; please review again.

taleinat · 2019-06-17T09:08:03Z

Misc/NEWS.d/next/IDLE/2019-06-04-23-27-33.bpo-37039.FN_fBf.rst

+Adjust "Zoom Height" zooms to individual screens by momemtarily maximizing
+the window the first time one is zoomed on a particular screen.  Zooms will
+not be correct if the resolution and scaling for a screen is subsequently
+changed.  While a window is maximized, "Zoom Height" has no effect.


As far as I can tell, changing the resolution and/or scaling of a screen will simply cause Tk to observe a different screen resolution, after which the current implementation will again maximize a screen once to get the proper values for the new screen configuration. So this is incorrect: "Zooms will not be correct if the resolution and scaling for a screen is subsequently changed."

The only cases where we could get things slightly wrong is when the dock/taskbar is moved or changed, or perhaps moving a window to a screen with identical effective resolution but with other differences, e.g. with/without a dock/taskbar.

Of course. The dict key will change and not be found. I will simplify to "Zooms may not be correct if the screen is subsequently changed." and then merge.

taleinat · 2019-06-17T09:10:33Z

@terryjreedy, indeed this appears ready to merge IMO, apart for my comment regarding the NEWS entry.

miss-islington · 2019-06-17T19:48:47Z

Thanks @taleinat for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2019-06-17T19:48:47Z

Thanks @taleinat for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington · 2019-06-17T19:48:52Z

Sorry @taleinat and @terryjreedy, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 5bff3c86ab77e9d831b3cd19b45654c7eef22931 3.8

bedevere-bot · 2019-06-17T19:49:05Z

GH-14167 is a backport of this pull request to the 3.7 branch.

…-13678) Measure required height by quickly maximizing once per screen. A search for a better method failed. (cherry picked from commit 5bff3c8) Co-authored-by: Tal Einat <taleinat+github@gmail.com>

miss-islington · 2019-06-17T19:51:54Z

Thanks @taleinat for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

bedevere-bot · 2019-06-17T19:52:08Z

GH-14168 is a backport of this pull request to the 3.8 branch.

…-13678) Measure required height by quickly maximizing once per screen. A search for a better method failed. (cherry picked from commit 5bff3c8) Co-authored-by: Tal Einat <taleinat+github@gmail.com>

Measure required height by quickly maximizing once per screen. A search for a better method failed. (cherry picked from commit 5bff3c8) Co-authored-by: Tal Einat <taleinat+github@gmail.com>

GH-14168) Measure required height by quickly maximizing once per screen. A search for a better method failed. (cherry picked from commit 5bff3c8) Co-authored-by: Tal Einat <taleinat+github@gmail.com>

…-13678) Measure required height by quickly maximizing once per screen. A search for a better method failed.

bpo-37039: Make IDLE's Zoom Height behave better across platforms

dd16ea5

taleinat requested review from terryjreedy and csabella May 30, 2019 09:59

the-knights-who-say-ni added the CLA signed label May 30, 2019

bedevere-bot added the awaiting core review label May 30, 2019

brettcannon added the type-bug An unexpected behavior, bug, or error label May 30, 2019

fixed two zoomheight issues on Win10

aaa873b

1. Tk's wm_geometry() doesn't report the actual position for maximized windows, but thankfully does report their actual dimensions. 2. The title bar height is different for normal vs. maximized windows.

taleinat self-assigned this Jun 4, 2019

taleinat added 2 commits June 4, 2019 23:11

handle edge-case of default height > max height

9aeb031

Also clean up code and add comments where needed.

add NEWS

c9f77d1

gracefully do nothing when the WM doesn't support the "zoomed" state

9a88520

get required window dimension info one for each screen resolution

f64ff40

terryjreedy requested changes Jun 6, 2019

View reviewed changes

bedevere-bot removed the awaiting core review label Jun 6, 2019

bedevere-bot added the awaiting changes label Jun 6, 2019

taleinat added 2 commits June 7, 2019 00:38

address code review comments

2add7ee

fix geometry formatting in set_window_geometry()

2e91cb9

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 6, 2019

taleinat commented Jun 17, 2019

View reviewed changes

Edit blurb and docs.

6b05d1e

terryjreedy approved these changes Jun 17, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jun 17, 2019

terryjreedy changed the title ~~bpo-37039: Make IDLE's Zoom Height behave better across platforms~~ bpo-37039: Make IDLE's Zoom Height behave better across screens Jun 17, 2019

terryjreedy changed the title ~~bpo-37039: Make IDLE's Zoom Height behave better across screens~~ bpo-37039: Make IDLE's Zoom Height adjust to users' screens Jun 17, 2019

terryjreedy merged commit 5bff3c8 into python:master Jun 17, 2019

bedevere-bot removed the awaiting merge label Jun 17, 2019

terryjreedy added needs backport to 3.7 needs backport to 3.8 only security fixes labels Jun 17, 2019

miss-islington assigned terryjreedy and unassigned taleinat Jun 17, 2019

bedevere-bot removed the needs backport to 3.7 label Jun 17, 2019

terryjreedy added needs backport to 3.8 only security fixes and removed needs backport to 3.8 only security fixes labels Jun 17, 2019

bedevere-bot removed the needs backport to 3.8 only security fixes label Jun 17, 2019

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019

bpo-37039: Make IDLE's Zoom Height adjust to users' screens (pythonGH…

225756d

…-13678) Measure required height by quickly maximizing once per screen. A search for a better method failed.

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020

bpo-37039: Make IDLE's Zoom Height adjust to users' screens (pythonGH…

210d606

…-13678) Measure required height by quickly maximizing once per screen. A search for a better method failed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-37039: Make IDLE's Zoom Height adjust to users' screens #13678

bpo-37039: Make IDLE's Zoom Height adjust to users' screens #13678

taleinat commented May 30, 2019 •

edited

Loading

aroberge commented May 30, 2019

csabella commented May 30, 2019

taleinat commented Jun 4, 2019

aroberge commented Jun 4, 2019

taleinat commented Jun 5, 2019

terryjreedy commented Jun 6, 2019

terryjreedy commented Jun 6, 2019 •

edited

Loading

taleinat commented Jun 6, 2019

taleinat commented Jun 6, 2019 •

edited

Loading

terryjreedy left a comment

terryjreedy Jun 6, 2019

terryjreedy Jun 6, 2019

terryjreedy Jun 6, 2019

taleinat Jun 6, 2019

bedevere-bot commented Jun 6, 2019

taleinat commented Jun 6, 2019

taleinat Jun 17, 2019

terryjreedy Jun 17, 2019

taleinat commented Jun 17, 2019 •

edited

Loading

miss-islington commented Jun 17, 2019

miss-islington commented Jun 17, 2019

miss-islington commented Jun 17, 2019

bedevere-bot commented Jun 17, 2019

miss-islington commented Jun 17, 2019

bedevere-bot commented Jun 17, 2019

bpo-37039: Make IDLE's Zoom Height adjust to users' screens #13678

bpo-37039: Make IDLE's Zoom Height adjust to users' screens #13678

Conversation

taleinat commented May 30, 2019 • edited Loading

aroberge commented May 30, 2019

csabella commented May 30, 2019

taleinat commented Jun 4, 2019

aroberge commented Jun 4, 2019

taleinat commented Jun 5, 2019

terryjreedy commented Jun 6, 2019

terryjreedy commented Jun 6, 2019 • edited Loading

taleinat commented Jun 6, 2019

taleinat commented Jun 6, 2019 • edited Loading

terryjreedy left a comment

Choose a reason for hiding this comment

terryjreedy Jun 6, 2019

Choose a reason for hiding this comment

terryjreedy Jun 6, 2019

Choose a reason for hiding this comment

terryjreedy Jun 6, 2019

Choose a reason for hiding this comment

taleinat Jun 6, 2019

Choose a reason for hiding this comment

bedevere-bot commented Jun 6, 2019

taleinat commented Jun 6, 2019

taleinat Jun 17, 2019

Choose a reason for hiding this comment

terryjreedy Jun 17, 2019

Choose a reason for hiding this comment

taleinat commented Jun 17, 2019 • edited Loading

miss-islington commented Jun 17, 2019

miss-islington commented Jun 17, 2019

miss-islington commented Jun 17, 2019

bedevere-bot commented Jun 17, 2019

miss-islington commented Jun 17, 2019

bedevere-bot commented Jun 17, 2019

taleinat commented May 30, 2019 •

edited

Loading

terryjreedy commented Jun 6, 2019 •

edited

Loading

taleinat commented Jun 6, 2019 •

edited

Loading

taleinat commented Jun 17, 2019 •

edited

Loading