-
-
Notifications
You must be signed in to change notification settings - Fork 688
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
Added on_gain_focus
, on_lose_focus
, on_show
& on_hide
handlers on toga.Window
#2096
base: main
Are you sure you want to change the base?
Conversation
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 for the contribution - this is another feature that is going to hit up against #2058 (and #2075); as such, I'm hesitant to merge it without tests.
I'm also hesitant because it doesn't currently have a Cocoa implementation (that should be easy enough, but it's worth flagging); and it's not 100% obvious how this would behave on mobile. My immediate reaction (and the one raised in #2006) is that gain/lose focus might link into application lifecycle hooks (so when the app comes into the foreground, that's the "gain focus" event for both the window and the app), but there's an open question of how those signals interact with tablet platforms that allow split-screen and other multi-app modes. This is an area where some additional design is required.
Yes, I agree with you. This should not be merged currently. I will need some time to write the implementation for other backends. Like #1930, I will wait until the audits are merged, and will write the tests thereafter. As for additional design for tablet modes, I agree with you. I will research more about it and will discuss with you while implementing for mobile platforms. |
@mhsmith I need your guidance on android side. According to: https://developer.android.com/guide/components/activities/intro-activities#onpause
On the Emulator(Android 12):When starting the app, the following events are triggered: When I select the app by pressing the Recents button, only
When I press the home button, neither On a Physical Device(Android 13):When starting the app, the following events are triggered:
When I select the app by pressing the Recents button, only
When I press the home button, neither
As, you can see, Why are the documented Activity lifecycle events not being triggered as per the documentation? |
Regarding the gain/lose focus on mobile platforms like android: From my testing, the app will lose focus when either the In split screen mode (like dual app mode), suppose there are two apps A and B. App In floating window mode, the app will gain focus when the user touches the app's screen. The focus is lost when the user touches anything outside the app, like interacting with the system launcher or another app. In iOS like the cocoa, there exists But, there needs to be another handler to differentiate between the states when (the app is not visible to the user & is not receiving inputs) and (when the app is visible to the user & is receiving inputs). Hence, I would like to propose other additional handlers, What do you guys think? Also, without confirmation from @mhsmith regarding the Activity life events triggering behavior, I cannot proceed with the android implementation. Hence, I was thinking about working on the iOS implementation first. |
That's because those methods aren't included in the Android template, either in
See this page for how this is notified on Android.
Every API has a maintenance and testing cost, so I'd prefer not to add additional events unless there's a clear need for them, especially if they're only applicable to certain platforms. |
Thank you for helping. I will add default implementations for the remaining methods in the Android template and will submit a PR there after getting a stable behavior. I agree with you that additional events will incur more maintenance. I feel that the For example, the app should be put to a sleep mode(not updating the layout or text) when it is in background or What do you think? |
I have tested android implementation both on a physical device and on the emulator. I have submitted a PR at beeware/briefcase-android-gradle-template#69 so that the app focus event can be detected. |
Completed implementations of all the platforms and also added a test in the window example app. I will write the tests after the audits are merged. But I think this PR is ready for a review. Also, the CI android testbed is failing on its own for some reason. |
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.
What you've done here looks like a good pass at implementing the API as proposed in #2009; however, I think we're hitting an area where we need more design before we proceed.
The detail you've dug up as part of the Android implementation has opened a bunch of design questions about what "focus" even means at the App/Window level. What are we actually trying to achieve with these signal handlers? Is "visibility" a better metaphor than "focus" in this case? Is there any use case for a literal "focus" event on a window? Do we need to differentiate between an app that is "visible", but isn't currently accepting input events, and an app that isn't accepting input events? How does the rest of the app lifecycle map into these events (on all platforms)?
Rather than pressing forward with an implementation, I think we need to step back and come up with a consistent design for these app/window lifecycle events, and work out how they map onto all the platforms we're targeting.
Researching some more on the topic, it seems like we need 3 categories of events:
The following are the states associated with the event categories mentioned above and their implications for other event categories: Input Focus ---> Visiblity -> Hover -> Use Cases:
Who should have which event categories:
APIs are not much of a problem as the available platform APIs can be properly mapped onto the above described event categories. |
Co-authored-by: Russell Keith-Magee <[email protected]>
Apologies - the close was a stray mouse click. |
Currently on the main branch, we are ignoring window state requests on hidden windows, as changing state of hidden window causes glitches on some platforms: Lines 477 to 480 in 7f02868
So, the following cannot be done: EDIT: To confirm that the valid pathway in the above works correctly, I have added a new test. This has helped me catch the cases where the events were getting double triggered. |
On testing, I have found that on gtk, when Also, when Furthermore, it turns out that Since, we need to accomodate all of these cases, the implementation on gtk will take some time. Therefore, can I do the gtk implementation on a separate PR, and skip it here? As the implementation is working correctly on all platforms except on gtk, so skipping the gtk implementation on this PR should not be an issue. |
Sure - an implementation that works on not-all platforms is fine - in fact, if there's an underlying issue with the state change tests, then we should definitely resolve those first. As long as the tests pass reliably on all platforms (and are skipped on platforms where the implementation doesn't exist/doesn't work reliably), and it's clear in the review request what does (and doesn't) work, I'm fine with reviewing and merging. All the usual guidelines about skips apply (i.e, avoid referencing GTK specifically in the test - try to raise the skip in the probe). |
Ok, I have removed the gtk's visibility events(i.e., So, both the focus events i.e., Also, another issue is testing of the events on mobile platforms, where the app cannot be sent to background and brought forward, switch between apps, or switch to the "App switcher" screen. All of these are required to automate the testing of focus and visibility events on the mobile platforms. Since, these automated tests are currently not possible, hence I have manually tested them on a physical Android 14 device, on the Android emulator and on the iOS simulator. |
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 started doing a full review; but two things
- There's a couple of outstanding comments from my previous review pass
- It seems like there's some bigger picture API design issues that still need to be resolved - in particular, the interaction of minimise and hide.
The interaction between Show/Hide and Minimize is clearly a lot more complex than just #3105 - there's also the inconsistent behavior on macOS. On that basis, it seems like it might make sense to make the two mutually exclusive - state changes are already prevented on non-visible windows, so it doesn't seem unreasonable that visibility changes should be prevented on windows that aren't in a state that can accept them. What does it mean to hide a window in presentation mode? What does it mean to show a minimized window? At the very least, the test suite indicates that there are inconsistencies; I'd argue that the best approach for now would be to prohibit all those interactions, and add them in the future if it makes sense to do so.
dummy/src/toga_dummy/app.py
Outdated
@@ -133,9 +133,15 @@ def get_current_window(self): | |||
return self._get_value("current_window", main_window) | |||
|
|||
def set_current_window(self, window): | |||
previous_current_window = getattr(self.get_current_window(), "interface", None) |
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.
It's somewhat frustrating to be asked for a review when previous review comments have been left without a response.
core/tests/utils.py
Outdated
@@ -24,3 +24,36 @@ def _create(self): | |||
|
|||
def __repr__(self): | |||
return f"Widget(id={self.id!r})" | |||
|
|||
|
|||
def assert_window_event_triggered(window, expected_event=None): |
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.
So - this is simultaneously extremely specific, and extremely generic.
This assertion method is in the top level testing utilities... but can only be used for on/off versions of 2 very specific events. It allows the developer to check 1 specific event has been triggered... but then asserts that completely unrelated events haven't been triggered.
What I was suggesting was a very literal assert_window_gain_focus
method that asserts on_focus is fired, on_hide isn't, and then resets both mocks. It's then clear what the assertion is asserting, and requires almost no logic in the assertion.
Yes, this means there's a second assert_window_lose_focus
, and another pair for show/hide... but that's easy code to write, and easy code to validate - but it dramatically simplifies the test case making it clear what is being tested, and ensures the mocks are consistently reset. As I've noted in previous reviews on other PRs - test code should avoid being smart, because if test code is smart, it can go wrong, and therefore the test code needs to be tested.
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 have removed the assert_window_event_triggered
assertion utility and replaced it with 4 separate assertion utilities: assert_window_gain_focus
, assert_window_lose_focus
, assert_window_on_hide
, assert_window_on_show
.
|
||
second_window.show() | ||
await second_window_probe.wait_for_window(f"Showing {second_window.title}") | ||
if second_window_probe.show_unminimizes_window: # For cocoa |
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 behavior of show()
on a minimized window should either be consistent on all platforms, or show()
/hide()
should raise an error if invoked on a minimized window. We shouldn't need a test workaround here - it's an area where we're in control of the API design.
If we're going to make the behavior consistent, I'd argue the Cocoa behavior is the better interpretation - if someone is calling "show()", they're indicating they want the window to be visible, not just minimized. But I suspect the "show/hide is an error if minimized" approach might be better as it's more explicit about intent.
Co-authored-by: Russell Keith-Magee <[email protected]>
The inconsistency is that on macOS, calling But this inconsistency is only in the calling of Since, hiding a MINIMIZED window is supported by every platfform, so disabling show/hide on a MINIMIZED window, seems like a bit too restrictive. So, I think the best approach would be to produce the Cocoa's interpretation of |
I can't argue it wouldn't be restrictive to disable show/hide on MINIMIZED. The question is whether that's a better option than the alternative - whether implementing macOS's behavior on GTK and Windows is problematic. The major argument again using macOS's behaviour is that it couples two otherwise unrelated features "show/hide" and state are now closely related features. I guess there's an argument that they already are, in that you can't change the state of a hidden window; but implementing macOS's behavior doubles down on that relationship in a way that I'm not sure is helpful. Another argument against macOS's behavior is a question I raised in my previous comment - what about other window states? What does it mean to hide a window in presentation mode? In fullscreen mode? We already restrict changing window state on non-visible windows; do we need to restrict changing visibility on some window states? This will require some more experimentation to determine what is even possible. Another complication that came up this morning in a conversation with @mhsmith - macOS has an app-level concept of "Hide all windows". It's not completely clear how this would interact with window-level hide - but it clearly needs to. One last thing that might be worth exploring - are we sure that |
Implements the APIs described in #2009.
on_gain_focus
,on_lose_focus
,on_show
&on_hide
handles are available both as properties and also as initialization parameters intoga.Window
.Only tested on
WinForms
andgtk
. This will take sometime to complete for all backends.Fixes #2009
PR Checklist: