Added support for maximize, minimize, full_screen in WinForms #1874

proneon267 · 2023-04-16T14:33:40Z

Added support for maximize, minimize, full_screen in WinForms.

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 · 2023-04-16T16:57:19Z

This adds supports for the following:

On a toga.App basis:

toga.App.maximized = True/False
print(toga.App.maximized)              # Returns True if any of the app's windows are maximized.

toga.App.minimized = True/False
print(toga.App.minimized)              # Returns True only if all of the app's windows are minimized.

toga.App.full_screen = True/False
print(toga.App.full_screen)            # Returns True if any of the app's windows are in fullscreen.

toga.App.set_normal_screen()           # Restores to initial window state of all the app's windows.

# These are still present as they are declared in the core API & docs and we need to keep backwards compatibility

toga.App.set_full_screen()
toga.App.exit_full_screen()

On a window by window basis:

window.maximized = True/False
print(window.maximized)               # Returns True if the window is maximized.

window.minimized = True/False
print(window.minimized)               # Returns True if the window is minimized.

window.full_screen = True/False
print(window.full_screen)             # Returns True if the window is in fullscreen.

window.set_normal_screen()            # Restores window to the initial window state

proneon267 · 2023-04-16T17:01:05Z

Skipping `test_full_screen_set` until all implementations are properly completed on their respective platforms.

This is because the new solution doesn't return a shadow variable and instead checks the _window_state for calculating the result.

Hence, there will be no return of value in platforms where there is no implementation.

I have Manually tested on Windows for this case and it works fine.

proneon267 · 2023-04-16T17:39:17Z

@freakboy3742 Take a look at this and let me know what you think.

freakboy3742

There's still some core API design issues here; I've flagged them inline.

freakboy3742 · 2023-04-17T01:50:45Z

core/src/toga/window.py

@@ -8,6 +9,14 @@
 from toga.widgets.base import WidgetRegistry


+@unique
+class WindowStates(Enum):


Enum classes should be singular, not plural, so they read as constants: WindowState.NORMAL.

freakboy3742 · 2023-04-17T01:53:57Z

core/src/toga/app.py

+    def set_normal_screen(self):
+        """Sets the app's windows in normal state"""
+        self._impl.set_normal_screen(self.windows)
+


This collection of changes doesn't make a whole lot of sense. An app can't be maximised - only a window can be. There is maybe an argument for an app-wide minimise, as a "minimise all, or a "minimise these windows" that mirrors the existing set_full_screen() call.

Speaking of - there's an existing set_full_screen() API, so having a property as well doesn't make sense.

Yes an app-wide as well as per window basis window state operations is what I aimed for the API.

But, we now have window state properties, so we can use them instead of functions like set_maximize_screen() or set_minimize_screen(). Hence, I have removed them to keep a clean API and provide a single way to do a thing.

If there will be maximized,minimized properties, then it's natural to have a full_screen property also to have an API continuity between the various windows states properties.

set_full_screen() is still there as it was defined in the core API and was also specified in the docs. So, for backwards compatibility, it is still present.

Sure - but that doesn't address "set_normal_screen()".

freakboy3742 · 2023-04-17T01:54:39Z

core/src/toga/window.py

+        return self._impl.minimized
+
+    @minimized.setter
+    def minimized(self, condition: bool):


"condition" isn't an especially meaningful variable name; I'd suggest minimize.

freakboy3742 · 2023-04-17T01:55:00Z

core/src/toga/window.py

+        return self._impl.maximized
+
+    @maximized.setter
+    def maximized(self, value: bool):


I'd suggest maximize rather than the generic "value".

freakboy3742 · 2023-04-17T01:57:33Z

core/src/toga/window.py

    @property
    def full_screen(self):
-        return self._is_full_screen
+        return self._impl.full_screen


The impl layer shouldn't be a property - it should be anis_full_screen() method.

freakboy3742 · 2023-04-17T01:58:27Z

core/tests/test_window.py

+    # instead checks the _window_state for calculating the result.
+    # Hence, there will be no return of value in platforms where
+    # there is no implementation. I have Manually tested this case
+    # on windows for this case and it works fine.


