-
-
Notifications
You must be signed in to change notification settings - Fork 661
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 states API #2473
base: main
Are you sure you want to change the base?
Add window states API #2473
Conversation
…to window_states
gtk/src/toga_gtk/window.py
Outdated
return WindowState.NORMAL | ||
|
||
def set_window_state(self, state): | ||
if ("WAYLAND_DISPLAY" in os.environ) and ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pardon the interjection...but I was curious if there has been discussions around this methodology to detect which protocol the display is using?
While the environment variable WAYLAND_DISPLAY
will probably be a reliable indicator that wayland is in use, this seems inherently fragile; I mean, it's trivial to set environment variables.
Furthermore, though, I can force an app to use X11 via Xwayland:
toga/examples/tutorial1 on main [!] via 🐍 v3.12.3 (briefcase-3.12) took 38s
❯ GDK_BACKEND=x11 python -m tutorial
Gdk.Display.get_default()=<GdkX11.X11Display object at 0x7f29e6da6500 (GdkX11Display at 0x35d826c0)>
^C
toga/examples/tutorial1 on main [!] via 🐍 v3.12.3 (briefcase-3.12) took 4s
❯ GDK_BACKEND=wayland python -m tutorial
Gdk.Display.get_default()=<__gi__.GdkWaylandDisplay object at 0x7f2fe8cfee40 (GdkWaylandDisplay at 0xde391c0)>
I've spent a little time seeing if PyGObject is exposing any of the useful C macros that can evaluate the nature of the display....not to much avail, though. But that's mostly why I'm back to my original question in case we've already evaluated this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I appreciate it and also wanted to discuss about it. While implementing the screens API, I had found 3 main methods to detect Wayland:
- Check for the presence of Wayland sockets in
/run/user/$UID/wayland-*
- Check if xrandr works i.e.,
xrandr --listmonitors
- Check for environment variables like:
$WAYLAND_DISPLAY
,$XDG_SESSION_TYPE
- Both of them are equally reliable, but$WAYLAND_DISPLAY
is simpler to check.
The 2nd one seemed way more fragile & unreliable.
The first one seemed a bit more compositor-implementation specific thing, as I am not sure if the presence of a Wayland socket at /run/user/$UID/
is defined in the Wayland protocol specification.
Hence, I had opted to use the third option. I know that it seems a bit fragile but I have manually tested the behaviour on 3 different DEs - KDE Plasma, Gnome, Cinnamon and on 5 different distros. The behaviour of $WAYLAND_DISPLAY
was as expected and gave the same results. So, I went ahead with it.
Let me know, what do you think about it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dug in to this a bit more today. Ideally, I think, Toga would be abstracted away from the details of X11 and Wayland altogether; however, obviously, there are high-level consequences when we aren't using X11 and we need a way to detect this situation.
I don't think Toga should be trying to make this determination itself; instead, we should defer to the GNOME stack we're building on since it's intimately aware of these details and already managing all of this. Since it was not at all apparent how to extract this information with PyGObject, I eventually solicited the GNOME team for some insight.
While we remain on Gtk3, we can do something like this:
import gi
gi.require_versions({'Gdk': '3.0', 'Gtk': '3.0', 'GdkX11': '3.0'})
from gi.repository import Gdk, Gtk, GdkX11
IS_WAYLAND = not isinstance(Gdk.Display.get_default(), GdkX11.X11Display)
Once we're able to transition to Gtk4, we can make this a little more straightforward but it wouldn't be required:
import gi
gi.require_versions({'Gdk': '4.0', 'Gtk': '4.0', 'GdkWayland': '4.0'})
from gi.repository import Gdk, Gtk, GdkWayland
IS_WAYLAND = isinstance(Gdk.Display.get_default(), GdkWayland.WaylandDisplay)
On Gtk3, GtkWayland
is not exposed through the introspection....but in Gtk4, it is.
We should be able to tuck this away somewhere and replace the os.environ
refs with a IS_WAYLAND
global.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last note: this code may not be generally portable. However, Toga only uses Gtk in *NIX environments where a Display
must use X11 or Wayland; so, it's portable insofar as Toga is concerned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this seems like a good option. I think we can put this in core/src/platform.py
or any other place you deem appropriate. But, I think this PR is already huge and I will leave it to you or someone else to implement it :)
As a side note, when we transition to Gtk4, I don't think this window state API, or even the screens API would work properly, since gtk4 has made much of the current API non-functional or has downright removed them. Wayland's security features are making this even more difficult. Gtk's docs mention one thing and their APIs do another thing.
Even while implementing this PR, I have found wayland doesn't allow for proper minimization-restoration, full screening, etc. and even the maximization-restoration gtk APIs fails sometimes. I am thinking of disabling window state setting altogether on wayland, as the behavior seems highly fragile and is breaking in new ways with each new release of gnome.
Hence, I think I'll just avoid implementing for Wayland and raise a NotImplementedError
. Let me know what you think.
I have reimplemented most of the PR. The only concern remaining is that the implementation of I had earlier chosen to do this because all of the platforms don't support all the states and we are creating those states, like PRESENTATION mode on Windows. Furthermore, to keep track of these non-natively supported states, we are using private flags like Hence, since both the backend implementation and probe implementation use the same native flags and private flags to check for the current window state, so I had kept their implementations same. I know that we need to check for the ground truth in the probe, but the ground truth can only be detected by using the same combination of native and private flags, both in the backend and probe. I am not sure how should I move forward with this. Let me know what you think :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementations make a lot more sense now; I've flagged a few general trends and potential cleanups, but the core logic is a lot easier to follow in this iteration.
The next step is to get the tests clear - not just getting coverage, but making it clear that we've got coverage.
|
||
def show_actionbar(self, show): | ||
actionbar = self.app.native.getSupportActionBar() | ||
actionbar.show() if show else actionbar.hide() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
show()
and hide()
aren't idempotent methods, so it's risky (or, at least, confusing) to use them in an inline if. This is much better as a traditional if show:
... construct.
pass | ||
|
||
elif state == WindowState.MINIMIZED: | ||
self.interface.factory.not_implemented( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be preferable to leave this as a documented no-op, and revisit it in the future if we want to add support for minimising/backgrounding an app.
# elif current_state == WindowState.PRESENTATION: | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see what you're saying here, but it's potentially confusing. Better to put the current_state == ...
part as a comment on the tail of the else:
clause directly, rather than make it look like there's an extra elif
# elif current_state == WindowState.PRESENTATION: | |
else: | |
else: # current_state == PRESENTATION |
# elif current_state == WindowState.NORMAL: | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above:
# elif current_state == WindowState.NORMAL: | |
else: | |
else: # current_state == NORMAL |
# elif state == WindowState.PRESENTATION: | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
# elif state == WindowState.PRESENTATION: | |
else: | |
else: # state == PRESENTATION |
await main_window_probe.wait_for_window("Full screen is a no-op") | ||
@pytest.mark.skipif( | ||
toga.platform.current_platform == "iOS", reason="Not implemented on iOS" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this an explicit platform skipIf, rather than a probe property?
|
||
# Add delay to ensure windows are visible after animation. | ||
await second_window_probe.wait_for_window( | ||
"Secondary window is visible", full_screen=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why full_screen =True
? The window isn't full screen at this point.
# Set to final state | ||
second_window.state = final_state | ||
# Add delay to ensure windows are visible after animation. | ||
await app_probe.redraw(f"Secondary window is in {final_state}", delay=1.5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sort of delay is what wait_for_window
is for - some platforms can move faster than others.
assert main_window_probe.get_window_state() == WindowState.NORMAL | ||
main_window.state = WindowState.MAXIMIZED | ||
await main_window_probe.wait_for_window("WindowState.MAXIMIZED is a no-op") | ||
assert main_window_probe.get_window_state() == WindowState.NORMAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again - I know I've said on multiple past reviews that these tests need to test every transition from every state to every state - and, more than that, it needs to be clear that this is what is being tested.
You have the "matrix" test further down the page; that test should be first, as it is the most important functional test.
Once you've got that test, it's not clear what is being added by other tests (like this one). We're already validating that the window can be maximized, from every other state. Why do we need an explicit "can be maximized from NORMAL test?
else: # pragma: no cover | ||
# Marking this as no cover, since the above cases cover all the possible values | ||
# of the FormWindowState enum type that can be returned by WinForms.WindowState. | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As previously - you avoid the pragma by using one of the actual cases as the bucket case.
If there's literally no difference, then you're correct - there's no value in having a duplicated probe. There's analogs to this elsewhere in the system. If you want to know the contents of TextInput, there's really only one way to do that - ask the text input for its value. That's the API that is used by the actual backend... and it's the API that a probe implementation would also need to use. However, there is a probe implementation to get the current text of a TextInput - because there are transforms that are performed on literal text (null to empty string conversations etc) That's a good guide for when a probe method is needed as well. If there's an "unvarnished truth" to be extracted, then add a probe; otherwise, just use and validate the public API. In this case, it looks like there's no difference; so having the test exercise the API and validate the APIs then reflect the correct state values is sufficient. |
I've logged this as #2796. In general, if you find a bug that exists in main, the best strategy is to report that bug, not cram it into a ticket discussion. It's probably worthwhile fixing that bug independent of this PR, so that it's clear what part of this PR is the fix for Cocoa presentation mode, and what part is the addition of a new feature. |
I am investigating why self.interface and self.impl are None in the window delegate events. The reason might be because interface and impl are assigned after TogaWindow delegate is initialized: toga/cocoa/src/toga_cocoa/window.py Lines 154 to 161 in 44c4262
But during the initialization, certain delegate events are always triggered like To ensure that both interface and impl are assigned first, I tried to do: class TogaWindow(NSWindow):
interface = objc_property(object, weak=True)
impl = objc_property(object, weak=True)
@objc_method
def initTogaWindow(self, interface, impl):
self.interface = interface
self.impl = impl
mask = NSWindowStyleMask.Titled
if self.interface.closable:
mask |= NSWindowStyleMask.Closable
if self.interface.resizable:
mask |= NSWindowStyleMask.Resizable
if self.interface.minimizable:
mask |= NSWindowStyleMask.Miniaturizable
self = self.initWithContentRect(
NSMakeRect(0, 0, 0, 0),
styleMask=mask,
backing=NSBackingStoreBuffered,
defer=False,
)
return self self.native = TogaWindow.alloc().initTogaWindow(self.interface, self) But I cannot directly pass python objects to objc classes and so I got the TypeError: TypeError: Don't know how to convert a toga.window.MainWindow to a Foundation object Which is expected as mentioned in the rubicon docs, and is probably the reason for why it is currently set up like it is. I then tried to first initialize the window and later assign the delegate in order to avoid triggering delegate events when interface and impl are not assigned. class WindowDelegate(NSObject, protocols=[ObjCProtocol("NSWindowDelegate")]):
interface = objc_property(object, weak=True)
impl = objc_property(object, weak=True)
... ...
self.native = NSWindow.alloc().initWithContentRect(
NSMakeRect(0, 0, 0, 0),
styleMask=mask,
backing=NSBackingStoreBuffered,
defer=False,
)
self.window_delegate = WindowDelegate.alloc().init()
self.window_delegate.interface = self.interface
self.window_delegate.impl = self
self.native.delegate = self.window_delegate
... But the delegate events were still triggered when interface and impl were not assigned. Finally, I looked at: toga/cocoa/src/toga_cocoa/window.py Lines 34 to 36 in 44c4262
Here if I remove weak=True from both: interface = objc_property(object)
impl = objc_property(object) Then it works correctly and there is no AttributeError when accessing interface or impl. As, per the rubicon docs:
I am guessing that not declaring the property as weak reference will cause memory leak issues, am I correct? Also, I don't understand how does making it a not weak reference, makes it work correctly, even though interface and impl are assigned after NSWindow initialization: toga/cocoa/src/toga_cocoa/window.py Lines 154 to 161 in 44c4262
|
Fixes #1857
Design discussion: #1884
The following are the API changes:
On the interface:
toga.constants.WindowState
WindowState.NORMAL
WindowState.MAXIMIZED
WindowState.MINIMIZED
WindowState.FULLSCREEN
WindowState.PRESENTATION
toga.Window
toga.Window.state
(getter)toga.Window.state
(setter)toga.Window.full_screen
(getter)toga.Window.full_screen
(setter)toga.App
toga.App.is_in_presentation_mode
(getter)toga.App.enter_presentation_mode()
toga.App.exit_presentation_mode()
toga.App.is_full_screen
(getter)toga.App.set_full_screen()
toga.App.exit_full_screen()
On the backend:
toga.Window
get_window_state()
set_window_state()
toga.App
enter_presentation_mode()
exit_presentation_mode()
However, I did encounter some issues, which I have put as inline comments. I do have some other questions about rubicon for implementing the new API, but I will post them later on.
TODO: Write and modify documentation, fix issues with tests, implement for the other remaining platforms, etc.
PR Checklist: