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

Add window refresh lock to block window from updating while handler is running. #2955

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions changes/2955.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Windows now have a refresh lock to stop refreshing window layout happening after every add()
4 changes: 4 additions & 0 deletions core/src/toga/widgets/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,10 @@ def enabled(self, value: bool) -> None:
self._impl.set_enabled(bool(value))

def refresh(self) -> None:
if self.window is not None and self.window.locked:
self.window.dirty(self)
return

self._impl.refresh()

# Refresh the layout
Expand Down
37 changes: 37 additions & 0 deletions core/src/toga/window.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from toga.types import PositionT, SizeT
from toga.widgets.base import Widget

from contextlib import contextmanager

_window_count = -1

Expand Down Expand Up @@ -190,6 +191,8 @@ def __init__(
self._content: Widget | None = None
self._is_full_screen = False
self._closed = False
self._locked = False
self._dirty = set()

self._resizable = resizable
self._closable = closable
Expand Down Expand Up @@ -353,6 +356,8 @@ def content(self) -> Widget | None:
def content(self, widget: Widget) -> None:
# Set window of old content to None
if self._content:
if self._content in self._dirty:
self._dirty.remove(self._content)
self._content.window = None

# Assign the content widget to the same app as the window.
Expand Down Expand Up @@ -393,6 +398,38 @@ def size(self, size: SizeT) -> None:
if self.content:
self.content.refresh()

def dirty(self, widget: Widget) -> None:
"""Mark widget as being 'dirty', so that it gets refreshed
when the window lock is lifted
"""
self._dirty.add(widget)

@property
def locked(self) -> bool:
"""Is the window locked from refreshes?"""
return self._locked

@locked.setter
def locked(self, locked: bool) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

This flag definitely shouldn't be writable. A user shouldn't be able to inadvertently change the locking state. A getter makes sense; but I can't see a use case for releasing the lock being a public API (outside of the context manager)

"""Lock or unlock window refresh.
When window is unlocked, refresh the content
"""
self._locked = locked
if not locked:
for child in self._dirty:
child.refresh()

self._dirty.clear()

@contextmanager
def refresh_lock(self, *args, **kwargs):
"""Obtain a refresh lock on the window. Unlocks
automatically when context manager leaves scope.
"""
self.locked = True
yield
self.locked = False

######################################################################
# Window position
######################################################################
Expand Down
14 changes: 7 additions & 7 deletions core/tests/test_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def test_noop_handler_with_cleanup_error(capsys):


def test_function_handler():
"""A function can be used as a handler."""
"""A function can be used as a handler"""
Copy link
Member

Choose a reason for hiding this comment

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

Why all these updates to remove punctuation?

obj = Mock()
handler_call = {}

Expand All @@ -86,7 +86,7 @@ def handler(*args, **kwargs):


def test_function_handler_error(capsys):
"""A function handler can raise an error."""
"""A function handler can raise an error"""
obj = Mock()
handler_call = {}

Expand Down Expand Up @@ -116,7 +116,7 @@ def handler(*args, **kwargs):


def test_function_handler_with_cleanup():
"""A function handler can have a cleanup method."""
"""A function handler can have a cleanup method"""
obj = Mock()
cleanup = Mock()
handler_call = {}
Expand Down Expand Up @@ -180,7 +180,7 @@ def handler(*args, **kwargs):


def test_generator_handler(event_loop):
"""A generator can be used as a handler."""
"""A generator can be used as a handler"""
obj = Mock()
handler_call = {}

Expand Down Expand Up @@ -213,7 +213,7 @@ def handler(*args, **kwargs):


def test_generator_handler_error(event_loop, capsys):
"""A generator can raise an error."""
"""A generator can raise an error"""
obj = Mock()
handler_call = {}

Expand Down Expand Up @@ -248,7 +248,7 @@ def handler(*args, **kwargs):


def test_generator_handler_with_cleanup(event_loop):
"""A generator can have cleanup."""
"""A generator can have cleanup"""
obj = Mock()
cleanup = Mock()
handler_call = {}
Expand Down Expand Up @@ -285,7 +285,7 @@ def handler(*args, **kwargs):


def test_generator_handler_with_cleanup_error(event_loop, capsys):
"""A generator can raise an error during cleanup."""
"""A generator can raise an error during cleanup"""
obj = Mock()
cleanup = Mock(side_effect=Exception("Problem in cleanup"))
handler_call = {}
Expand Down
87 changes: 87 additions & 0 deletions core/tests/window/test_window.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
assert_action_not_performed,
assert_action_performed,
assert_action_performed_with,
performed_actions,
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to see this in the form of an assert_action_count (or better still, an assert_ action_performed(count=N)).

)


Expand Down Expand Up @@ -172,6 +173,92 @@ def test_change_content(window, app):
assert content1.window is None


def test_change_content_locked(window, app):
"""The refresh of a window can be locked"""
with window.refresh_lock():
assert window.content is None
assert window.app == app

# Set the content of the window
content1 = toga.Box()
window.content = content1

# The content has been assigned and not (yet) refreshed
assert content1.app == app
assert content1.window == window
assert_action_performed_with(window, "set content", widget=content1._impl)
assert_action_not_performed(content1, "refresh")

# Set the content of the window to something new
content2 = toga.Box()
window.content = content2

# The content has been assigned and not (yet) refreshed
assert content2.app == app
assert content2.window == window
assert_action_performed_with(window, "set content", widget=content2._impl)
assert_action_not_performed(content2, "refresh")

# The original content has been removed
assert content1.window is None

# Action refresh must not have been performed on content1
assert_action_not_performed(content1, "refresh")

# Action refresh must have been performed on content2
assert_action_performed(content2, "refresh")
assert len(performed_actions(content2, "refresh")) == 2


def test_change_content_locked_normal_refreshes(window, app):
"""When a window is locked, a lot less refreshes would be called..."""
assert window.content is None
assert window.app == app
boxes = []

# Set the content of the window
content1 = toga.Box()
window.content = content1

for w in range(0, 200):
boxes.append(toga.Box())
content1.add(boxes[-1])

# Action refresh must have been performed on content1
assert_action_performed(content1, "refresh")
assert len(performed_actions(content1, "refresh")) == 402

# Action refresh must have been performed on all boxes
for box in boxes:
assert_action_performed(box, "create Box")
assert len(performed_actions(box, "create Box")) == 1


def test_change_content_locked_less_refreshes(window, app):
"""When a window is locked, a lot less refreshes are called..."""
with window.refresh_lock():
assert window.content is None
assert window.app == app
boxes = []

# Set the content of the window
content1 = toga.Box()
window.content = content1

for w in range(0, 200):
boxes.append(toga.Box())
content1.add(boxes[-1])

# Action refresh must have been performed on content1
assert_action_performed(content1, "refresh")
assert len(performed_actions(content1, "refresh")) == 2

# Action refresh must have been performed on all boxes
for box in boxes:
assert_action_performed(box, "create Box")
assert len(performed_actions(box, "create Box")) == 1


def test_set_position(window):
"""The position of the window can be set."""
window.position = (123, 456)
Expand Down
13 changes: 13 additions & 0 deletions dummy/src/toga_dummy/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,19 @@ def layout(self, root, viewport):
###############################################################################


def performed_actions(_widget, _action):
"""Retrieve the specified actions executed by the widget

:param _widget: The interface of the widget to check
:param _action: The action.
:returns: The events log entries associated with the widget/action pair.
"""
try:
return EventLog.performed_actions(_widget, _action)
except AttributeError:
return ()


def attribute_value(_widget, _attr):
"""Retrieve the current value of a widget property.

Expand Down