No - you can't ever disable a test, especially on the Core API. The backend implementations shouldn't matter - because the core tests run against the dummy.

freakboy3742 · 2023-04-17T01:59:59Z

core/src/toga/app.py

+        if condition:
+            self._impl.set_maximize_screen(self.windows)
+        else:
+            self._impl.set_normal_screen(self.windows)


why explicit set_maximize_screen() calls, rather than set_window_state()?

freakboy3742 · 2023-04-17T02:01:00Z

core/src/toga/window.py

+        if value:
+            self._impl.set_maximize_screen()
+        else:
+            self._impl.set_normal_screen()


As above - why explicit set_maximize/normal_screen() calls, rather than a single set_window_state()?

freakboy3742 · 2023-04-17T02:04:32Z

changes/1874.feature.rst

@@ -0,0 +1 @@
+Added support for maximize, minimize, full_screen in WinForms.


Suggested change

Added support for maximize, minimize, full_screen in WinForms.

An API for maximising and minimising windows was added.

Also - this should be tied to the ticket discussing the feature: #1857 - rather than the PR.

proneon267 · 2023-04-17T02:18:55Z

Thanks. I will work on the changes as you requested and push another commit.

proneon267 · 2023-04-17T21:01:38Z

I have identified the problems and solved them. I will publish the changes soon.

proneon267 · 2023-04-18T13:29:47Z

The APIs are as: #1874 (comment)
@freakboy3742 Please take a look at this and let me know what do you think.

freakboy3742

More comments from another review.

Keep in mind that API design is key here. There is an existing API for part of the feature you're describing, so that existing API (and implementation needs to be accommodated. 2 parallel implementations isn't a viable option here.

freakboy3742 · 2023-04-18T18:38:20Z

core/src/toga/window.py

