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

Add App.focus() method #3034

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add App.focus() method #3034

wants to merge 1 commit into from

Conversation

raj921
Copy link

@raj921 raj921 commented Dec 9, 2024

This PR introduces a new focus() method to the App class that lets you programmatically bring the application into focus (#3032). The implementation depends on the platform:

macOS: Uses activateIgnoringOtherApps_.
Windows: Calls Form.Activate().
GTK: Doesn't do anything (window managers manage focus).
Mobile/Web: Also no-op.
The method comes with a note in the docs to remind everyone that forcing focus isn't great UX and should only be used when necessary.

Why this change?
It solves the problem of needing a way to programmatically focus the app, especially for certain workflows or edge cases.

PR Checklist:
Tested the new feature.
Updated the documentation.
Read the CONTRIBUTING.md.
Promise to follow the code of conduct.

This adds a new focus() method to App that allows programmatically bringing
the application into focus. The implementation is platform-specific:

- macOS: Uses activateIgnoringOtherApps_
- Windows: Uses Form.Activate()
- GTK: No-op (window managers control focus)
- Mobile/Web: No-op

The method includes documentation noting that programmatically stealing
focus is generally considered poor practice and should be used sparingly.
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.

Hi - thanks for the contribution!

This PR is currently raising failures on CI because of pre-commit and towncrier problems; our contribution guide contains details on what you need to do to correct these problems.

Once those problems are fixed, you'll hit a second problem - testing. You've added a new feature - that means you need to add tests. In this case you'll need a test in the core (which will test the dummy backend), and a test in the testbed. The testbed test will be a bit of a no-op because we give focus to another app; but we can exercise the API to make sure it doesn't raise an error (demonstrating that at least the APIs we're invoking actually exist).

@@ -352,7 +352,7 @@ def show_about_dialog(self):
if self.interface.author is None:
options["Copyright"] = ""
else:
options["Copyright"] = f"Copyright © {self.interface.author}"
Copy link
Member

Choose a reason for hiding this comment

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

Why have you made this change? The copyright is a valid symbol, and it displays correctly in my testing.

@@ -1020,6 +1020,16 @@ def set_full_screen(self, *windows: Window) -> None:
# End backwards compatibility
######################################################################

def focus(self):
Copy link
Member

Choose a reason for hiding this comment

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

The methods on this class are sorted; this should be added to the "Window control" section, alphabetically sorted in that section.

Comment on lines +1024 to +1030
"""Bring the application into focus.

This method will attempt to bring the application window into focus. Note that
it is generally considered poor practice for applications to steal focus from
the user, so use this method sparingly.

This is a no-op on mobile and console platforms.
Copy link
Member

Choose a reason for hiding this comment

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

We try not to call out specific platforms in docstrings; we put platform specific notes in the Notes section for the class in question.

Suggested change
"""Bring the application into focus.
This method will attempt to bring the application window into focus. Note that
it is generally considered poor practice for applications to steal focus from
the user, so use this method sparingly.
This is a no-op on mobile and console platforms.
"""Force the app to be the currently active app on the user's desktop.
This request will not be honoured by all platforms; see the :ref:`platform notes
<app-notes>` for details.
This method should be used sparingly, if at all. It is generally considered poor
practice for an application to steal focus from other applications.

(This will also require adding a .. app-notes: declaration just before the Notes header in the app documentation.)

@@ -90,7 +90,7 @@ def show_about_dialog(self):
name_and_version += f" v{self.interface.version}"

if self.interface.author is not None:
copyright = f"\n\nCopyright © {self.interface.author}"
Copy link
Member

Choose a reason for hiding this comment

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

Again - why this change?

@@ -385,3 +385,6 @@ def get_current_window(self):

def set_current_window(self, window):
window._impl.native.makeKeyAndOrderFront(window._impl.native)

def focus(self):
self.native.activateIgnoringOtherApps_(True)
Copy link
Member

Choose a reason for hiding this comment

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

The trailing underscore isn't needed here; it's a valid but historical syntax.

Suggested change
self.native.activateIgnoringOtherApps_(True)
self.native.activateIgnoringOtherApps(True)

@mhsmith
Copy link
Member

mhsmith commented Dec 10, 2024

According to this page, Form.Activate doesn't actually activate a background app, but only flashes the taskbar button. So this is apparently a feature which can only be implemented on one platform, and is highly discouraged on that one. That seems inconsistent with the Toga philosophy.

Other Microsoft sources:

@freakboy3742
Copy link
Member

According to this page, Form.Activate doesn't actually activate a background app, but only flashes the taskbar button.

Form.Activate() may be the wrong (or incomplete) implementation - this SO answer suggests some other steps are needed.

Even if Activate is the right API, the implementation here likely needs some modification - it should be using current_window rather than main_window.

So this is apparently a feature which can only be implemented on one platform, and is highly discouraged on that one. That seems inconsistent with the Toga philosophy.

Agreed this is a weird one. My counterargument would be that from a high level, the concept has a clear expression, which is why the subject has come up more than once. However bad an idea it may be, people ask for it; and I suspect being able to say "Here's the API, but be aware it won't work in a lot of scenarios and that's the platform's fault" is a lot more effective as line of argument than trying to convince people they don't want to use a feature (cf "image button" discussions).

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