Skip to content
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

Decoupled winforms event loop from AppContext. #2112

Merged
merged 13 commits into from
Sep 15, 2023

Conversation

proneon267
Copy link
Contributor

@proneon267 proneon267 commented Sep 9, 2023

The WinForms event loop earlier depended on the MainForm to process events and run background tasks. This has been decoupled through the use of https://learn.microsoft.com/en-us/dotnet/api/system.windows.threading.dispatcher?view=windowsdesktop-7.0. Also the window closing event has been changed slightly to accomodate for this decoupling and for situations where on_close event handler of a window is not provided by the user.

Fixes #750

This Patch has been tested on Windows. The following are the test apps that have been used:

Test App 1:

import toga
from toga.style import Pack
from toga.style.pack import COLUMN, ROW
import asyncio


class HelloWorld(toga.App):
    async def print_test(self, widget, **kwargs):
        while True:
            await asyncio.sleep(1)
            print("hi")

    def startup(self):
        self.add_background_task(handler=self.print_test)


def main():
    return HelloWorld()

Test App 2:

import toga
from toga.style import Pack
from toga.style.pack import COLUMN, ROW
import asyncio


class HelloWorld(toga.App):
    async def print_test(self, widget, **kwargs):
        while True:
            await asyncio.sleep(1)
            print("hi")

    def startup(self):
        self.add_background_task(handler=self.print_test)

        self.first_window = toga.Window(title="First Window")
        self.windows += self.first_window
        self.first_window.show()

        self.second_window = toga.Window(title="Second Window")
        self.windows += self.second_window
        self.second_window.show()

        self.third_window = toga.Window(title="Third Window")
        self.windows += self.third_window
        self.third_window.show()


def main():
    return HelloWorld()

Test App 3:

import toga
from toga.style import Pack
from toga.style.pack import COLUMN, ROW
import asyncio


class HelloWorld(toga.App):
    async def print_test(self, widget, **kwargs):
        while True:
            await asyncio.sleep(1)
            print("hi")

    def startup(self):
        self.add_background_task(handler=self.print_test)

        self.main_window = toga.Window(title="Main Window")
        self.main_window.show()

        self.second_window = toga.Window(title="Second Window")
        self.windows += self.second_window
        self.second_window.show()


def main():
    return HelloWorld()

Test App 4:

import toga
from toga.style import Pack
from toga.style.pack import COLUMN, ROW
import asyncio


class HelloWorld(toga.App):
    async def print_test(self, widget, **kwargs):
        while True:
            await asyncio.sleep(1)
            print("hi")

    def startup(self):
        self.add_background_task(handler=self.print_test)

        self.main_window = toga.MainWindow(title="Main Window")
        self.main_window.show()

        self.second_window = toga.Window(title="Second Window")
        self.windows += self.second_window
        self.second_window.show()


def main():
    return HelloWorld()

The added background tasks run as expected after this decoupling.
.
Any changes and suggestions are welcome :)

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

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This failed on startup for me, but because the assembly that you're loading doesn't exist. The fact that you're referencing something in "Program Files" should be the first hint something is off - it suggests the code you're referencing is user-installed, not part of the OS itself.

winforms/src/toga_winforms/app.py Outdated Show resolved Hide resolved
core/src/toga/app.py Outdated Show resolved Hide resolved
winforms/src/toga_winforms/app.py Outdated Show resolved Hide resolved
winforms/src/toga_winforms/window.py Outdated Show resolved Hide resolved
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm this now works in my testing... but it's almost entirely opaque why. Why is Version 4.0.0.0 the right assembly to load? Why can't this assembly be found with a "normal" clr.AddReference call?

winforms/src/toga_winforms/app.py Outdated Show resolved Hide resolved
@proneon267
Copy link
Contributor Author

proneon267 commented Sep 11, 2023

I can confirm this now works in my testing... but it's almost entirely opaque why. Why is Version 4.0.0.0 the right assembly to load? Why can't this assembly be found with a "normal" clr.AddReference call?

🙂 I would have thought this was like a blackbox magic, if I hadn't implemented this myself. So, here is its working and logic:

The assembly name is very specific or else, it can't be loaded. Specifically the PublicKeyToken is important.

I have found that these assemblies when used in a C# project which uses .net framework or .net core framework using visual studio or mono then a reference to the required assembly is added in the metadata of the project. This assembly is then loaded automatically using the correct Assembly name string.

I couldn't find these Assembly Strings anywhere on the internet, hence I have extracted them from install.wim of the latest windows image and have uploaded them to: https://github.com/proneon267/dotnet-core-redist-lists/blob/098fd735aeb41313e4e1b20829a911d7162b20b5/AssemblyList_4_client.xml#L91

The assembly can be absolutely loaded with clr.AddReference but the specific location of the assembly dll should be known. EDIT: Or the Assembly String should be known.

Dotnet core earlier used the assembly references folder(that I had earlier used to load the WindowsBase.dll). But in the newer versions, it stopped using that folder. So, the clr.AddReference can only work when it will be provided with the location of the WindowsBase.dll file. EDIT: Or the Assembly String is provided to it.

When I use System.Reflection to load the assembly then it searches for it in the GAC. So, its exact location doesn't need to be known.

But, I have to say, I have sampled 5 different Windows machines(including Windows 10 & 11), and all of them had the WindowsBase.dll file at that specific location.

Anyways, the current implementation is far better and portable than the previous one and we don't need to worry about the file location.

So, that's the logic of why & how this implementation works.

I will add a link to this discussion instead, for future reference. Also, I will research more and report you back about how we should handle application closing.

@proneon267
Copy link
Contributor Author

On Further testing, it turns out that clr.AddReference can resolve assemblies using Assembly String. Hence, I have modified it to use clr.AddReference.

@proneon267
Copy link
Contributor Author

proneon267 commented Sep 11, 2023

I have refactored the code and made the behavior such that the app closes when either the main_window is closed or the last window is closed(in cases where main_window is not defined).

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have refactored the code and made the behavior such that the app closes when either the main_window is closed or the last window is closed(in cases where main_window is not defined).

As I said before: "last window" has no bearing on whether the app exits or not. The only criteria that matters is whether main_window is closed. When the close button is pressed on a MainWindow, the on_exit handler should be triggered. You can't set a user-space on_close handler on a Main Window specifically because of this.

core/src/toga/app.py Outdated Show resolved Hide resolved
winforms/src/toga_winforms/app.py Outdated Show resolved Hide resolved
@proneon267
Copy link
Contributor Author

@freakboy3742 Let me know, if this PR requires any changes.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can make out, the changes you've made to the close handler are entirely unnecessary - they're logically equivalent to the pre-existing code, so I've reverted them. I've also made some minor tweaks to the handling and documentation of the CLR import.

Otherwise, I think this is good to land. Thanks in particular for the investigation work that lead to finding the Dispatcher as an alternate basis for event handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decouple winforms event loop from AppContext
2 participants