@@ -54,7 +63,8 @@ def __init__(
        self._impl = None
        self._app = None
        self._content = None
-        self._is_full_screen = False
+
+        self.window_state = WindowState.NORMAL


In general, we should be deferring to the backend for this, preferring any native mechanism for determining actual current state over a proxy variable.

The problem is in the backend there is no native differentiation between a full screened window and a maximized window state. Hence, the proxy variable.

But that's a backend issue. If Windows doesn't provide a way to differentiate, that's something the Windows backend can work around with a proxy variable.

Sure, I will change accordingly in the next commit.

freakboy3742 · 2023-04-18T23:36:44Z

core/src/toga/app.py

+    def set_normal_screen(self):
+        """Sets the app's windows in normal state"""
+        self._impl.set_normal_screen(self.windows)
+


Sure - but that doesn't address "set_normal_screen()".

freakboy3742 · 2023-04-18T23:38:33Z

core/src/toga/app.py

+        for window in self.windows:
+            window.set_normal_screen()
+
+    # For backwards compatibility in core API


Backwards compatibility that is only visible with a code comment isn't really backwards compatibility.

This should be causing a DeprecationWarning, and should then be implemented in terms of the new API, rather than maintaining a parallel implementation.

I will modify it to add a DeprecationWarning and change the internal implementation. But I am not sure what do you mean by "address set_normal_screen()". Can you tell me what is the intended behaviour you want with set_normal_screen API?

I want to know why it is needed at all. As an API entry point, it's completely inconsistent with all the other methods - there's no "inverse", and it's a method, not a property. Why isn't maximized=False (or minimized=False) sufficient? Why is an explicit method needed for "normal" state? Or, if there is a need for a method - why isn't it set_window_state(), rather than a normal specific method?

I mean there might be situations where the app developer doesn't care about what state a particular window is in and just wants the window to not remain in maximize, minimize or full_screen state.

Making normal=True/False as a property doesn't make much sense as doing normal=False doesn't make any logical sense.

Hence, there is a method set_normal_screen().

Sure - but then, why not have set_window_state()? That means you have an explicit API to handle all window states, rather than a single method that stands out as a different pattern to all the others. Yes, window.set_window_state(WindowState.MAXIMIZED) is exactly the same as window.maximized=True... but that's a documentation issue.

@freakboy3742 I agree we can have a set_window_state() as a single method. Do you want it to become a core API? If so, please confirm, so that I will change it in the next commit.

Yes, I agree a core-level API is called for; however, to be consistent, it should be a window_state property with a getter and setter, not a set_window_state() method.

I'd also suggest that the property is state on the Window class, and window_state on the App class.

Note that this doesn't change the backend API, which only requires the get/set_window_state() method (as all the core APIs can be expressed based on that single underlying API).

As an aside - this sort of back-and-forth is why writing up finalising design tickets like #1857 before implementing a feature is important. The discussion on #1857 had lots of ideas, but it didn't really get to a final concrete approved design. Rather than go through lots of code churn where the comments are 50% code style and 50% API design questions, it's better to sort out a complete design first.

I agree, we should have finalized API first. In the future, I will first discuss with you and then start on the implementation.

Anyways, I will implement as per your suggestion and submit a commit.

freakboy3742 · 2023-04-18T23:40:06Z

core/src/toga/app.py

    @property
    def is_full_screen(self):
        """Is the app currently in full screen mode?"""
        return self._full_screen_windows is not None

+    # For backwards compatibility in core API


This isn't backwards-compatibility - it's maintaining a feature set: the ability to set a selection of windows full screen.

I will modify it to add a DeprecationWarning and change the internal implementation.

freakboy3742 · 2023-04-18T23:41:45Z

core/src/toga/app.py

+                window.full_screen = True
+        else:
+            for window in self.windows:
+                window.full_screen = False


This approach won't work with the Cocoa implementation. This needs to be done in a batch, so that Cocoa can assign windows to screens.

This is likely true of other platforms as well - there's no point making all the windows full screen on the same screen.

I am not sure that I understand you correctly.

Doingtoga.App.full_screen = True means that all the windows of the app will be set to full screen. If only selected windows need to be full screened then the user can do window.full_screen = True on those windows.

Also what do you mean by "assign window to screens"? Are you taking about multi-monitor setup or something similar?

I'm saying look at the existing implementation of the exiting feature on Cocoa. The API that you've proposed here won't work.

And yes - it's the multi-monitor situation that is the problem here. The full-screen feature exists specifically because of multi-screen support. It was added to support Podium - where you have 2 screens - a slide presentation screen, and a speakers note screen. But - you can have multiple "presentation" documents open, and you only want 2 windows to be made full screen, and you need a specific window to go to screen 1.

proneon267 · 2023-04-20T13:06:17Z

I have modified as per the suggestions. There is now complete gtk support and partial cocoa support. However, please test the cocoa implementation and let me know how it goes. The gtk backend also needs testing but not as much as the cocoa backend.

Now, there are no shadow variable for getting the current window state. The state is retrieved directly. All the window operations are done in the screen where the windows are present.

But, I couldn't write a proper test for dummy core. I think the problem is incorrect setting and retrieving of a shadow variable state which leads to set_window_state() not getting called. The shadow variable was needed here as there is nothing to retrieve the current window status in the dummy backend.

@freakboy3742 Please take look at this and let me know how should I proceed.

freakboy3742 · 2023-04-21T03:55:30Z

But, I couldn't write a proper test for dummy core. I think the problem is incorrect setting and retrieving of a shadow variable state which leads to set_window_state() not getting called. The shadow variable was needed here as there is nothing to retrieve the current window status in the dummy backend.

The dummy backend should behave like a real backend. If a real backend is "stateful"... then the dummy backend should dummy out that state. This can be done by directly adding an attribute to the dummy widget (see the ProgressBar dummy), or by using the set_value/get_value methods of the dummy (see the Slider dummy).

freakboy3742

This pass doesn't feel any closer to the final goal - in fact, it feels further away from being mergable in many respects, because the implementation you've provided here has changed a bunch of the behavior and API surface of the previous implementation, and it's completely altered the implementation of existing features.

I think we need to stop iterating on implementations, and actually finish the design discussion; otherwise, we're going to keep going round in circles without getting any closer to a working design.

freakboy3742 · 2023-04-21T03:56:25Z

cocoa/src/toga_cocoa/window.py

+                NSNumber.numberWithBool(True), forKey="NSFullScreenModeAllScreens"
+            )
+
+            # Find the screen on which the window is present


This isn't what the previous implementation did - and it's not a change that will satisfy the use case for which the previous implementation existed.

freakboy3742 · 2023-04-21T03:58:09Z

cocoa/src/toga_cocoa/app.py

+
+    def set_window_state(self, window_state):
+        for window in self.interface.windows:
+            window.state = window_state


Why is get_window_state an app level method here? The Window is the object that has state.

proneon267 · 2023-04-28T14:47:04Z

Closing this and opening a new one to prevent non meaningful commit messages.

proneon267 added 6 commits April 16, 2023 07:23

Added support for maximize, minimize, full_screen.

3eed95b

Added a changelog file.

5d7d13b

Modified to tackle test errors.

6034a20

Modified to fix test errors.

d3aee40

Made changes to tackle test errors.

7fe14c3

Skipping test_full_screen_set.

855d1cb

Fixed the DocString to tackle error.

feb747a

freakboy3742 requested changes Apr 17, 2023

View reviewed changes

proneon267 and others added 4 commits April 17, 2023 22:23

Merge branch 'beeware:main' into patch-3

622f012

Made changes as per the suggestions.

6c02ba8

Resumed the test test_full_screen_set.

3db2b71

Modified to tackle errors in test.

83420e5

proneon267 force-pushed the patch-3 branch from 3dc7cad to 83420e5 Compare April 17, 2023 21:02

proneon267 requested a review from freakboy3742 April 17, 2023 21:03

proneon267 added 2 commits April 18, 2023 06:10

Made changes to refine the API.

de7d4d7

Removed an invalid function check from dummy test.

c0cee0e

freakboy3742 requested changes Apr 18, 2023

View reviewed changes

proneon267 and others added 3 commits April 19, 2023 12:34

Merge branch 'beeware:main' into patch-3

f041071

Merge branch 'beeware:main' into patch-3

c61f6bd

Refined the API and support for gtk, cocoa.

74db17d

proneon267 requested a review from freakboy3742 April 20, 2023 13:06

freakboy3742 requested changes Apr 21, 2023

View reviewed changes

freakboy3742 mentioned this pull request Apr 21, 2023

Added Support for all cursor functions in WinForms #1864

Closed

4 tasks

proneon267 closed this Apr 28, 2023

proneon267 deleted the patch-3 branch June 17, 2024 14:57

		@@ -0,0 +1 @@
		Added support for maximize, minimize, full_screen in WinForms.

	Added support for maximize, minimize, full_screen in WinForms.
	An API for maximising and minimising windows was added.

Added support for maximize, minimize, full_screen in WinForms #1874

Added support for maximize, minimize, full_screen in WinForms #1874

Conversation

proneon267 commented Apr 16, 2023 • edited Loading

PR Checklist:

proneon267 commented Apr 16, 2023 • edited Loading

proneon267 commented Apr 16, 2023

Skipping test_full_screen_set until all implementations are properly completed on their respective platforms.

proneon267 commented Apr 16, 2023

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

proneon267 Apr 17, 2023 • edited Loading

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 Apr 17, 2023

proneon267 commented Apr 17, 2023 • edited Loading

proneon267 commented Apr 18, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proneon267 commented Apr 20, 2023 • edited Loading

freakboy3742 commented Apr 21, 2023

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

proneon267 commented Apr 28, 2023

proneon267 commented Apr 16, 2023 •

edited

Loading

proneon267 commented Apr 16, 2023 •

edited

Loading

Skipping `test_full_screen_set` until all implementations are properly completed on their respective platforms.

proneon267 Apr 17, 2023 •

edited

Loading

proneon267 commented Apr 17, 2023 •

edited

Loading

proneon267 commented Apr 20, 2023 •

edited

Loading