Added APIs for detecting multiple displays and setting windows on them.

[proneon267]

Implements the features described in #1884 for detecting multiple displays.

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

[freakboy3742]

A couple of comments about the specific implementation; the comments I've added to the Cocoa implementation also apply to the other backends.

There's one other major comment: How do I test this?

As you should have seen from the last couple of PRs from you that I've merged, I've added manual test cases. This is just formalising what I have to do myself - in order to validate that a new feature works, I need to have a way to exercise that API. I presume you have tested this code as well - one way to prove to me that you have is to provide your testing app.

In this case, there's no need for an entirely new app - you can use the existing Window example app. Add a row of buttons where the number of buttons equals the number of screens returned by the screens API. Pressing button 1 moves the window to screen 1, and so on.

[freakboy3742]

I need some advice on how to review this.

Yes, there is now an example app. However...

1. It doesn't work in the multiple screen case... which is the one case you'd care about for a feature adding multiple screens. It always sends the window to the last discovered screen, because the screen variable is a closure over a variable that is modified in the loop.
2. It raises an error on macOS because an attribute is being used as a function
3. On GTK, it raises numerous warnings about using deprecated APIs
4. On Windows, the new buttons don't appear in the app's window unless you resize the app window. This isn't a bug that you've introduced (beyond the "the buttons all fitted previously"), but it's odd you didn't mention it, since it's a notable problem.

If there are known gaps in an implementation (e.g., I haven't been able to test this on macOS) it's a courtesy to let the reviewer know so that they're able to restrict the core of their review to what you, as the developer, have confirmed actually works.

If you're not able to verify the implementation at all... I have to wonder why you've decided to implement this feature. I understand that it's an offshoot of the original "min/max" API... but at no point in the process of discussing the implementation of this have you raised any potential issues with testing this feature; nor have you asked the question about whether this feature is a necessary pre-requisite of implementing the min/max APIs that we iterated on.

Outside of those bugs; it's not clear why the behavior you've implemented is "move the window to the origin on the new display" (and, on macOS, that position is bottom left, not top left of screen). Why not preserve the on screen position, but on a new window? Sure, not all windows have the same resolution, but most are similar; and would seem a lot less surprising to me.

I've flagged a couple of other things that stood out; but it's a little frustrating to be asked to review something when it appears significant portions of it haven't been tested.

[proneon267]

If there are known gaps in an implementation (e.g., I haven't been able to test this on macOS) it's a courtesy to let the reviewer know so that they're able to restrict the core of their review to what you, as the developer, have confirmed actually works.
If you're not able to verify the implementation at all... I have to wonder why you've decided to implement this feature.

You are correct. I apologize for any inconvenience caused due to this commit. I should have mentioned that thorough testing had not been done. Moving forward, I will always specify the testing status of the commit.

It doesn't work in the multiple screen case... which is the one case you'd care about for a feature adding multiple screens. It always sends the window to the last discovered screen, because the screen variable is a closure over a variable that is modified in the loop.

I have addressed this issue in the latest commit.

On GTK, it raises numerous warnings about using deprecated APIs

I have addressed the deprecated APIs and updated the code to follow the proper APIs. The deprecation warnings were raised as Gdk.Screen will be deprecated. Hence, I have replaced the deprecated APIs with appropriate APIs from Gdk.Display and Gdk.Monitor.

It raises an error on macOS because an attribute is being used as a function

Could you please also provide the specific error message, since I do not have access to a mac and haven't tested on it.

On Windows, the new buttons don't appear in the app's window unless you resize the app window. This isn't a bug that you've introduced (beyond the "the buttons all fitted previously"), but it's odd you didn't mention it, since it's a notable problem.

I mean the main box was using a Toga.Box instead of a Toga.ScrollContainer, so I thought it was intentional. I can modify it to use a Toga.ScrollContainer, if that is what you intended.

Also, a related question, is there any particular reason for there to be a separate Toga,ScrollContainer and Toga.Box, instead of a singular Toga.Box with options to specify overflow behavior - scroll, hide, etc. ?

Outside of those bugs; it's not clear why the behavior you've implemented is "move the window to the origin on the new display" (and, on macOS, that position is bottom left, not top left of screen). Why not preserve the on screen position, but on a new window? Sure, not all windows have the same resolution, but most are similar; and would seem a lot less surprising to me.

Currently, the windows are set at the origin of the display to which they are moved. On MacOS, the origin happens to be the bottom left. Since, it is a native behavior, I thought of not messing with it to change it to the top left.

If a window will be originally positioned between 2 displays, then preserving the original position can make it seem like the window didn't move to the other display.

but it's a little frustrating to be asked to review something when it appears significant portions of it haven't been tested.

Won't happen again. I will now always attach the test status of the latest commit before requesting a review.

For the current commit, I have:

• Windows - Completely tested with multi monitor (triple monitor) setup.
• Gtk - Tested with dual monitor setup.
• MacOS - Not tested, but should work as per the usage of the APIs.

[freakboy3742]

It doesn't work in the multiple screen case... which is the one case you'd care about for a feature adding multiple screens. It always sends the window to the last discovered screen, because the screen variable is a closure over a variable that is modified in the loop.

I have addressed this issue in the latest commit.

I don't see how. I can't see any change to the demo app.

It raises an error on macOS because an attribute is being used as a function

Could you please also provide the specific error message, since I do not have access to a mac and haven't tested on it.

I've pushed an update that fixes the API error (as well as simplifying the use of generators in favor of a simple comprehension. There's no benefit to a generator in this case).

On Windows, the new buttons don't appear in the app's window unless you resize the app window. This isn't a bug that you've introduced (beyond the "the buttons all fitted previously"), but it's odd you didn't mention it, since it's a notable problem.

I mean the main box was using a Toga.Box instead of a Toga.ScrollContainer, so I thought it was intentional. I can modify it to use a Toga.ScrollContainer, if that is what you intended.

It shouldn't need to be a scroll container. There's no specifier of window size; the window should size itself to fix the content. However, as I noted, this isn't a problem of your making.

Also, a related question, is there any particular reason for there to be a separate Toga,ScrollContainer and Toga.Box, instead of a singular Toga.Box with options to specify overflow behavior - scroll, hide, etc. ?

No specific reason, beyond the fact that they're implemented with different widgets on every platform except the web.

Outside of those bugs; it's not clear why the behavior you've implemented is "move the window to the origin on the new display" (and, on macOS, that position is bottom left, not top left of screen). Why not preserve the on screen position, but on a new window? Sure, not all windows have the same resolution, but most are similar; and would seem a lot less surprising to me.

Currently, the windows are set at the origin of the display to which they are moved. On MacOS, the origin happens to be the bottom left. Since, it is a native behavior, I thought of not messing with it to change it to the top left.

Look at the implementation of set_position(). There's no point having a cross-platform API if it has different behavior on every platform.

If a window will be originally positioned between 2 displays, then preserving the original position can make it seem like the window didn't move to the other display.

I'm not sure we're meaning the same thing. I mean "if the window is currently at 100,100 on screen 1, and I move to screen 2, it should be at 100,100 on screen 2".

As an example why this matters: consider the behavior of window.screen = window.screen.

but it's a little frustrating to be asked to review something when it appears significant portions of it haven't been tested.

Won't happen again. I will now always attach the test status of the latest commit before requesting a review.

For the current commit, I have:

• Windows - Completely tested with multi monitor (triple monitor) setup.
• Gtk - Tested with dual monitor setup.

I don't see how this can be true, because the test app does not work as currently implemented. I can only assume your test regimen is "click the first display button", and see that the window moved screen. If you use all the available screen-move buttons, you should see that every button directs the window to the same screen.

[proneon267]

I don't see how. I can't see any change to the demo app.
...
I don't see how this can be true, because the test app does not work as currently implemented. I can only assume your test regimen is "click the first display button", and see that the window moved screen. If you use all the available screen-move buttons, you should see that every button directs the window to the same screen.

My apologizes. It seems that I had made changes to the demo app in a separate local branch and had forgot to merge with the working branch which was pushed to the origin. I have committed and pushed the changes of the demo app and now it should work as expected.

I'm not sure we're meaning the same thing. I mean "if the window is currently at 100,100 on screen 1, and I move to screen 2, it should be at 100,100 on screen 2".

I agree that preserving the window positions would be better.

I also propose that get_position() and set_position() should return and set the window positions relative to the screen on which the window is present.

Since, there will be dedicated APIs for multi-screen, hence users would want to set window positions on a particular screen, instead of manually calculating the coordinates themselves.

[ItsCubeTime]

Since, there will be dedicated APIs for multi-screen, hence users would want to set window positions on a particular screen, instead of manually calculating the coordinates themselves.

I would propose having the option to do it both ways. In some cases it could be easier to set window position by 1 coordinate system that includes all windows. Like for instance: In my app I currently have a window drag functionality that offsets the windows position as the user moves the mouse - this would be very difficult for me to do if I had to take monitors into account

[freakboy3742]

I would propose having the option to do it both ways. In some cases it could be easier to set window position by 1 coordinate system that includes all windows. Like for instance: In my app I currently have a window drag functionality that offsets the windows position as the user moves the mouse - this would be very difficult for me to do if I had to take monitors into account

To clarify the use case here - is the issue being able to position the window on other screens relative to the current screen, or having an absolute position that is always present? i.e, if window.position = (-1000, 200) positioned a monitor to the left of the current screen, rather than the primary screen - would that work for you? Essentially I'm trying to work out if we need a second position "absolute position" API, or an API that lets you specify position relative to a given screen (with the default being the current screen for a window)

[ItsCubeTime]

Yes, I think there should be an absolute position API. Maybe like window.position_absolute = (1920, 1080) (assuming that (0,0) means top left, this would set the windows top left corner to bottom right of your top left monitor, so if this was a quad monitor setup with 4 monitors in a 2x2 grid, the window would be displayed in the top left of the bottom right monitor) if window.position is going to be for position relative to a particular display.

[freakboy3742]

Ok - on that basis, I'm going to say we should make window.position relative to the current screen top-left, preserving the ability to move a window to a separate screen by providing a negative or larger-than-screen-size coordinate. We can also look at adding an window.absolute_position attribute, which is always relative to screen 0's origin - but we can add that as a separate API if you want.

[proneon267]

Hello
I'll research some more, before we finalize about the position thing. However, due to my current semester exams, I won't be able to give time here for a couple of days or weeks. But do note that I'll complete this PR, the related issue and other PRs or designs, albeit only after my exams. Until then, this PR will lay dormant.

Thanks
proneon267

[freakboy3742]

@proneon267 Understood. Good luck with the exams; we'll see you back when your free time returns!

[proneon267]

Hello, I am back :)
I looked and tested out the current implementation of window.position. It supports negative values to move window between displays. I think since the exisiting API works, we do not need to change or create a new API. Moreover, it will not be breaking any existing APIs. What do you all think?

[freakboy3742]

I looked and tested out the current implementation of window.position. It supports negative values to move window between displays. I think since the exisiting API works, we do not need to change or create a new API. Moreover, it will not be breaking any existing APIs. What do you all think?

Yes, position supports negative values - but the position is relative to the Screen 0 origin (i.e., it's always absolute position, rather than relative position), which wasn't what we discussed when we left off this conversation pre-exams.

One other detail thats stand out from a quick re-review: The issue of preserving position as you move across desktops still exists. On macOS, the window is always positioned at the origin of the new screen - which, in native macOS coordinates, is the bottom left. I haven't tested Windows and GTK, but from a quick read, I suspect a similar problem exists.

Two other details that have emerged in your absence:

1. In the time you've been away, we've got to almost 100% code coverage in testing. [widget Audit] toga.Window #2058 is currently in preparation, adding full unit tests for all of Window; and I suspect App will be fully tested within a week or two. In the past, we've let testing slide because testing coverage wasn't that thorough; however, now that we have coverage, we're going to maintain it - so testing coverage of new features is now a requirement.
2. As part of this testing review, we've been leaning away from "not_required_on", in favor of "no-op" APIs on mobile platforms. So - we'll need to add a dummy implementation on iOS, Android and web that returns a single screen.

[proneon267]

Hello,
I agree with you and I would like to follow your earlier recommendation:

Ok - on that basis, I'm going to say we should make window.position relative to the current screen top-left, preserving the ability to move a window to a separate screen by providing a negative or larger-than-screen-size coordinate. We can also look at adding an window.absolute_position attribute, which is always relative to screen 0's origin - but we can add that as a separate API if you want.

Based on it, I am suggesting to do something like this on the backend( Example: WinForms):

    def get_position(self):
        # Returns relative window position
        current_screen = self.get_current_screen()
        current_origin = current_screen.native.Bounds.X, current_screen.native.Bounds.Y
        current_abs_window_position = self.get_abs_position()
        current_relative_position = (
            current_abs_window_position[0] - current_origin[0],
            current_abs_window_position[1] - current_origin[1],
        )
        return current_relative_position

    def set_position(self, position):
        # Set relaive window position
        current_screen = self.get_current_screen()
        current_origin = current_screen.native.Bounds.X, current_screen.native.Bounds.Y
        new_position = (
            position[0] - current_origin[0],
            position[1] - current_origin[1],
        )
        self.set_abs_position(new_position)

    def get_abs_position(self):
        return self.native.Location.X, self.native.Location.Y

    def set_abs_position(self, position):
        self.native.Location = Point(*position)

And as for preserving the original window position, a simple change of origin should be good like:

    OriginalWindowLocation = window.Location
    OriginalOrigin = WinForms.Screen.AllScreens[0].Bounds.X, WinForms.Screen.AllScreens[0].Bounds.Y
    NewOrigin = WinForms.Screen.AllScreens[1].Bounds.X, WinForms.Screen.AllScreens[1].Bounds.Y
    NewWindowLocation = (
    OriginalWindowLocation.X - OriginalOrigin[0] + NewOrigin[0],
    OriginalWindowLocation.Y - OriginalOrigin[1] + NewOrigin[1],
    )

    window.StartPosition = WinForms.FormStartPosition.Manual
    window.Location = Drawing.Point(NewWindowLocation[0],NewWindowLocation [1])

What do you think?

As for the unit test and dummy backends, I will write them after we finalize the window position things.

[freakboy3742]

What do you think?

I think a code snippet in a comment is probably the worst way to review this sort of thing.

However, it's not immediately clear to me that this requires 2 platform specific implementations. The implementation API is "set position". The public interface API of "set screen relative position" and "set absolute position" can both be expressed in a single underlying implementation API, and the math of "position of a window relative to the screen it is on" should be identical (AFAICT) on every platform.

My other thought is whether we should reverse the naming to avoid a backwards incompatibility - retain position as the "global" position, and use screen_position as the new API that is screen relative.

[proneon267]

Yeah, I should have submitted a new commit for review. I agree with you that:

we should reverse the naming to avoid a backwards incompatibility - retain position as the "global" position, and use screen_position as the new API that is screen relative.

Also:

However, it's not immediately clear to me that this requires 2 platform specific implementations. The implementation API is "set position". The public interface API of "set screen relative position" and "set absolute position" can both be expressed in a single underlying implementation API

Do you mean there should be a single backend function called set_position() that takes an extra argument to indicate if the passed position are absolute or relative?

the math of "position of a window relative to the screen it is on" should be identical (AFAICT) on every platform.

You mean the origin should be at the top left of the screen on every platform?

[freakboy3742]

Yeah, I should have submitted a new commit for review. I agree with you that:

we should reverse the naming to avoid a backwards incompatibility - retain position as the "global" position, and use screen_position as the new API that is screen relative.

Also:

However, it's not immediately clear to me that this requires 2 platform specific implementations. The implementation API is "set position". The public interface API of "set screen relative position" and "set absolute position" can both be expressed in a single underlying implementation API

Do you mean there should be a single backend function called set_position() that takes an extra argument to indicate if the passed position are absolute or relative?

No - I mean that there should be a single backend function called set_position() that takes an absolute position, and it's the public interface API that converts a relative position into an absolute position. This is because (AFAICT) the logic to convert a relative position to an absolute position is identical across all platforms, once you've got a cross-platform way to get the origin of each screen.

the math of "position of a window relative to the screen it is on" should be identical (AFAICT) on every platform.

You mean the origin should be at the top left of the screen on every platform?

I'm not sure what you're asking. There is not single "origin". The origin of the screen specific set position API should be the top left of the screen in question. The origin of the absolute set position API should be the top left of the "primary" screen. Regardless of platform, origin is always top left of a screen; the only question is which screen.

[proneon267]

Just a heads up, since #2155 is not merged, the current dpi scaling implementation is non-functional. So, try to run any app at 100% dpi scaling for expected and correct results.

[freakboy3742]

Just a heads up, since #2155 is not merged, the current dpi scaling implementation is non-functional. So, try to run any app at 100% dpi scaling for expected and correct results.

Ack - I've just rolled back the scaling changes that I made; I think we can wait until #2155 for a full fix (or, at least, merge this and fix the winforms scaling issue as part of #2155).

[proneon267]

Thanks. I'll try to complete that PR as soon as I get a review.

[freakboy3742]

This is now passing on my macOS install, so it looks like we've finally nailed the retina fixes - nice work!

I've pushed a few other minor cleanups; so this PR is looking good... except for one thing that showed up in final testing: the take screenshot button on the window example doesn't seem to work - it's returning a "coroutine was never awaited" error.

At least part of the cause - the screen name is \\.DISPLAY1... and the filename being saved is screenshot_{screen.name}.png.... and hilarity ensues :-) I'm not familiar with Windows naming conventions for screens, but I wonder if it might be preferable to strip the \\.\ part of the display name.

[proneon267]

Sure. I'll just quickly remove it and try to run the tests.

[proneon267]

I have removed the non-text part but even before removing it, the screenshot button worked correctly and didn't return the error that you got.
I'll try to run on other platforms and check if I get the same error on any of them.

[proneon267]

Confirmed that the coroutine was never awaited error only occurs on macOS.

[freakboy3742]

Confirmed that the coroutine was never awaited error only occurs on macOS.

I don't see this error on macOS. I only see it on Winforms.

[proneon267]

Does this script produce the same error on winforms:

"""
My first application
"""

from functools import partial
import toga
from toga.style import Pack
from toga.style.pack import COLUMN, ROW, RIGHT


class HelloWorld(toga.App):
    async def do_save_screenshot(self, screen, window, **kwargs):
        screenshot = screen.as_image()
        path = await self.main_window.save_file_dialog(
            "Save screenshot",
            suggested_filename=f"Screenshot_{screen.name}.png",
            file_types=["png"],
        )
        if path is None:
            return
        screenshot.save(path)

    def startup(self):
        """
        Construct and show the Toga application.

        Usually, you would add your application to a main content box.
        We then create a main window (with a name matching the app), and
        show the main window.
        """
        screen_as_image_btns_box = toga.Box(
            children=[
                toga.Label(
                    text="Take screenshot of screen:",
                    style=Pack(width=200, text_align=RIGHT),
                )
            ],
            style=Pack(padding=5),
        )
        for index, screen in sorted(enumerate(self.screens), key=lambda s: s[1].origin):
            screen_as_image_btns_box.add(
                toga.Button(
                    text=f"{index}: {screen.name}",
                    on_press=partial(self.do_save_screenshot, screen),
                    style=Pack(padding_left=5),
                )
            )
        main_box = toga.Box(children=[screen_as_image_btns_box])

        self.main_window = toga.MainWindow(title=self.formal_name)
        self.main_window.content = main_box
        self.main_window.show()


