Added on_resize handler on toga.Window
#2364
Conversation
|
Quick question, there are actually 2 events that can be interpreted as
So, which one should we implement for |
|
I think the event would only be useful if we deliver it after the new size is available for rendering the window content. Whether this happens continuously during the resize, or only at the end, may vary between platforms. |
|
I am not sure, if I understood what you meant. There are 2 ways to resize a window:
So, in all cases the new size will be valid, as the new window size is obtained from the platform itself. So, what should we be validating here? As per the current implementation, the behavior is that the But I am not sure if this is the behavior that the user expects. Do you think the |
The mechanism for changing the window size isn't the issue. Depending on the platform, it's possible for the window system to generate window sizes that are invalid - consider the case of resizing a window to be smaller than the size that would be allowed by the content layout rules. Toga then internally rejects or adjusts those window sizes as necessary. In this case, you may get a system notification of a window size that isn't legal, because it's smaller than Toga will allow for content. This would be the window size that a "pre-resize" event (on_resizing, by your naming scheme) would generate. However, I'd argue there's no practical purpose to such an event in Toga. It would only be useful if there was a mechanism to reject the proposed size, and that won't be the case here. On that basis, I'd argue the "on_resized" interpretation is the only one that makes sense for Toga's public API purposes. This event may be generated multiple times as a window is dragged; however, that's a platform implementation detail. When the user-space event is triggered, it should reflect the fact that a new window size (however transient it may be) is available and in effect; and the last on_resize event that is received should be sent when the final window size has been confirmed (i.e., evaluating |
|
@danyeaw Apologies for the direct ping! I wanted to get your insights on detecting window resize events on GTK4. On GTK3, diff --git a/gtk/src/toga_gtk/window.py b/gtk/src/toga_gtk/window.py
index 7c1a7ea21..eac30d32a 100644
--- a/gtk/src/toga_gtk/window.py
+++ b/gtk/src/toga_gtk/window.py
@@ -50,6 +50,8 @@ class Window:
self.native.connect("notify::fullscreened", self.gtk_window_state_event)
self.native.connect("notify::maximized", self.gtk_window_state_event)
self.native.connect("notify::minimized", self.gtk_window_state_event)
+ self.native.connect("notify::default-width", self.gtk_configure_event)
+ self.native.connect("notify::default-height", self.gtk_configure_event)
self._window_state_flags = None
self._in_presentation = FalseBut the events do not get triggered when the window is resized. Following @freakboy3742 's advice, I also tried to use change notification signal on the diff --git a/gtk/src/toga_gtk/window.py b/gtk/src/toga_gtk/window.py
index 59d15f87e..9e20f201e 100644
--- a/gtk/src/toga_gtk/window.py
+++ b/gtk/src/toga_gtk/window.py
@@ -79,6 +80,8 @@ class Window:
self.layout.pack_end(self.container, expand=True, fill=True, padding=0)
self.native.add(self.layout)
else: # pragma: no-cover-if-gtk3
+ self.container.connect("notify::default-width", self.gtk_configure_event)
+ self.container.connect("notify::default-height", self.gtk_configure_event)
self.container.set_valign(Gtk.Align.FILL)
self.container.set_vexpand(True)
self.layout.append(self.container)Am I overlooking anything, or does GTK4 not support detecting window resize events? |
|
Hi @proneon267, I like to come up with small examples to test ideas in GTK4 directly, and then try to get them working with Toga. Here is an example app that gets Window resize notifications:
|
|
Thank you for helping @danyeaw, it seems that the I tried to solve this by listening for For example: Example Source:import gi
gi.require_version("Gtk", "4.0")
from gi.repository import Gtk
class MainWindow(Gtk.Window):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
# Set initial window properties
self.set_title("Window Size Notification Demo")
self.set_default_size(400, 300)
# Create a box to hold our content
self.box = Gtk.Box(orientation=Gtk.Orientation.VERTICAL, spacing=10)
self.box.set_margin_start(10)
self.box.set_margin_end(10)
self.box.set_margin_top(10)
self.box.set_margin_bottom(10)
# Add a label to display the window size
self.size_label = Gtk.Label()
self.update_size_label()
maximize_button = Gtk.Button(label="Maximize")
maximize_button.connect("clicked", self.on_button_maximize)
unmaximize_button = Gtk.Button(label="Un-Maximize")
unmaximize_button.connect("clicked", self.on_button_unmaximize)
fullscreen_button = Gtk.Button(label="Fullscreen")
fullscreen_button.connect("clicked", self.on_button_fullscreen)
unfullscreen_button = Gtk.Button(label="Un-Fullscreen")
unfullscreen_button.connect("clicked", self.on_button_unfullscreen)
self.box.append(self.size_label)
self.box.append(maximize_button)
self.box.append(unmaximize_button)
self.box.append(fullscreen_button)
self.box.append(unfullscreen_button)
# Set the box as the main content
self.set_child(self.box)
# Connect to property notifications
self.connect("notify::default-width", self.on_size_change)
self.connect("notify::default-height", self.on_size_change)
self.connect("notify::maximized", self.on_size_change)
self.connect("notify::fullscreened", self.on_size_change)
def update_size_label(self):
width = self.get_width()
height = self.get_height()
self.size_label.set_text(f"Window size: {width}x{height}")
def on_button_maximize(self, widget):
self.maximize()
def on_button_unmaximize(self, widget):
self.unmaximize()
def on_button_fullscreen(self, widget):
self.fullscreen()
def on_button_unfullscreen(self, widget):
self.unfullscreen()
def on_size_change(self, widget, param):
self.update_size_label()
# We can also get the specific property that changed
property_name = param.get_name()
print(f"Property changed: {property_name}")
class SizeNotifyApp(Gtk.Application):
def __init__(self):
super().__init__(application_id="com.example.sizenotify")
def do_activate(self):
win = MainWindow(application=self)
win.present()
def main():
app = SizeNotifyApp()
return app.run(None)
if __name__ == "__main__":
main()
As we can see in the example above, the window dimensions that the callback event receives when it queries after the window is maximized are outdated (i.e., the returned values are from previous state of the window, not from the current state). I am testing on:
|
|
Hi @proneon267, we are sort of working against how GTK4 wants to be used. libadwaita handles all the dynamic / reactive layout changes and the window manager is in charge of resizing. You are seeing that window state changes are handled asynchronously by the window manager and may not happen immediately (or at all). However, something like this seems to work if we schedule an update after the event occurs: import gi
gi.require_version('Gtk', '4.0')
from gi.repository import Gtk, GLib
class MainWindow(Gtk.ApplicationWindow):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.set_title("Window State Monitor")
self.set_default_size(400, 300)
# Create main box
self.box = Gtk.Box(orientation=Gtk.Orientation.VERTICAL, spacing=10)
self.box.set_margin_start(10)
self.box.set_margin_end(10)
self.box.set_margin_top(10)
self.box.set_margin_bottom(10)
# Add labels for window states
self.size_label = Gtk.Label()
self.state_label = Gtk.Label()
self.box.append(self.size_label)
self.box.append(self.state_label)
# Add control buttons
button_box = Gtk.Box(orientation=Gtk.Orientation.HORIZONTAL, spacing=5)
self.fullscreen_button = Gtk.Button(label="Toggle Fullscreen")
self.fullscreen_button.connect('clicked', self.on_fullscreen_toggle)
button_box.append(self.fullscreen_button)
self.maximize_button = Gtk.Button(label="Toggle Maximize")
self.maximize_button.connect('clicked', self.on_maximize_toggle)
button_box.append(self.maximize_button)
self.box.append(button_box)
# Set the box as the main content
self.set_child(self.box)
# Connect to property notifications
self.connect('notify::default-width', self.on_state_change)
self.connect('notify::default-height', self.on_state_change)
self.connect('notify::fullscreen', self.on_state_change)
self.connect('notify::maximized', self.on_state_change)
# Store timeout ID
self.update_timeout_id = None
# Initial update
self.update_labels()
def update_labels(self):
# Update size information
width = self.get_width()
height = self.get_height()
self.size_label.set_text(f"Window size: {width}x{height}")
# Update state information
states = []
if self.is_fullscreen():
states.append("Fullscreen")
if self.is_maximized():
states.append("Maximized")
if not states:
states.append("Normal")
self.state_label.set_text("Window state: " + ", ".join(states))
# Return False to stop the timeout if this was called from a timeout
return False
def schedule_update(self):
# Cancel any pending update
if self.update_timeout_id is not None:
GLib.source_remove(self.update_timeout_id)
# Schedule a new update
self.update_timeout_id = GLib.timeout_add(150, self.update_labels)
def on_state_change(self, widget, param):
property_name = param.get_name()
print(f"Property changed: {property_name}")
print(f"- Fullscreen: {self.is_fullscreen()}")
print(f"- Maximized: {self.is_maximized()}")
print(f"- Size: {self.get_width()}x{self.get_height()}")
# Schedule an update to get the final size
self.schedule_update()
def on_fullscreen_toggle(self, button):
if self.is_fullscreen():
self.unfullscreen()
print("Requesting unfullscreen...")
else:
self.fullscreen()
print("Requesting fullscreen...")
# Schedule update for size changes
self.schedule_update()
def on_maximize_toggle(self, button):
if self.is_maximized():
self.unmaximize()
print("Requesting unmaximize...")
else:
self.maximize()
print("Requesting maximize...")
# Schedule update for size changes
self.schedule_update()
class SizeNotifyApp(Gtk.Application):
def __init__(self):
super().__init__(application_id='com.example.sizenotify')
def do_activate(self):
win = MainWindow(application=self)
win.present()
def main():
app = SizeNotifyApp()
return app.run(None)
if __name__ == '__main__':
main() |
|
@danyeaw Ah! I see, this is the same trick that I had used while implementing window states on wayland: toga/gtk/src/toga_gtk/window.py Lines 155 to 169 in 7ea9b9f While working on #2473, I realized that window state change operations are asynchronous. However, according to the GTK docs:
This suggested that property change notifications like Does this mean GTK doesn't provide any reliable indicators for when a specific async operation is completed? Does libadwaita uses its own internal undocumented APIs to detect when an async operation is completed? Also for setting the GLib Timeout, is there any specific rule to calculate the ideal schedule time, or does it need to be done on a trial and error basis? For determining the ideal timeout for #2473, I had tested a range of durations, which were then repeatedly validated on CI and local machines. The final duration was chosen to be as small as possible while still ensuring the async operation completes reliably. |
|
Hey @proneon267! I've been digging into this and found some interesting stuff in libadwaita's ToolbarView code that explains what's going on. You know how GTK says those notify signals should fire after operations are done? Well, turns out it's a bit trickier than that - particularly when you want to know the actual window size. As you pointed out, the signals fire alright, but Looking at libadwaita's code, they handle window sizing in three key parts:
static void
adw_toolbar_view_measure (GtkWidget *widget,
GtkOrientation orientation,
int for_size,
int *minimum,
int *natural,
int *minimum_baseline,
int *natural_baseline)
{
AdwToolbarView *self = ADW_TOOLBAR_VIEW (widget);
int top_min, bottom_min, content_min = 0;
int top_nat, bottom_nat, content_nat = 0;
gtk_widget_measure (self->top_bar, orientation, -1,
&top_min, &top_nat, NULL, NULL);
// ... more measuring code ...
}
static void
adw_toolbar_view_size_allocate (GtkWidget *widget,
int width,
int height,
int baseline)
{
AdwToolbarView *self = ADW_TOOLBAR_VIEW (widget);
int top_min, top_nat, bottom_min, bottom_nat, content_min = 0;
int top_height, bottom_height;
int content_height, content_offset;
gtk_widget_measure (self->top_bar, GTK_ORIENTATION_VERTICAL, width,
&top_min, &top_nat, NULL, NULL);
// ... allocation code ...
}
if (self->top_bar_height != top_height) {
self->top_bar_height = top_height;
g_object_notify_by_pspec (G_OBJECT (self), props[PROP_TOP_BAR_HEIGHT]);
}Based on this, I put together a simpler Python version that follows the same pattern: def do_size_allocate(self, width, height, baseline):
# Let GTK do its thing first
Gtk.ApplicationWindow.do_size_allocate(self, width, height, baseline)
# Now we can safely check if size actually changed
if width != self.prev_width or height != self.prev_height:
self.prev_width = width
self.prev_height = height
self.update_labels()This approach is pretty sweet because:
So about your questions:
Here is a full example: import gi
gi.require_version('Gtk', '4.0')
from gi.repository import Gtk
class MainWindow(Gtk.ApplicationWindow):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.set_title("Window State Monitor")
self.set_default_size(400, 300)
# Create main box
self.box = Gtk.Box(orientation=Gtk.Orientation.VERTICAL, spacing=10)
self.box.set_margin_start(10)
self.box.set_margin_end(10)
self.box.set_margin_top(10)
self.box.set_margin_bottom(10)
# Add labels for window states
self.size_label = Gtk.Label()
self.state_label = Gtk.Label()
self.box.append(self.size_label)
self.box.append(self.state_label)
# Add control buttons
button_box = Gtk.Box(orientation=Gtk.Orientation.HORIZONTAL, spacing=5)
self.fullscreen_button = Gtk.Button(label="Toggle Fullscreen")
self.fullscreen_button.connect('clicked', self.on_fullscreen_toggle)
button_box.append(self.fullscreen_button)
self.maximize_button = Gtk.Button(label="Toggle Maximize")
self.maximize_button.connect('clicked', self.on_maximize_toggle)
button_box.append(self.maximize_button)
self.box.append(button_box)
# Set the box as the main content
self.set_child(self.box)
# Store previous dimensions to detect changes
self.prev_width = 0
self.prev_height = 0
# Initial update
self.update_labels()
def do_size_allocate(self, width, height, baseline):
# Call parent's size_allocate first
Gtk.ApplicationWindow.do_size_allocate(self, width, height, baseline)
# Now update our labels if the size has changed
if width != self.prev_width or height != self.prev_height:
self.prev_width = width
self.prev_height = height
self.update_labels()
def update_labels(self):
# Update size information
width = self.get_width()
height = self.get_height()
self.size_label.set_text(f"Window size: {width}x{height}")
# Update state information
states = []
if self.is_fullscreen():
states.append("Fullscreen")
if self.is_maximized():
states.append("Maximized")
if not states:
states.append("Normal")
self.state_label.set_text("Window state: " + ", ".join(states))
def on_fullscreen_toggle(self, button):
if self.is_fullscreen():
self.unfullscreen()
print("Requesting unfullscreen...")
else:
self.fullscreen()
print("Requesting fullscreen...")
def on_maximize_toggle(self, button):
if self.is_maximized():
self.unmaximize()
print("Requesting unmaximize...")
else:
self.maximize()
print("Requesting maximize...")
class SizeNotifyApp(Gtk.Application):
def __init__(self):
super().__init__(application_id='com.example.sizenotify')
def do_activate(self):
win = MainWindow(application=self)
win.present()
def main():
app = SizeNotifyApp()
return app.run(None)
if __name__ == '__main__':
main() |
|
@danyeaw Thank you so much for researching this. When I had read that the size allocate was removed from GTK4, I had thought that the As you have noted and from my manual testing, this works with every window operation. Although we need to track the window size ourselves, but by using this we would be avoiding adding timeouts, which as @freakboy3742 has noted earlier, are fragile in nature. Thank you again for finding this. I will complete this PR with the GTK4 implementation, and re-implement the existing features for GTK4 in separate PRs. |
gtk/src/toga_gtk/window.py
Outdated
| class TogaWindow(Gtk.Window): # pragma: no-cover-if-gtk3 | ||
| def __init__(self, interface, impl): | ||
| self.interface = interface | ||
| self.impl = impl | ||
| super().__init__() | ||
|
|
||
| def do_size_allocate(self, width, height, baseline): | ||
| # Call parent's size_allocate first | ||
| Gtk.Window.do_size_allocate(self, width, height, baseline) | ||
|
|
||
| if self.impl._window_size != (width, height): | ||
| self.impl._window_size = Size(width, height) | ||
| self.interface.on_resize() | ||
|
|
||
|
|
||
| class TogaMainWindow(Gtk.ApplicationWindow): # pragma: no-cover-if-gtk3 | ||
| def __init__(self, interface, impl): | ||
| self.interface = interface | ||
| self.impl = impl | ||
| super().__init__() | ||
|
|
||
| def do_size_allocate(self, width, height, baseline): | ||
| # Call parent's size_allocate first | ||
| Gtk.ApplicationWindow.do_size_allocate(self, width, height, baseline) | ||
|
|
||
| if self.impl._window_size != (width, height): | ||
| self.impl._window_size = Size(width, height) | ||
| self.interface.on_resize() | ||
|
|
||
|
|
There was a problem hiding this comment.
These 2 classes are almost identical, except for the use of Gtk.Window and Gtk.ApplicationWindow. To reduce code duplication, I had tried to use a base class: Attempt 1 and Attempt 2
This didn't work as the do_size_allocate() vfunc on Gtk wasn't getting overridden properly by the do_size_allocate() method in BaseTogaWindow Class.
Next, I tried to implement it as: Attempt 3, using the 3 argument form of type(): https://docs.python.org/3/library/functions.html#type
This worked correctly by properly overriding the Gtk's do_size_allocate() vfunc, and gave correct results. However, the window cleanup didn't work properly, and the secondary_window_cleanup test was failing on GTK4: Attempt 3 Test failure
Hence, I had to use the current implementation of duplicating majority of the code for Gtk.Window and Gtk.ApplicationWindow. Is there any other way in which we could implement it, so that the duplicated code could be shared among them?
EDIT: I have moved the huge codeblocks into gist, as per the suggestion.
There was a problem hiding this comment.
These 2 classes are almost identical, except for the use of
Gtk.WindowandGtk.ApplicationWindow.
As a stylistic note - your habit of posting large chunks of code (and in this case, a hand-crafted side-by-side code table) that is in the commit history doesn't make your comments easy to read - quite the opposite, in fact... because eventually Github starts to choke on rendering long comment threads.
This most recent comment renders as almost 2 pages long... and it amounts to "I've tried a couple of approaches to subclassing to avoid code duplication (link to attempt 1, link to attempt 2), and none of them work; any ideas?"
Could I ask you to try and be a little more terse in your comments, and make more use of links, rather than inline code blocks?
As for sharing implementations - I'm not deeply familiar with how GTK interacts with multiple inheritance, but it doesn't surprise me that there's some complication. Some possible approaches:
- Changing the inheritance order. There's a difference between
class X(A,B)andclass X(B, A)in terms of how methods are resolved; you may find reversing the order helps. - You've used
super()to calldo_size_allocate()in your "common base class" version... but your "duplicated" version uses explicit invocation (e.g.,Gtk.Window.do_size_allocate(self, ...)). You might want to investigate ifsuper()is actually returning the object you want/need; and if it isn't, whether maybe storing a handle togtk_class = Gtk.Windowand then invokinggtk_class.do_size_allocate(self, ...)works better. - Contruct a class out of primitives. This is the least appealing options, but if you define
do_size_allocate()as a standalone function, and then assignTogaApplicationWindow.do_size_allocate = do_size_allocate, you will get a class level function that can have a shared implementation. It's a messy hand-rolled implementation of subclassing... but as a last resort, it might work.
There was a problem hiding this comment.
As a stylistic note - your habit of posting large chunks of code (and in this case, a hand-crafted side-by-side code table) that is in the commit history doesn't make your comments easy to read - quite the opposite, in fact... because eventually Github starts to choke on rendering long comment threads.
This most recent comment renders as almost 2 pages long... and it amounts to "I've tried a couple of approaches to subclassing to avoid code duplication (link to attempt 1, link to attempt 2), and none of them work; any ideas?"
Could I ask you to try and be a little more terse in your comments, and make more use of links, rather than inline code blocks?
I agree with you, I’ll keep my comments more concise moving forward. Apologies for the lengthy post!
Changing the inheritance order. There's a difference between class X(A,B) and class X(B, A) in terms of how methods are resolved; you may find reversing the order helps.
I tried both the orders in Attempt 1 and Attempt 2. However, the overridden do_size_allocate() vfunc wasn't being called by GTK.
You've used super() to call do_size_allocate() in your "common base class" version... but your "duplicated" version uses explicit invocation (e.g., Gtk.Window.do_size_allocate(self, ...)). You might want to investigate if super() is actually returning the object you want/need; and if it isn't, whether maybe storing a handle to gtk_class = Gtk.Window and then invoking gtk_class.do_size_allocate(self, ...) works better.
I also tried to use the class name instead of super() in Attempt 3, but still GTK never triggered the overridden do_size_allocate() vfunc.
Contruct a class out of primitives. This is the least appealing options, but if you define do_size_allocate() as a standalone function, and then assign TogaApplicationWindow.do_size_allocate = do_size_allocate, you will get a class level function that can have a shared implementation. It's a messy hand-rolled implementation of subclassing... but as a last resort, it might work.
I tried to manually modify it with TogaApplicationWindow.do_size_allocate = do_size_allocate in Attempt 4. But it still didn't work.
However, at last I have implemented Attempt 5, which works correctly without any errors or test failures. Will this be an acceptable solution?
There was a problem hiding this comment.
However, at last I have implemented Attempt 5, which works correctly without any errors or test failures. Will this be an acceptable solution?
I don't love it.
The thing I can't escape: Attempt 5 is 34 lines of code. The first attempt - with full "code duplication" - is 28 lines. And that includes at least 2 comments that are fairly redundant.
The code that is being duplicated is either boilerplate, or the signal handling. This version (based on the first attempt) is only 26 lines:
def handle_resize(widget, width, height):
if widget.impl._window_size != (width, height):
widget.impl._window_size = Size(width, height)
widget.interface.on_resize()
class TogaWindow(Gtk.Window): # pragma: no-cover-if-gtk3
def __init__(self, interface, impl):
super().__init__()
self.interface = interface
self.impl = impl
def do_size_allocate(self, width, height, baseline):
Gtk.Window.do_size_allocate(self, width, height, baseline)
handle_resize(self, width, height)
class TogaMainWindow(Gtk.ApplicationWindow): # pragma: no-cover-if-gtk3
def __init__(self, interface, impl):
super().__init__()
self.interface = interface
self.impl = impl
def do_size_allocate(self, width, height, baseline):
Gtk.ApplicationWindow.do_size_allocate(self, width, height, baseline)
handle_resize(self, width, height)
and while there is "duplication", it's all duplicated boilerplate, not business logic.
Sometimes it helps to not overthink things :-)
Lastly - you've got these two classes defined with #pragma definitions. If the classes will only exist for GTK4, then their definitions should be inside an if GTK_VERSION >= (4, 0, 0) block, with that block marked #pragma.
(Also - can the Gtk.Window.do_size_allocate() calls be super() calls? I haven't tried, but it's worth checking).
There was a problem hiding this comment.
At last, I did it :D
I'm now able to hook up any custom function to any vfunc. This approach reduces code duplication and fixes a potential bug that would have arisen from the duplicated code approach, used in my first attempt (details in explanation).
@freakboy3742 Thank you for suggesting to use a cleaner approach, I wouldn't have tried it otherwise.
Also, a partial review of the GTK4 changes would be great.
Long Form Explanation:
NOTE: Although this uses the internal function hook_up_vfunc_implementation, note that it is used directly by PyGObject and has been stable for over 14 years.
from gi._gi import hook_up_vfunc_implementation
def create_toga_native(native_gtk_class):
toga_native_class = type(
native_gtk_class.__gtype__.name,
(native_gtk_class,),
{"base_class": native_gtk_class}, # Store the base class type
)
return toga_native_class
class Window:
def __init__(self, interface, title, position, size):
...
self.create()
hook_up_vfunc_implementation(
self.native.do_size_allocate, self.native.__gtype__, self.do_size_allocate
)
...
def do_size_allocate(self, native, width, height, baseline):
# Call parent's size_allocate first
native.base_class.do_size_allocate(native, width, height, baseline)
if self._window_size != (width, height):
self._window_size = Size(width, height)
self.interface.on_resize()
def create(self):
self.native = create_toga_native(Gtk.Window)()
class MainWindow:
def create(self):
self.native = create_toga_native(Gtk.ApplicationWindow)()do_size_allocate() is a virtual function. Unlike normal methods on a class, PyGObject connects virtual functions using hook_up_vfunc_implementation during class initialization (via a metaclass helper, as shown here). This means that default virtual methods can only be overridden on a class that directly inherits from a Gtk class (e.g. a class inheriting from Gtk.ApplicationWindow), before the class is instantiated. Once instantiated, virtual methods cannot be overridden because PyGObject only connects and overrides them during class initialization.
Moreover, the method resolution order (MRO) for classes inheriting from multiple objects is modified by PyGObject. Overriding any virtual function on a class affects all instances of that class (and even its parent/super classes), because PyGObject connects virtual functions based on the class’s __gtype__. All instances share the same __gtype__, and therefore the same virtual function implementation.
For example, if we have:
self.native1 = Gtk.ApplicationWindow()and then override do_size_allocate using hook_up_vfunc_implementation, and later create:
self.native2 = Gtk.ApplicationWindow()
self.native3 = Gtk.ApplicationWindow()all of native1, native2, and native3 will have the same overridden do_size_allocate virtual function. If the overridden do_size_allocate() does something specific for native1, it will affect native2 and native3 as well.
In short, all instances of a class share the same __gtype__ and virtual functions. This can lead to unexpected bugs if we override a virtual function expecting instance-specific behavior.
To solve this, every object whose virtual functions are to be overridden must have a unique __gtype__. Since there isn’t a direct way to create a new __gtype__, we must ensure that for classes whose virtual functions need overriding, only one instance is created—or, more practically, use the three-argument form of type() to create a new class (and therefore a unique __gtype__) for each instance.
For example, if we instantiate Gtk.ApplicationWindow directly:
self.native1 = Gtk.ApplicationWindow()
self.native2 = Gtk.ApplicationWindow()
self.native3 = Gtk.ApplicationWindow()then:
self.native1.__gtype__ == self.native2.__gtype__ == self.native3.__gtype__ # TruePrinting the gtype:
print(self.native1.__gtype__)
print(self.native2.__gtype__)
print(self.native3.__gtype__)produces:
<GType GtkApplicationWindow (101039147306688)>
<GType GtkApplicationWindow (101039147306688)>
<GType GtkApplicationWindow (101039147306688)>
However, if we do:
def create_toga_native(native_gtk_class):
toga_native_class = type(
native_gtk_class.__gtype__.name,
(native_gtk_class,),
{"base_class": native_gtk_class},
)
return toga_native_class
class Window:
def create():
self.native1 = create_toga_native(Gtk.Window)()
self.native2 = create_toga_native(Gtk.Window)()
self.native3 = create_toga_native(Gtk.Window)()
print(self.native1.__gtype__)
print(self.native2.__gtype__)
print(self.native3.__gtype__)we get:
<GType toga_gtk+window+GtkWindow (108742433761824)>
<GType toga_gtk+window+GtkWindow-v2 (108742433730080)>
<GType toga_gtk+window+GtkWindow-v3 (108742432575200)>
Now each instance has a unique __gtype__, so overriding a virtual function on one does not affect the others.
There was a problem hiding this comment.
The overall comment from my last review still stands:
The only technical detail that stood out is the exact ordering of the event - should it occur before or after the call to refresh layout? I imagine one of the major use cases for this will be "The window has resized; I want to set the width of widget X to 1/2 the window size". If we trigger the refresh before, then changing the widget size will immediately trigger another layout pass.
There's an inconsistency between Cocoa and Winforms; it isn't clear what the interpretation on GTK, Android and iOS will be because the signal and the resize aren't obviously tied together.
I also suggested there was a need for a test to validate behaviour on mobile, and to validate that when inside the on_resize handler, window.size returns the "new" size.
gtk/src/toga_gtk/window.py
Outdated
|
|
||
| ###################################################################### | ||
| # Native event handlers | ||
| ###################################################################### | ||
| def gtk_do_size_allocate( |
There was a problem hiding this comment.
When we're adding code that has different code for GTK3/GTK4, we're putting it into if GTK_VERSION ... blocks. This means in future it's clear what code is GTK3 specific; so when the eventual "remove GTK3 support" PR happens, we know exactly what code we can delete.
There was a problem hiding this comment.
Thanks for clarifying it. I have moved GTK4 code into if blocks.
gtk/src/toga_gtk/window.py
Outdated
| if GTK_VERSION < (4, 0, 0): # pragma: no-cover-if-gtk4 | ||
| self.native = Gtk.ApplicationWindow() | ||
| else: # pragma: no-cover-if-gtk3 | ||
| self.native = create_toga_native(Gtk.ApplicationWindow)() | ||
| if GTK_VERSION < (4, 0, 0): # pragma: no-cover-if-gtk4 |
There was a problem hiding this comment.
You've got 2 GTK_VERSION blocks in rapid succession here - we don't need both.
There was a problem hiding this comment.
Thanks, I have merged them.
gtk/src/toga_gtk/libs/wrapper.py
Outdated
There was a problem hiding this comment.
Why does this need another module? Isn't it just another utility? It's in wrapper for winforms because there is not "utils" class.
There was a problem hiding this comment.
Thanks, I have moved the wrapper class to utils.
gtk/src/toga_gtk/libs/utils.py
Outdated
| @@ -11,3 +11,14 @@ def gtk_text_align(alignment): | |||
| CENTER: (0.5, Gtk.Justification.CENTER), | |||
| JUSTIFY: (0.0, Gtk.Justification.FILL), | |||
| }[alignment] | |||
|
|
|||
|
|
|||
| def create_toga_native(native_gtk_class): # pragma: no-cover-if-gtk3 | |||
There was a problem hiding this comment.
As noted elsewhere - if this is code that isn't needed on GTK4, it should be in an if GTK_VERSION... block.
There was a problem hiding this comment.
Thanks, I have moved it into an if block.
I have changed it so that
I have added a new test on mobile to check if |
| if GTK_VERSION >= (4, 0, 0): # pragma: no-cover-if-gtk3 | ||
| from gi._gi import hook_up_vfunc_implementation # noqa: E402, F401 | ||
|
|
||
| if GTK_VERSION < (4, 0, 0): # pragma: no-cover-if-gtk4 |
There was a problem hiding this comment.
Again - this is the same comparison, in directly seqeuential blocks. You can combine them.
|
|
||
| if self._window_size != (width, height): | ||
| self._window_size = Size(width, height) | ||
| self.interface.on_resize() |
There was a problem hiding this comment.
Are you sure this is correct? It's sending the on_resize() after applying the size allocation - which is the opposite order of other platforms.
|
|
||
| app_probe.rotate() | ||
| await app_probe.redraw("Device has been rotated") | ||
| main_window_on_resize_handler.assert_called_with(main_window) |
There was a problem hiding this comment.
This asserts the handler is invoked, but doesn't verify that the size of the window is correct at the time it is invoked.
There was a problem hiding this comment.
The app_probe.rotate() method isn't actually changing the orientation, and is just invoking the native event notification to simulate changing orientation. So, the window size isn't actually changing.
I took a look at the native APIs required for actually implementing orientation change on mobile platforms, and it seems real simple to implement. So, I'll implement it and report back.
There was a problem hiding this comment.
Definitely interested to know if you find something - but I'm not sure if you'll be able to expose this in a test, as it requires the simulator to undergo a rotation, and I don't think that's controllable programmatically (at least, not through the interface that we have).
I guess we might be able to fake it by forcing the device to render as landscape when the phone is physically in portrait... but that's really a different set of features.
So - any test of this is going to be a little weak, as the device won't actually be rotating... but we should verify that we're getting the right size for whatever is actually happening on the phone.
There was a problem hiding this comment.
Although we cannot change the device orientation(i.e., global device rotation), but we can change the app's orientation. The effect would be the same. Here is a quick prototype that you can try on Android: https://gist.github.com/proneon267/0cb257651edfde5b8bd412459ecf23ff
We can do the same on iOS using https://developer.apple.com/documentation/uikit/uideviceorientation?language=objc. Currently, I don't have a prototype for iOS, but I believe it could be also done for iOS. Let me know if the android prototype would be good enough for the test probe.
There was a problem hiding this comment.
To be clear - are you proposing this as a public API, or just a probe? I can see how this could be interesting as a public API for mobile platforms, but I'm wary of scope creep - I'd be OK with this being "just the probe".
The one detail that needs to be confirmed is that "width" and "height" are the expected directions even when the physical direction hasn't moved. I haven't run your example code, but it looks like you're able to confirm that the measurement axes actually switch when you rotate the rendering but not the device.
There was a problem hiding this comment.
To be clear - are you proposing this as a public API, or just a probe?
Only as a probe, as I don't think allowing the app to control orientation would be a good idea.
The one detail that needs to be confirmed is that "width" and "height" are the expected directions even when the physical direction hasn't moved. I haven't run your example code, but it looks like you're able to confirm that the measurement axes actually switch when you rotate the rendering but not the device.
Yes, the measurement axes switch on Android, but I'd need to confirm this on iOS. Also, I will implement this first in a separate PR for easier reviewing.
There was a problem hiding this comment.
To be clear - are you proposing this as a public API, or just a probe?
Only as a probe, as I don't think allowing the app to control orientation would be a good idea.
So - I'm not sure about that. There are some legitimate uses for this - games, for instance will often set and lock orientations to ensure the game fits on the screen. Toga isn't a good fit for games, but there are uses where landscape will make sense.
At the very least, Briefcase likely needs modifications so you can specify the valid orientations at time of app launch; but having in-app control might also be helpful for some applications. There's obviously the "what does this button do on desktop/web/textual" question to resolve, but we already have features that exist on desktop and not on mobile; it makes sense the reverse would also be possible.
The one detail that needs to be confirmed is that "width" and "height" are the expected directions even when the physical direction hasn't moved. I haven't run your example code, but it looks like you're able to confirm that the measurement axes actually switch when you rotate the rendering but not the device.
Yes, the measurement axes switch on Android, but I'd need to confirm this on iOS. Also, I will implement this first in a separate PR for easier reviewing.
👍 As long as this PR checks the width/height in the "non rotating" context, that's fine.
There was a problem hiding this comment.
FYI: Android recently announced that they're phasing out the ability for an app to control its own orientation, unless it's flagged as a game.
| second_window_on_resize_handler.assert_not_called() | ||
| second_window_on_resize_handler.reset_mock() | ||
| else: | ||
| second_window_on_resize_handler.assert_called_with(second_window) |
There was a problem hiding this comment.
As with the mobile test - this verifies that the resize handler has been called, but doesn't verify that it has been called with the correct size.
| # be: FULLSCREEN -> NORMAL -> MAXIMIZED. Therefore, on_resize() | ||
| # would be triggered multiple times. Hence, just assert that the | ||
| # on_resize() event has been called. | ||
| second_window_on_resize_handler.assert_called_with(second_window) |
There was a problem hiding this comment.
Again - this verifies that the resize handler has been called, but doesn't verify that it has been called with the correct size.
| second_window.size = (200, 150) | ||
| await second_window_probe.wait_for_window("Second window has been resized") | ||
| assert second_window.size == (200, 150) | ||
| second_window_on_resize_handler.assert_called_with(second_window) |
There was a problem hiding this comment.
Again - this verifies that the resize handler has been called, but doesn't verify that it has been called with the correct size.
| ) | ||
| ], | ||
| ) | ||
| async def test_window_size_updated_on_resize_event( |
There was a problem hiding this comment.
There's no need for a separate test for this. We're performing multiple tests of on_resize, under all sorts of conditions. On some backends, those conditions trigger entirely different logic paths. We need to verify this property is always true, not just true on the 1 specific test where we're checking it.
Toga Windows now supports calling user functions on resize events using
on_resizehandler.Fixes #2304
Also, gtk doesn't provide a native signal for detecting window resize only. The closest, I have found is the
configure-event, but it triggers when the window is resized and also when the window is moved.I was thinking of using the
configure-event& checking if the window is not moving, and only then triggering theon_resizehandler. If this doesn't work, we would need to use a timer and periodically check the window size.PR Checklist: