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

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

Merged
1 change: 1 addition & 0 deletions changes/3109.removal.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
The `show()` and `hide()` APIs can no longer be used on a window while it is in a `MINIMIZED`, `FULLSCREEN` or `PRESENTATION` state.
19 changes: 16 additions & 3 deletions core/src/toga/window.py
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,8 @@ def closed(self) -> bool:
def show(self) -> None:
"""Show the window. If the window is already visible, this method has no
effect."""
self._impl.show()
if not self.visible:
self._impl.show()

######################################################################
# Window content and resources
Expand Down Expand Up @@ -436,8 +437,20 @@ def screen_position(self, position: PositionT) -> None:

def hide(self) -> None:
"""Hide the window. If the window is already hidden, this method has no
effect."""
self._impl.hide()
effect.

:raises ValueError: If the window is currently in a minimized, full screen or
presentation state.
"""
if self.state in {
WindowState.MINIMIZED,
WindowState.FULLSCREEN,
WindowState.PRESENTATION,
}:
raise ValueError(f"A window in {self.state} state cannot be hidden.")
else:
if self.visible:
self._impl.hide()

@property
def visible(self) -> bool:
Expand Down
64 changes: 64 additions & 0 deletions core/tests/window/test_window.py
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ def test_show_hide(window, app):

def test_hide_show(window, app):
"""The window can be hidden then shown."""
window.show()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previously form of this test was:

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.

Copy link
Member

Choose a reason for hiding this comment

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

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().

assert window.app == app
window.hide()

Expand Down Expand Up @@ -304,6 +305,69 @@ def test_visibility(window, app):
assert not window.visible


def test_visibility_no_op(window, app):
"""Changing visibility when already in that visibility state is a no-op."""
# Show the window
window.show()
assert window.visible
assert_action_performed(window, "show")
EventLog.reset()

window.show()
assert window.visible
assert_action_not_performed(window, "show")

window.visible = True
assert window.visible
assert_action_not_performed(window, "show")

# Hide the window
window.hide()
assert not window.visible
assert_action_performed(window, "hide")
EventLog.reset()

window.hide()
assert not window.visible
assert_action_not_performed(window, "hide")

window.visible = False
assert not window.visible
assert_action_not_performed(window, "hide")


@pytest.mark.parametrize(
"state",
[
WindowState.MINIMIZED,
WindowState.FULLSCREEN,
WindowState.PRESENTATION,
],
)
def test_hide_disallowed_on_window_state(window, app, state):
"""A window in MINIMIZED, FULLSCREEN or PRESENTATION state cannot be
set to hidden."""
window.show()

window.state = state
assert window.state == state
assert window.visible is True

with pytest.raises(
ValueError,
match=f"A window in {state} state cannot be hidden.",
):
window.hide()
assert_action_not_performed(window, "hide")

with pytest.raises(
ValueError,
match=f"A window in {state} state cannot be hidden.",
):
window.visible = False
assert_action_not_performed(window, "hide")


@pytest.mark.parametrize(
"initial_state, final_state",
[
Expand Down
10 changes: 7 additions & 3 deletions dummy/src/toga_dummy/window.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ def __init__(self, interface, title, position, size):
self.set_size(size)

self._state = WindowState.NORMAL
self._visible = False

######################################################################
# Window properties
Expand All @@ -82,7 +83,7 @@ def set_app(self, app):

def show(self):
self._action("show")
self._set_value("visible", True)
self._visible = True

######################################################################
# Window content and resources
Expand Down Expand Up @@ -122,11 +123,14 @@ def set_position(self, position):
######################################################################

def get_visible(self):
return self._get_value("visible", False)
# We cannot store the visibility value on the EventLog, since the value
# would be cleared on EventLog.reset(), thereby preventing us from
# testing no-op condition of requesting the same visibility as current.
return self._visible

def hide(self):
self._action("hide")
self._set_value("visible", False)
self._visible = False

######################################################################
# Window state
Expand Down
5 changes: 2 additions & 3 deletions testbed/tests/window/test_window.py
Original file line number Diff line number Diff line change
Expand Up @@ -917,10 +917,9 @@ async def test_window_state_content_size_increase(
"state",
[
WindowState.NORMAL,
WindowState.MINIMIZED,
WindowState.MAXIMIZED,
WindowState.FULLSCREEN,
WindowState.PRESENTATION,
# Window cannot be hidden while in MINIMIZED, FULLSCREEN or
# PRESENTATION. So, those states are excluded from this test.
],
)
@pytest.mark.parametrize(
Expand Down
Loading