def main():
    return HelloWorld()

[proneon267]

Can confirm that the error arises from the line:

Line 241: on_press=partial(self.do_save_screenshot, screen)

[proneon267]

Final Confirmation :-)

The examples/window works on python 3.10.11 but not on 3.8.9. I had 3.8.9 on macOS and got the error there but not on windows as I had a newer version there.

I'm guessing the same is true for you, you have a newer version on macOS but an older version on windows and hence got the error.

[freakboy3742]

The examples/window works on python 3.10.11 but not on 3.8.9. I had 3.8.9 on macOS and got the error there but not on windows as I had a newer version there.

I'm guessing the same is true for you, you have a newer version on macOS but an older version on windows and hence got the error.

Nope. I'm running 3.10.5 on Windows, 3.10.11 on macOS.

Your sample program works for me on macOS; fails on Winforms with the same "coroutine never awaited" error.

[proneon267]

Yeah also doesn't work on 3.10.5 and confirmed that it only works on >=3.10.6. Test it on 3.10.6 and let me know.

[freakboy3742]

Yeah also doesn't work on 3.10.5 and confirmed that it only works on >=3.10.6. Test it on 3.10.6 and let me know.

Nice catch! I can confirm that fixes the issue - although I'd be interested to know the exact bug that fixed the issue.

With that resolved, I think we're good to go - the only issue I'm aware of that remains is the HiDPI screenshot issue with Winforms, which will be addressed by #2155.

[proneon267]

Thanks. I'll make the required changes of winforms screen in #2155.