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

Use isFocused() instead of isVisible() in tray service to refocus Signal window #7048

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

major-mayer
Copy link
Contributor

First time contributor checklist:

Contributor checklist:

  • My contribution is not related to translations.
  • My commits are in nice logical chunks with good commit messages
  • My changes are rebased on the latest main branch
  • A npm run ready run passes successfully (more about tests here)
  • My changes are ready to be shipped to users

Description

This PR should close #6429.
As suggested by @Sajito, it changes the SystemTrayService in a way that instead of using the isVisible method it now makes use of the isFocused method in the click handler/ menu builder to determine, whether the Signal Desktop app should be hidden or shown.
While in theory isVisible should also lead to the desired behavior, on platforms like KDE Plasma this doesn't work and reports true even if the window is out of focus.
With the isFocused method, this is not the case anymore.

I only tested this PR on Manjaro Linux running KDE Plasma 6 (Wayland), so it would be helpful to test the new behavior on other platforms as well.
I can maybe do the Windows 11 test as well.

@Sajito
Copy link

Sajito commented Oct 15, 2024

I tested this on KDE Plasma (X11 and Wayland) and on Windows. Can't test on MacOS, but from my understand should work as expected.

Couldn't test gnome yet. Since the gnome tray has no click handler, but instead opens directly the context menu, might be worth trying to check if focus is still on the application, when clicking the tray icon.

Also I think there is a test for the modified service, which mocks the isVisible call. That should be changed to isFocused.

sinon.stub(browserWindow, 'isVisible').callsFake(() => fakeIsVisible);

Last thing. This changes the behavior a little bit, so the text displayed in the context menu might need new translations.
Right now it switches between "hide" and "show". With the change it should be more like "show/focus" and "hide".

@trevor-signal trevor-signal self-requested a review October 15, 2024 14:03
@trevor-signal trevor-signal self-assigned this Oct 15, 2024
@major-mayer
Copy link
Contributor Author

major-mayer commented Oct 16, 2024

@Sajito Thanks again for your valuable input.
I've added the suggested changes to the context menu label and the test file.

Furthermore, I noticed that even with my changes, the context menu label was often incorrect, for example, when you open Signal Desktop, and then focus a different window.
In that case, the label still showed "Hide" and the click handler would consequently but incorrectly hide the window.

This is because the correct label was only re-evaluated using the isVisible() method when renderEnabled() is called, which only happens, when you click the tray icon (and start Signal for the first time), but not when you open the context menu.
So I now added an event listener for the "focus" and "blur" event and recreate the context menu with the correct items, whenever this event is fired.

This fixes the problem for me.

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

Successfully merging this pull request may close these issues.

If Signal is open in the background and the tray icon is clicked, focus Signal instead of closing it
3 participants