Add window states API #2473

proneon267 · 2024-04-02T16:50:29Z

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`
Added	`toga.Window.state`(getter)
	`toga.Window.state`(setter)
Deprecated	`toga.Window.full_screen`(getter)
	`toga.Window.full_screen`(setter)

`toga.App`
Added	`toga.App.is_in_presentation_mode`(getter)
	`toga.App.enter_presentation_mode()`
	`toga.App.exit_presentation_mode()`
Deprecated	`toga.App.is_full_screen`(getter)
	`toga.App.set_full_screen()`
	`toga.App.exit_full_screen()`

On the backend:

`toga.Window`
Added	`get_window_state()`
	`set_window_state()`

`toga.App`
Added	`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:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

proneon267 · 2024-04-04T02:06:55Z

@freakboy3742 Can you also review this, when you are free?

freakboy3742 · 2024-04-04T02:12:15Z

Unless there's a design or implementation issue that requires feedback, I generally don't review code until it's passing CI - this is currently failing coverage and Android tests, and I'm going to guess iOS tests as well (although #2476 is likely masking that problem).

Is there a specific design or implementation issue where you're seeking feedback prior to getting the tests to pass?

proneon267 · 2024-04-04T02:19:07Z

I needed some guide regarding the Android testbed, specifically test_app.py::test_full_screen and test_presentation_mode. I explained the issue with inline comments on test_app.py.

freakboy3742 · 2024-04-04T03:09:40Z

I needed some guide regarding the Android testbed, specifically test_app.py::test_full_screen and test_presentation_mode. I explained the issue with inline comments on test_app.py.

Well - I guess my first question is why is there anything to test at all? Mobile apps don't have any concept of "full screen" or "maximized"... even the representation of "window" is pretty loose. What behaviour are you implementing here?

Beyond that - if a test passes in isolation, but not as part of the suite, that usually indicates that one of the previous tests has left the test suite in an inconsistent state. A common example in the window tests is when a test leaves an open window (sometimes due to a test failing); subsequent checks of the window count then fail.

proneon267 · 2024-04-04T03:48:56Z

Well - I guess my first question is why is there anything to test at all? Mobile apps don't have any concept of "full screen" or "maximized"... even the representation of "window" is pretty loose. What behaviour are you implementing here?

Mostly Fullscreen and presentation mode where navigation bar, title bar& menu bars remain hidden.

Beyond that - if a test passes in isolation, but not as part of the suite, that usually indicates that one of the previous tests has left the test suite in an inconsistent state. A common example in the window tests is when a test leaves an open window (sometimes due to a test failing); subsequent checks of the window count then fail.

Yes, I realize that, in this case, the test seems to fail as they don't exit the presentation mode after testing. I thought maybe adding a delay to the redraw method would work, but it doesn't. The tests only pass when I set the window state directly on the window object or run the test in isolation.

I have also tried to identify any problems with the app interface but didn't find any. The same test logic is run in test_window, but there it works properly. The app implementation of presentation mode calls the window implementation of presentation mode. So, the behaviour should be identical.

proneon267 · 2024-04-05T00:37:46Z

@freakboy3742 Also could you also please take a quick peek at the mobile platforms tests on testbed of test_app.py::test_full_screen and test_presentation mode. Their implementation is identical to that of test_window.py::test_presentation_state, since the app APIs call into the window API endpoints.

freakboy3742 · 2024-04-05T01:01:41Z

@freakboy3742 Also could you also please take a quick peek at the mobile platforms tests on testbed of test_app.py::test_full_screen and test_presentation mode. Their implementation is identical to that of test_window.py::test_presentation_state, since the app APIs call into the window API endpoints.

I have, and I've already told you the general class of problem you're hitting. You've even hinted at the problem yourself:

Yes, I realize that, in this case, the test seems to fail as they don't exit the presentation mode after testing.

If a single test isn't leaving the app in the same state that it found it... well, that's the source of your problem. That's what you need to fix.

As for "the implementation is identical"... well, the golden rule of computers applies: if in doubt, the computer is right. The computer doesn't have opinions. It runs the code it has. If you think something is identical, but testing is producing inconsistent results... something about your implementation isn't identical.

You're the one proposing this PR; the onus is on you to solve problems with the implementation. If you've got a specific question about the way Toga or the testbed operates, then I can do what I can to provide an explanation, but I can only dedicate time to resolving an open ended "why doesn't it work?" questions when the problem is on my personal todo list - which usually means it's something on Toga's roadmap, or it's the source of a bug that is preventing people from using the published version of Toga.

proneon267 · 2024-10-13T01:00:18Z

Great news! The Android testbed is passing! How? 🤔 No idea—maybe it’s magic, maybe it’s the extra delay I added, or maybe the universe just felt generous today.

proneon267 · 2024-10-15T00:16:55Z

@freakboy3742 do take a look at this whenever you are free :)

