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

major-mayer commented 1 month ago

First time contributor checklist:

[x] I have read the README and Contributor Guidelines
[x] I have signed the Contributor Licence Agreement

Contributor checklist:

[x] My contribution is not related to translations.
[x] My commits are in nice logical chunks with good commit messages
[x] My changes are rebased on the latest main branch
[x] A npm run ready run passes successfully (more about tests here)
[x] 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 commented 1 month ago

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. https://github.com/signalapp/Signal-Desktop/blob/76a77a9b7fde3fc21b86e29071eb4e93b5e12ff1/ts/test-node/app/SystemTrayService_test.ts#L78

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".

major-mayer commented 1 month ago

@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.

signalapp / Signal-Desktop