Skip to content

Conversation

@joao-voltarelli
Copy link
Contributor

No description provided.

@joao-voltarelli joao-voltarelli marked this pull request as ready for review October 4, 2024 14:22
Comment on lines 42 to 53
if "path" in connection_selectors.keys():
connection_selectors.pop("path")

def find_window(app: Application, waiting_time=10000, **selectors) -> WindowSpecification:
if not connection_selectors:
if connect_exception:
raise connect_exception
return None

app = Desktop(backend=backend).window(**connection_selectors)
if not app.exists():
raise WindowNotFoundError(f"Unable to find an app using these criteria: {connection_selectors}")
return app
Copy link
Collaborator

Choose a reason for hiding this comment

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

This implementation idea looks great, but I think we can possibly improve it.
As of now, it will try up to timeout milliseconds to connect to the Application and only if that fails it will try the fallback method with the Desktop approach.

While the Desktop approach was introduced to handle a corner case, I believe it could be added into the timeout loop in some way for us to also check using this method if the Application connection fails as an alternative to speed up and not required that applications relying on the Desktop.window method wait the full timeout period before they can connect.

What do you think?

@joao-voltarelli
Copy link
Contributor Author

Here are some updates about the 'Application' and 'Desktop' classes...

  • Basically, both classes use the same selectors to connect to/search for an application window. The main difference is that the 'Desktop' class does not accept the path parameter.

  • Based on the new tests carried out, it didn't seem to make much sense to change the connection_to_app method because the classes use the same selectors, so the 'Application' class is always used, so there would practically never be a case where it would fall to using the 'Desktop'. (The 'Application' class can retrieve the window reference in the same way as the 'Desktop' class does).

  • From my perspective, we can keep connection_to_app as it is today but adjust the bot.app property to be of type Application | WindowSpecification. That way, if a user is using the 'Desktop' class for some reason to find the window, he would be able to pass that reference to the bot.app property and continue using our find methods normally.

@hhslepicka hhslepicka merged commit 16633b0 into main Dec 19, 2024
7 checks passed
@hhslepicka hhslepicka deleted the enh/windows-apps-connection branch December 19, 2024 20:29
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.

3 participants