freakboy3742 · 2024-10-15T00:47:53Z

I'm aware this is ready for review. I get a notification every time you make a commit... and you make a lot of commits :-)

Please be aware that this is a large PR, so it will take a significant block of time to review - and this won't be the first time I've reviewed it - most recently, just over a week ago. As much as I'd like to provide feedback as soon as a PR lands, that sometimes isn't possible - especially with PRs that have already consumed a lot of review time.

I also need to balance the time spent doing code reviews with other work priorities. This is especially relevant at this time of year, when Python has just published a new release, so there's lots of release-related housekeeping that needs to be done - and double especially this year, because the release contains official iOS and Android support that BeeWare has contributed.

proneon267 · 2024-10-15T03:05:47Z

Absolutely, I totally understand! I’ll try to give your inbox a break from my commit spree. 😄 I know it’s a big PR, so I really appreciate you finding time for it, especially with everything else—congrats on the Python release and BeeWare’s awesome contributions! I’ll keep chipping away at things while I wait. Thanks again for juggling it all!

freakboy3742

A couple more tweaks; the macOS "pending" implementation is the only one that is a major concern at this point.

freakboy3742 · 2024-10-15T06:34:34Z

core/src/toga/window.py

-        self._impl.set_full_screen(is_full_screen)
+    def state(self) -> WindowState:
+        """The current state of the window."""
+        return self._impl.get_window_state(actual_state=False)


The meaning of "actual_state" won't necessarily be obvious to anyone who isn't deep in the implementation. It would be worth adding a comment here describing what "actual" means, and a similar clarifying comment on each platform implementation (even if that comment is to the effect that state changes are immediate, so there's no difference between actual and current state)

I'd also suggest that a name like "in_progress" might be more meaningful - it's always "actually" the window state - it just matter what "actual" you "actually" mean :-)

I have modified the parameter name to be in_progress_state and also added an explanatory comment on the core state property.

it's always "actually" the window state - it just matter what "actual" you "actually" mean :-)

I noticed what you did there 😄

freakboy3742 · 2024-10-15T06:36:56Z

android/src/toga_android/window.py

+        decor_view = self.app.native.getWindow().getDecorView()
+
+        if current_state == state:
+            return


In which case - is the check right here still needed?

freakboy3742 · 2024-10-15T07:03:13Z

cocoa/src/toga_cocoa/window.py

+
+    def _apply_state(self, target_state):
+        current_state = self.get_window_state()
+        if target_state is None:


This check can go before the retrieval of current state; if it's going to abort, there's no need to get the current state first.

Thanks, I have modified the order.

freakboy3742 · 2024-10-15T07:35:56Z

cocoa/src/toga_cocoa/window.py

+        elif target_state == current_state:
+            self._pending_state_transition = None
+            return


This doesn't seem right. Consider the case where I'm in NORMAL, and I've requested FULLSCREEN, but that transition hasn't completed.

I then request a transition to NORMAL.

get_current_state() will return the "actual" state - NORMAL. This is the same as the target state, so the pending state will be set to None, and this method will exit.

The window will then continue to complete the transition to FULLSCREEN, fire the "entered fullscreen" handler, and exit because there's no pending transition.

So - I'll be in a FULLSCREEN state, having requested NORMAL; and until the transition to fullscreen completes, window.state will report NORMAL as the current state.

I'm actually not convinced this clause is needed at all. The interface-level checks whether there's a pending transition to the requested state; the role of this method is to move the window closer to the desired "pending" state (which is whatever was requested by the user)

Consider the case where I'm in NORMAL, and I've requested FULLSCREEN, but that transition hasn't completed.

Here, the methods called and attributes set will be:

set_window_state(FULLSCREEN) self._pending_state_transition = FULLSCREEN self._apply_state(FULLSCREEN)

I then request a transition to NORMAL

set_window_state(NORMAL)

Since, self._pending_state_transition would be set, so it will do:

self._pending_state_transition = NORMAL

and then just return, without actually applying NORMAL. Then when the FULLSCREEN transition is complete, it will apply the new pending state.

I'm actually not convinced this clause is needed at all. The interface-level checks whether there's a pending transition to the requested state; the role of this method is to move the window closer to the desired "pending" state (which is whatever was requested by the user)

