Disable hiding window while in MINIMIZED, FULLSCREEN or PRESENTATION state and ignore no-op visibility requests

[proneon267]

As identified in #2096, the behavior of hiding a window while in MINIMIZED, FULLSCREEN or PRESENTATION state, is inconsistent on macOS:

From state	Result on calling window.hide()	Result of then calling window.show()
NORMAL	Window is not visible and window icon is not visible in the dock	Window becomes visible and window icon becomes visible in the dock
MINIMIZED	Window is not visible and window icon is not visible in the dock	Window becomes visible & un-minimized, and window icon becomes visible in the dock
MAXIMIZED	Window is not visible and window icon is not visible in the dock	Window becomes visible and window icon becomes visible in the dock
FULLSCREEN	Screen becomes black	Window becomes visible in fullscreen like before
PRESENTATION	window content is still visible on the screen, but NSWindow.isVisible reports False	window content is still visible on the screen, but NSWindow.isVisible reports True

This behavior is also inconsistent when compared to other platforms. Therefore, to keep the behavior consistent on all platforms and to be able to implement a stable API on #2096, the hiding of window while in MINIMIZED, FULLSCREEN or PRESENTATION state is disabled.

Context:

• Added on_gain_focus, on_lose_focus, on_show & on_hide handlers on toga.Window #2096 (comment)
• Added on_gain_focus, on_lose_focus, on_show & on_hide handlers on toga.Window #2096 (comment)

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

[freakboy3742]

This addresses hide(), but doesn't do anything for show() - which is the actual use case that was a problem in #2096.

[freakboy3742]

Suggested change

	Hiding of a window while in `MINIMIZED`, `FULLSCREEN` or `PRESENTATION` state is now disabled, in order to keep the API behavior consistent on all platforms.
	The `show()` and `hide()` APIs can no longer be used on a window while it is in a `MINIMIZED`, `FULLSCREEN` or `PRESENTATION` state.

[proneon267]

I think we do not need to mention show() here, since calling window.show() while in MINIMIZED, FULLSCREEN or PRESENTATION state, will be a no-op(as window.visible would be true in these cases), and window.show() isn't specifically disabled for MINIMIZED, FULLSCREEN or PRESENTATION cases.

[freakboy3742]

I agree it would be exceptionally difficult to get into this state, but it is possible if you invoke system native APIs.

At the very least, it's useful for completeness/symmetry in the docs.

[proneon267]

This addresses hide(), but doesn't do anything for show() - which is the actual use case that was a problem in #2096.

Yes, I hadn't changed show(), because I had thought calling show() on an already visible window would be no-op, but on macOS, the show() call was still calling makeKeyAndOrderFront_ even when called on a already shown window. So, I have blocked calls to the _impl show()/hide() methods when the window is already in the requested visibility state.

[proneon267]

The previously form of this test was:

toga/core/tests/window/test_window.py

Lines 265 to 274 in fbedd3f

	def test_hide_show(window, app):
	"""The window can be hidden then shown."""
	assert window.app == app
	window.hide()
	# The window has been assigned to the app, and is not visible
	assert window.app == app
	assert window in app.windows
	assert_action_performed(window, "hide")
	assert not window.visible

Since windows are not visible at the start(since window.show() hadn't been called). Therefore assert_action_performed(window, "hide") on line 273 would not be performed, as the call to window.hide() will be a no-op(since window was already in hidden state.).

Hence, I have added the window.show() call at the start.

[freakboy3742]

The fact that windows wasn't visible at the start of the test was literally this point of this test - to confirm that invoking hide() before show() didn't do anything internally stateful.

That's no longer possible/applicable because invoking hide() on a non-visible window now has different (state-specific) behavior.

So - test_hide_show is no longer required. But the test you've added as "test_visilibity_noop" isn't testing the visibility property - it's checking the stateful behavior of show() and hide().

These three tests (test_show_hide, test_hide_show, and test_visibility_noop) should be merged, testing the "show, show (no-op), hide, hide (no-op), show" sequence. The visibility test then only needs to verify that assigning visibile = True is an analog of show(), and visible = False is an analog of hide().

[proneon267]

Apologies for requesting a review, but it seems that I need to do more tests on iOS.

[proneon267]

Turns out the problem was that on iOS window.visible always returned True. But the UIWindow is hidden as default by the system, unless makeKeyAndVisible has been called on the UIWindow.

Requesting the same visibility as the current visibility state is a no-op and is ignored at the core level. But, the no-op checking at the core uses window.visible to detect it, so window.visible always returning True on iOS caused the UIWindow to never be shown.

I have fixed the issue by actually checking if the window is hidden or not with UIWindow.isHidden().

[freakboy3742]

Yes, I hadn't changed show(), because I had thought calling show() on an already visible window would be no-op, but on macOS, the show() call was still calling makeKeyAndOrderFront_ even when called on a already shown window. So, I have blocked calls to the _impl show()/hide() methods when the window is already in the requested visibility state.

Yes... and that's a change in behavior that has been the direct cause of a known bug/design inconsistency. This is literally why we write regression tests.

Yes, the test that calling show() on a visible window exists in this PR, with 100% coverage of the code base. What we're trying to validate is that no matter what happens to the implementation, the same behavior is preserved. The interaction of window state is a key part of the bug as observed in the wild, Adding a test to make sure that when those same conditions exist the problem no longer occurs will not increase coverage - but it does validate that a specific problem that has been observed in the wild won't re-occur.

[freakboy3742]

The fact that windows wasn't visible at the start of the test was literally this point of this test - to confirm that invoking hide() before show() didn't do anything internally stateful.

That's no longer possible/applicable because invoking hide() on a non-visible window now has different (state-specific) behavior.

So - test_hide_show is no longer required. But the test you've added as "test_visilibity_noop" isn't testing the visibility property - it's checking the stateful behavior of show() and hide().

These three tests (test_show_hide, test_hide_show, and test_visibility_noop) should be merged, testing the "show, show (no-op), hide, hide (no-op), show" sequence. The visibility test then only needs to verify that assigning visibile = True is an analog of show(), and visible = False is an analog of hide().

[freakboy3742]

I agree it would be exceptionally difficult to get into this state, but it is possible if you invoke system native APIs.

At the very least, it's useful for completeness/symmetry in the docs.

[proneon267]

I have merged the 3 core tests. test_visibility was already checking that visible=True/False is an analog of calling show()/hide(), so I haven't changed it further.

[freakboy3742]

You haven't added validation and tests for the show() case like I asked in my last review; but the tests are easy to add, so I've done it for you rather than go round another iteration of reviews.

Otherwise, this now looks good.

[proneon267]

Thanks for adding them. I had thought that you had meant of keeping the symmetry only in the form of documentation, and not actually raise the error on show(). I will be more thorough with the reviews, from next time.