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

Modify container handling for GTK #1794

Merged
merged 18 commits into from
Mar 5, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions android/src/toga_android/window.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ def width(self):
def height(self):
return self.native.getHeight()

def make_dirty(self, widget=None):
# TODO: This won't be required once we complete the refactor
# making container a separate impl concept.
pass


class Window:
def __init__(self, interface, title, position, size):
Expand Down
1 change: 1 addition & 0 deletions changes/1205.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Issues with reducing the size of windows on GTK have been resolved.
1 change: 1 addition & 0 deletions changes/1794.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
The handling of GTK layouts has been modified to reduce the frequency and increase the accuracy of layout results.
5 changes: 5 additions & 0 deletions cocoa/src/toga_cocoa/window.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ def width(self):
def height(self):
return 0 if self.view is None else self.view.frame.size.height

def make_dirty(self, widget=None):
# TODO: This won't be required once we complete the refactor
# making container a separate impl concept.
pass


class WindowDelegate(NSObject):
interface = objc_property(object, weak=True)
Expand Down
2 changes: 2 additions & 0 deletions core/src/toga/widgets/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,8 @@ def refresh(self):
# We can't compute a layout until we have a viewport
if self._impl.viewport:
super().refresh(self._impl.viewport)
# Refreshing the layout means the viewport needs a redraw.
self._impl.viewport.make_dirty()
Copy link
Member Author

@mhsmith mhsmith Feb 26, 2023

Choose a reason for hiding this comment

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

See top-level review comment.

Copy link
Member

Choose a reason for hiding this comment

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

This is a make_dirty() with no arguments, so it's marking the container as dirty. On GTK, this triggers a "queue_resize" event, which (eventually) causes the actual layout to occur.

In the GTK context, this results in a redundant layout calculation (the one 2 lines above the highlighted line); however, this call is the one that is required on other platforms.

To me, this is part of the problem with "container/viewport" abstraction. The refresh() API entry point is defined on Node, where it requires a viewport definition; but a good portion of the implementation in toga-core replicates the definition from Node so that the viewport can be injected.

The existence of refresh() on toga.core.Widget is also a source of confusion, with people using it as a public facing API in the hope of applying any style change. Admittedly, this has been needed due to various bugs; however, once we've fixed those bugs (and this PR and #1761 fixes a lot of those cases), manual calls to refresh() shouldn't be needed at all.

A lot of what refresh() has historically been used for could actually be replaced with make_dirty() on the container. If the places that are currently calling some_widget.refresh() was replaced with some_widget.container.make_dirty(), that would remove the redundant layout in the GTK case, and other platforms could implement make_dirty() as required (e.g., immediately invoking layout on the root widget of the layout with the appropriate viewport on Cocoa).

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the example you've provided - this make_dirty() won't be needed, because any change on the text widget that marks the text widget as needing a rehint (e.g., changing the text) will also flag the container as dirty, causing a layout. This specific mark_dirty() is needed for the case where a layout is happening but there's no widget-specific rehint requested. One example would be marking the text widget as style.flex=1. That will affect the layout, but not the hinting of a specific widget.

Again, this would be a situation where it would be preferable for the applicator to invoke mark_dirty() directly on the container, and have backends respond with a layout at the appropriate time (immediately on Cocoa; on a redraw for GTK).


def refresh_sublayouts(self):
for child in self.children:
Expand Down
5 changes: 5 additions & 0 deletions dummy/src/toga_dummy/window.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ def width(self):
def height(self):
return self.window.get_size()[1]

def make_dirty(self, widget=None):
# TODO: This won't be required once we complete the refactor
# making container a separate impl concept.
pass


class Window(LoggedObject):
def __init__(self, interface, title, position, size):
Expand Down
199 changes: 199 additions & 0 deletions gtk/src/toga_gtk/container.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
from .libs import Gdk, Gtk


class TogaContainer(Gtk.Fixed):
"""A GTK container widget implementing Toga's layout.

This is a GTK widget, with no Toga interface manifestation.
"""

def __init__(self):
super().__init__()
self._content = None
self.min_width = 100
self.min_height = 100

# GDK/GTK always renders at 96dpi. When HiDPI mode is enabled, it is
# managed at the compositor level. See
# https://wiki.archlinux.org/index.php/HiDPI#GDK_3_(GTK_3) for details
self.dpi = 96
self.baseline_dpi = self.dpi

# The dirty widgets are the set of widgets that are known to need
# re-hinting before any redraw occurs.
self._dirty_widgets = set()

# A flag that can be used to explicitly flag that a redraw is required.
self._needs_redraw = True

@property
def needs_redraw(self):
"""Does the container need a redraw?"""
return self._needs_redraw or bool(self._dirty_widgets)

def make_dirty(self, widget=None):
"""Mark the container (or a specific widget in the container) as dirty.

:param widget: Optional; if provided, the widget that is now dirty. If
not provided, the entire container is considered dirty.
"""
if widget is None:
self._needs_redraw = True
self.queue_resize()
else:
self._dirty_widgets.add(widget)
widget.native.queue_resize()

@property
def width(self):
"""The display width of the container.

If the container doesn't have any content yet, the width is 0.
"""
if self._content is None:
return 0
return self.get_allocated_width()

@property
def height(self):
"""The display height of the container.

If the container doesn't have any content yet, the height is 0.
"""
if self._content is None:
return 0
return self.get_allocated_height()

@property
def content(self):
"""The Toga implementation widget that is the root content of this
container.

All children of the root content will also be added to the container as
a result of assigning content.

If the container already has content, the old content will be replaced.
The old root content and all it's children will be removed from the
container.
"""
return self._content