The reason, why it is required on non-blocking API backends is because of processing of non-blocking APIs. For example:

toga/cocoa/src/toga_cocoa/window.py

Lines 70 to 80 in 0d67322

@objc_method

def windowDidMiniaturize_(self, notification) -> None:

if (

self.impl._pending_state_transition

and self.impl._pending_state_transition != WindowState.MINIMIZED

):

# Marking as no cover, since the operation is native OS delay

# dependent and this doesn't get covered under macOS CI.

self.impl._apply_state(WindowState.NORMAL) # pragma: no cover

else:

self.impl._pending_state_transition = None

As we see here, on the completion of the non blocking API(setIsMiniaturized(True)), _apply_state() is again called, and the same state check on _apply_state() is used to terminate further processing.

toga/cocoa/src/toga_cocoa/window.py

Lines 405 to 412 in 0d67322

def _apply_state(self, target_state):

current_state = self.get_window_state()

if target_state is None:

return

elif target_state == current_state:

self._pending_state_transition = None

return

Hence, the check is required on backends with non-blocking API. However, the check is not required on backends with blocking API like WinForms and Android. So, I have removed it from those backends.

freakboy3742 · 2024-10-15T08:09:54Z

testbed/tests/window/test_window.py

+        main_window.state = initial_state
+        await main_window_probe.wait_for_window(f"Main window is in {initial_state}")
+
+        assert main_window.state == initial_state


With the change to get_window_state(), this now requires a check of the backend with "actual=False" so that we get actual window data, not just the pending state.

I have added a new property on the window probe window_probe.instantaneous_state that reports the actual window state and not the pending state. Although both instantaneous_state and state will be same on the Android backend.

freakboy3742 · 2024-10-15T08:12:55Z

testbed/tests/window/test_window.py

+        # Add delay to ensure windows are visible after animation.
+        await second_window_probe.wait_for_window("Secondary window is visible")
+
+        assert second_window.state == WindowState.NORMAL


As above, this needs to be a check of the "true" state.

I have added a new property on the window probe window_probe.instantaneous_state that reports the actual window state and not the pending state.

freakboy3742 · 2024-10-15T08:16:34Z

testbed/tests/window/test_window.py

+        assert second_window.state == initial_state
+
+        # Set to the intermediate states but don't wait for the OS delay.
+        for state in intermediate_states:


Won't this be transitioning through intermediate states that the backend may not support?

You are right, I have missed this case, it would be transitioning through unsupported window state like MINIMIZED on wayland. So, I have added a check to skip assigning the unsupported state.

I know that the tests should minimize the amount of extra logic in them. But, the other two alternatives were to remove the unsupported state from the intermediate states set at the start of the test or to skip the testing altogether.

I have reverted the change I had made, since assigning unsupported window states is a silent no-op. Trying to set the intermediate states ensures coverage on wayland.

freakboy3742 · 2024-10-15T08:17:39Z

testbed/tests/window/test_window.py

+    @pytest.mark.parametrize(
+        "intermediate_states",
+        [
+            (


It's not clear to me what this specific set of "intermediate" states are achieving. There's 2 non-trivial lists of intermediate states... why these in particular?

I agree with you that these sets of intermediate states weren't testing anything useful, especially given the blocking nature of the Android API. So, I have removed one of the two cases, leaving the other to test that rapidly assigning new window state doesn't cause any unexpected problems on the Android backend.

proneon267 · 2024-10-15T18:00:52Z

The failing macOS CI test is failing on the CI and is not reproducible on my local machine. I'll research more and report back.

proneon267 and others added 19 commits March 27, 2024 00:49

Misc Fixes

041f15b

Fixed presentation mode on cocoa, gtk, winforms

3591bb0

Fixed presentation mode core tests

3b98fe7

Added tests to testbed and backend probe methods on cocoa, gtk, winforms

06e7f25

Fixed winforms backend and testbed tests

0cf8cf8

Corrected gtk backend and core

d82c615

Fixed winforms backend and cocoa

96ef3ed

Fixed core tests

4657567

Fixed cocoa backend and tests

00491c8

Fixed testbed tests

41c1dd7

corrected testbed

60d5e43

Added android implementation

f497a1d

Fixed core

d34cb2f

Fixed core

4d790b2

Added stub implementations for iOS, textual, web

18b0cbc

Merge branch 'beeware:main' into window_states

618d647

Added mobile platform tests

b7eb117

Added changelog

cadc4cc

Restart CI

bf3511a

proneon267 and others added 4 commits May 10, 2024 20:16

Merge remote-tracking branch 'remotes/origin/main' into window_states

d61a644

Merge branch 'beeware:main' into window_states

5b58045

Minor modifications

c9a1307

Minor modifications

ab9b778

Debugging android CI failure

0d67322

freakboy3742 requested changes Oct 15, 2024

View reviewed changes

proneon267 added 6 commits October 15, 2024 06:30

replaced with

07e82f9

replaced with

8be4d8d

added new method on window probe

1692a0d

Revert commented code

f8ee69d

Revert

b82b6d0

Corrected probe method on android

247847e

Merge branch 'beeware:main' into window_states

1b624ad

proneon267 force-pushed the window_states branch from ff84e6a to 0b0faa9 Compare October 16, 2024 04:13

Diagnosing macOS-arm64 failures

5560e3d

proneon267 force-pushed the window_states branch from c9b3238 to 5560e3d Compare October 16, 2024 15:31

Merge branch 'beeware:main' into window_states

a8fed19

proneon267 mentioned this pull request Oct 22, 2024

Fixed macOS App.current_window for when a dialog is in focus #2926

Open

4 tasks

proneon267 added 9 commits October 26, 2024 20:13

Correcting testbed failure

5f0c1a2

Removed unused android code

136cdcf

Fixed testbed for macOS

8fcdbd6

Fixed testbed for Android

ef584b1

Enable additional test cases

7f28348

Removed stray comments

6e0c091

Restart CI for intermittent Android failure

f535cd6

Fix Android delay

c175b6a

Added note on state setter

bc1f902

proneon267 force-pushed the window_states branch from f44d48c to bc1f902 Compare October 27, 2024 03:14

Merge branch 'beeware:main' into window_states

e8864d5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add window states API #2473

Add window states API #2473

proneon267 commented Apr 2, 2024 •

edited by freakboy3742

Loading

proneon267 commented Apr 4, 2024

freakboy3742 commented Apr 4, 2024

proneon267 commented Apr 4, 2024

freakboy3742 commented Apr 4, 2024

proneon267 commented Apr 4, 2024

proneon267 commented Apr 5, 2024

freakboy3742 commented Apr 5, 2024

proneon267 commented Oct 13, 2024

proneon267 commented Oct 15, 2024

freakboy3742 commented Oct 15, 2024

proneon267 commented Oct 15, 2024

freakboy3742 left a comment

freakboy3742 Oct 15, 2024

proneon267 Oct 15, 2024

freakboy3742 Oct 15, 2024

freakboy3742 Oct 15, 2024

proneon267 Oct 15, 2024

freakboy3742 Oct 15, 2024

proneon267 Oct 15, 2024

freakboy3742 Oct 15, 2024

proneon267 Oct 15, 2024

freakboy3742 Oct 15, 2024

proneon267 Oct 15, 2024

freakboy3742 Oct 15, 2024

proneon267 Oct 15, 2024

proneon267 Oct 15, 2024

freakboy3742 Oct 15, 2024

proneon267 Oct 15, 2024

proneon267 commented Oct 15, 2024

	@objc_method
	def windowDidMiniaturize_(self, notification) -> None:
	if (
	self.impl._pending_state_transition
	and self.impl._pending_state_transition != WindowState.MINIMIZED
	):
	# Marking as no cover, since the operation is native OS delay
	# dependent and this doesn't get covered under macOS CI.
	self.impl._apply_state(WindowState.NORMAL) # pragma: no cover
	else:
	self.impl._pending_state_transition = None

	def _apply_state(self, target_state):
	current_state = self.get_window_state()
	if target_state is None:
	return

	elif target_state == current_state:
	self._pending_state_transition = None
	return

Add window states API #2473

Are you sure you want to change the base?

Add window states API #2473

Conversation

proneon267 commented Apr 2, 2024 • edited by freakboy3742 Loading

On the interface:

On the backend:

PR Checklist:

proneon267 commented Apr 4, 2024

freakboy3742 commented Apr 4, 2024

proneon267 commented Apr 4, 2024

freakboy3742 commented Apr 4, 2024

proneon267 commented Apr 4, 2024

proneon267 commented Apr 5, 2024

freakboy3742 commented Apr 5, 2024

proneon267 commented Oct 13, 2024

proneon267 commented Oct 15, 2024

freakboy3742 commented Oct 15, 2024

proneon267 commented Oct 15, 2024

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proneon267 commented Oct 15, 2024

proneon267 commented Apr 2, 2024 •

edited by freakboy3742

Loading