@content.setter
def content(self, widget):
if self._content:
self._content.container = None

self._content = widget
if widget:
widget.container = self

def recompute(self):
"""Rehint and re-layout the container's content, if necessary.

Any widgets known to be dirty will be rehinted. The minimum
possible layout size for the container will also be recomputed.
"""
if self._content and self.needs_redraw:
# If any of the widgets have been marked as dirty,
# recompute their bounds, and re-evaluate the minimum
# allowed size fo the layout.
while self._dirty_widgets:
widget = self._dirty_widgets.pop()
widget.gtk_rehint()

# Compute the layout using a 0-size container
self._content.interface.style.layout(
self._content.interface, TogaContainer()
)

# print(" computed min layout", self._content.interface.layout)
self.min_width = self._content.interface.layout.width
self.min_height = self._content.interface.layout.height

def do_get_preferred_width(self):
"""Return (recomputing if necessary) the preferred width for the
container.

The preferred size of the container is it's minimum size. This
preference will be overridden with the layout size when the layout is
applied.

If the container does not yet have content, the minimum width is set to
0.
"""
# print("GET PREFERRED WIDTH", self._content)
if self._content is None:
return 0, 0

# Ensure we have an accurate min layout size
self.recompute()

# The container will conform to the size of the allocation it is given,
# so the min and preferred size are the same.
return self.min_width, self.min_width

def do_get_preferred_height(self):
"""Return (recomputing if necessary) the preferred height for the
container.

The preferred size of the container is it's minimum size. This
preference will be overridden with the layout size when the
layout is applied.

If the container does not yet have content, the minimum height
is set to 0.
"""
# print("GET PREFERRED HEIGHT", self._content)
if self._content is None:
return 0, 0

# Ensure we have an accurate min layout size
self.recompute()

# The container will conform to the size of the allocation it is given,
# so the min and preferred size are the same.
return self.min_height, self.min_height

def do_size_allocate(self, allocation):
"""Perform the actual layout for the widget, and all it's children.

The container will assume whatever size it has been given by GTK -
usually the full space of the window that holds the container.
The layout will then be re-computed based on this new available size,
and that new geometry will be applied to all child widgets of the
container.
"""
# print(self._content, f"Container layout {allocation.width}x{allocation.height} @ {allocation.x}x{allocation.y}")

# The container will occupy the full space it has been allocated.
self.set_allocation(allocation)

if self._content:
# Re-evaluate the layout using the allocation size as the basis for geometry
# print("REFRESH LAYOUT", allocation.width, allocation.height)
self._content.interface.refresh()

# WARNING! This is the list of children of the *container*, not
# the Toga widget. Toga maintains a tree of children; all nodes
# in that tree are direct children of the container.
for widget in self.get_children():
if not widget.get_visible():
# print(" not visible {widget.interface}")
pass
else:
# Set the size of the child widget to the computed layout size.
# print(f" allocate child {widget.interface}: {widget.interface.layout}")
widget_allocation = Gdk.Rectangle()
widget_allocation.x = (
widget.interface.layout.absolute_content_left + allocation.x
)
widget_allocation.y = (
widget.interface.layout.absolute_content_top + allocation.y
)
widget_allocation.width = widget.interface.layout.content_width
widget_allocation.height = widget.interface.layout.content_height

widget.size_allocate(widget_allocation)

# The layout has been redrawn
self._needs_redraw = False
30 changes: 19 additions & 11 deletions gtk/src/toga_gtk/widgets/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,15 @@ def __init__(self, interface):
self.interface = interface
self.interface._impl = self
self._container = None
self.viewport = None
self.native = None
self.create()
self.interface.style.reapply()

@property
def viewport(self):
# TODO: Remove the use of viewport
return self._container

def create(self):
pass

Expand All @@ -38,12 +42,12 @@ def container(self, container):
# so removing a widget from its container can cause that widget to be
# destroyed. If you want to use widget again, you should add a
# reference to it.
self._container.native.remove(self.native)
self._container.remove(self.native)
self._container = None
elif container:
# setting container, adding self to container.native
self._container = container
self._container.native.add(self.native)
self._container.add(self.native)
self.native.show_all()

for child in self.interface.children:
Expand All @@ -68,9 +72,9 @@ def set_tab_index(self, tab_index):
######################################################################

def set_bounds(self, x, y, width, height):
# No implementation required here; the new sizing will be picked up
# by the box's allocation handler.
pass
# If the bounds have changed, we need to queue a resize on the container
if self.container:
self.container.make_dirty()
mhsmith marked this conversation as resolved.
Show resolved Hide resolved

def set_alignment(self, alignment):
# By default, alignment can't be changed
Expand Down Expand Up @@ -102,11 +106,7 @@ def set_font(self, font):
######################################################################

def add_child(self, child):
if self.viewport:
# we are the the top level container
child.container = self
else:
child.container = self.container
child.container = self.container

def insert_child(self, index, child):
self.add_child(child)
Expand All @@ -115,6 +115,14 @@ def remove_child(self, child):
child.container = None

def rehint(self):
# GTK doesn't/can't immediately evaluate the hinted size of the widget.
# Instead, put the widget onto a dirty list to be rehinted before the
# next layout.
if self.container:
self.container.make_dirty(self)

def gtk_rehint(self):
# Perform the actual GTK rehint.
# print("REHINT", self, self.native.get_preferred_width(), self.native.get_preferred_height())
width = self.native.get_preferred_width()
height = self.native.get_preferred_height()
Expand Down
Loading