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

Option to toggle the system tray icon #23220

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

Conversation

BLM16
Copy link
Contributor

@BLM16 BLM16 commented Jan 9, 2023

Summary of the Pull Request

Adds the ability to toggle the system tray icon through a setting in the General tab.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Closes #9526 by contributing the functionality to hide the tray icon. This does not remove the icon process, which the modules depend on, but rather uses Windows' dwState and dwStateMask and only hides the icon from view. This does not impact the functionality of the modules or settings.

Validation Steps Performed

  • Automated testing succeeds
  • Built and tested the toggle observing the settings file
  • Confirmed the hide icon context menu option exists
  • Confirmed the icon disappears from the system tray when it should
  • Confirmed the exit button in the settings UI kills PowerToys

Todo

  • Ensure/fix running the executable opens the current settings menu
  • Call the icon visibility change function when IPC is notified of a change to the setting
  • Add a context menu option to hide the system tray icon
  • Add a button in settings to exit PowerToys both in the absence of an icon and as an intuitive place for such an option

@BLM16
Copy link
Contributor Author

BLM16 commented Jan 9, 2023 via email

@BLM16 BLM16 changed the title Added option to toggle the system tray icon Option to toggle the system tray icon Jan 9, 2023
@htcfreek
Copy link
Collaborator

htcfreek commented Jan 9, 2023

@crutkas
Can we accept this PR? The issue is closed. And how does someone opens the settings menu without icon?

@BLM16
Copy link
Contributor Author

BLM16 commented Jan 9, 2023

And how does someone opens the settings menu without icon?

If PowerToys is running, launching the shortcut or the executable again finds the running system tray process and calls the settings command bound to it as if you right clicked and pressed it yourself.

This is not working currently with my dev environment so I need to make sure this works before marking this not a draft.

@@ -2870,4 +2870,10 @@ Activate by holding the key for the character you want to add an accent to, then
<data name="TextExtractor_Languages.Header" xml:space="preserve">
<value>Preferred language</value>
</data>
<data name="GeneralPage_HideSystemTrayIcon.Description" xml:space="preserve">
<value>You will not be able to access PowerToys settings once you do this.</value>
Copy link
Collaborator

Choose a reason for hiding this comment

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

When it's said like this, that sounds very... Let's say unlogical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote that as a temporary comment until I could implement the fix to launching PowerToys as a duplicate instance correctly launching settings. I will be pushing that here soon with a more descriptive message.

@Jay-o-Way
Copy link
Collaborator

Jay-o-Way commented Jan 9, 2023

If PowerToys is running, launching the shortcut or the executable again finds the running system tray process and calls the settings command bound to it as if you right clicked and pressed it yourself.

Unfortunately, that does not seem to be the case.

P.S. 765 is dup'ed against #9526

Note
Removing the tray icon requires a set of steps that can be implemented ahead of adding the actual option to hide the icon.

@BLM16 Looks like this is much more complicated...

@BLM16
Copy link
Contributor Author

BLM16 commented Jan 10, 2023

If PowerToys is running, launching the shortcut or the executable again finds the running system tray process and calls the settings command bound to it as if you right clicked and pressed it yourself.

Unfortunately, that does not seem to be the case.

I commented on this in #8277 and will be including that fix in my next push. My fix seems to have resolved the issue.

@BLM16 BLM16 closed this Jan 10, 2023
@BLM16 BLM16 reopened this Jan 10, 2023
@BLM16 BLM16 marked this pull request as ready for review January 10, 2023 02:01
@BLM16
Copy link
Contributor Author

BLM16 commented Jan 15, 2023

There are numerous steps under #9526 that I would like to clarify.

  • Creating a new icon for the bug report tool & documentation about the bug tool.
    • Is this necessary? This seems unrelated to hiding the tray icon itself. People accept the loss of functionality such as quick access to the bug report tool by enabling this.
  • Documentation about changes affecting the Exit command.
    • I have not added any option to exit PowerToys when the icon is hidden. When enabled, it would act as a service that would not need to be exited.
    • PowerToys can always be killed via task manager if needed.
    • An exit option at the bottom of general settings could be implemented if desired which would be self explanatory.
    • What sort of documentation needs to be written with regards to closing the application?
  • Specs for changes affecting the tray icon context menu items.
    • What needs to be clarified here? Hiding the icon removes the ability to use the context menu. Is this related to the future flyout window?
  • Documentation for the settings option to hide the tray icon.
    • What needs to be documented here? Toggling the "Hide system tray icon" switch along with the description of how to access settings again seems self explanatory.

What of these needs to be done? It seems to me like the setting itself along with how all other similar tools operate means documentation is self explanatory and theoretically shouldn't be needed.

Does the bug report tool change need to be included in this feature? Should it not be migrated to a separate issue to be included in the roadmap?

@BLM16 BLM16 requested a review from Jay-o-Way January 15, 2023 00:17
@Jay-o-Way
Copy link
Collaborator

Jay-o-Way commented Jan 15, 2023

Thanks for asking me, but those things are beyond me 😇 I would point to @enricogior, but he fell off the grid...
Best I could do try the branch on my computer in the coming days...

@Jay-o-Way Jay-o-Way removed their request for review January 15, 2023 16:57
@jaimecbernardo
Copy link
Collaborator

We're currently extending the system tray icon functionalities: #22408

Why is the change in this PR required?
Indeed I don't think it can be accepted right now since the tray icon is the only way (in the UI, not talking about directly terminating the process) to exit PowerToys and creating a Bug Report.

@Jay-o-Way
Copy link
Collaborator

Why is the change in this PR required?

Hundreds of people have been asking for this?

@BLM16
Copy link
Contributor Author

BLM16 commented Jan 16, 2023

We're currently extending the system tray icon functionalities: #22408

Why is the change in this PR required?
Indeed I don't think it can be accepted right now since the tray icon is the only way (in the UI, not talking about directly terminating the process) to exit PowerToys and creating a Bug Report.

If the user wishes to use PowerToys as a service, their choice to remove the UI flyout is their own.

There is already a link at the top of the settings menu (at least general settings) that links to the bug report.

My thought was adding a button to the settings menu (would have to determine an ideal location if you have any ideas) that exits the program.

If any functionality is desired once the icon has been hidden, it is simply a matter of relaunching PowerToys via the executable and disabling the hide icon setting.

@htcfreek
Copy link
Collaborator

Please have in mind that the context menu entry in tray creates a bug report file and the link in settings app opens the GitHub page.

I personally don't like this change because it brings us trouble.

@BLM16
Copy link
Contributor Author

BLM16 commented Jan 16, 2023

Please have in mind that the context menu entry in tray creates a bug report file and the link in settings app opens the GitHub page.

I think this is a product design issue overall actually, as different methods of reporting a bug feels inconsistent. Currently my bug report option from the context menu doesn't work (I am investigating this before creating an issue), however different methods of reporting bugs seems rather unmanageable and confusing.

@Jay-o-Way
Copy link
Collaborator

Oh by the way, is common that the menu itself has a "hide menu" item too 💡

@BLM16
Copy link
Contributor Author

BLM16 commented Jan 16, 2023

Oh by the way, is common that the menu itself has a "hide menu" item too 💡

That is one of the features I plan to implement soon. Which part of the context menu do you think should this go under?

@Jay-o-Way
Copy link
Collaborator

Above exit should be fine if you ask me

@crutkas
Copy link
Member

crutkas commented Jan 19, 2023

As @jaimecbernardo called out, we are in the process of extending the system tray icon functionalities, #22408 to add in a quick launcher and super charge that menu.

I do understand the user ask, but I feel like this is a partial solution to what was called out.

Two big items is I don't think are actually addressed that the root issue did call out as needed implementation

  • How to know PT is running / How do you quit
    • To me, this i think is a larger Q, what is the design here.
  • How to report a bug report
    • I think guidance in the bug report we may be able to solve with direct EXE @jaimecbernardo assuming this would work, a direct exe call

@BLM16
Copy link
Contributor Author

BLM16 commented Jan 19, 2023

As @jaimecbernardo called out, we are in the process of extending the system tray icon functionalities, #22408 to add in a quick launcher and super charge that menu.

Yes, and I think this is a great idea. A Win11 action center style flyout makes total sense for PowerToys. If people wish to run PT as a service (simply using the keyboard shortcuts for a few modules, or setting FancyZones once and leaving them) they can hide the PT icon with no repercussions (once the quit/bug report are fixed). I think giving people the option since they have asked is a good idea, leaving the decision up to them about whether or not they want it.

  • How to know PT is running / How do you quit

I cannot think of any other way to indicate PT is running, but that is the same as tons of other apps and services that are background processes. The icon will be visible by default, and it would only be hidden by people who don't want it. As such, the users who have hidden the icon know that the process is running (unless they quit or don't have run on start enabled). The use case for hiding the icon would be for people who aren't going to be frequently changing anything and simply wish for it to run in the background.

As for quitting, I am going to add the ability to stop PT directly from the settings menu, which I think should be implemented regardless of whether this PR is merged. Add the ability to quit alongside all the other settings options where you would expect.

  • How to report a bug report

I think it is important to determine how bugs are to be reported firstly. There are 2 different methods where the icon shortcut launches the exe, but the link from settings redirects to the github issues. This should be standardized (unless I am unaware of the reason for this). The link from settings would report the bugs fine. In the case of reporting a bug regarding opening the settings window (this was mentioned in one of the related issues), the bug report tool being a standalone process would make more sense, where the user could simply launch the executable themselves as you mentioned.

@Jay-o-Way
Copy link
Collaborator

How to report a bug report

Do #7423

@Jay-o-Way
Copy link
Collaborator

FYI can't seem to build. There's about 238 errors from the ImageResizer project.

@BLM16
Copy link
Contributor Author

BLM16 commented Jan 19, 2023

FYI can't seem to build. There's about 238 errors from the ImageResizer project.

Interesting. That probably happened during my rebase merge to pull main in along with my commit. The build succeeded in CI though... Are you able to build the commit prior to integrating the context menu option?

@BLM16
Copy link
Contributor Author

BLM16 commented Jan 19, 2023

How to report a bug report

Do #7423

This is possible, however I again think this is out of the scope of this PR. Standardizing the bug report and fixing things like this should be integrated through a separate PR.

@jaimecbernardo
Copy link
Collaborator

Hi @BLM16 , indeed with recent changes to PowerToys, I think this PR can be reviewed.
I'm assuming it would still be possible to Exit PowerToys / generate a Bug report with the changes here.
Can you please merge main in and resolve conflicts? Would love to give this a test.
Thank you!

@niels9001 niels9001 self-requested a review February 6, 2024 11:49
Copy link
Contributor

@niels9001 niels9001 left a comment

Choose a reason for hiding this comment

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

I think the naming of this setting is not inline with what Windows normally uses.

I'd assume that in the UI we'd show a CheckBox or ToggleSwitch with a label saying something like "Show PowerToys icon in system tray", and this would be turned on by default (like it always was), and can now be turned off.

This is inline with how W11 Settings does this:
image

cc @Jay-o-Way @trajano as a follow-up to your earlier comment

@niels9001
Copy link
Contributor

@BLM16 Do you mind posting a few screenshots of what the UI changes look like?

@@ -145,6 +146,10 @@
x:Uid="Feedback_NavViewItem"
Icon="{ui:FontIcon FontFamily={StaticResource SymbolThemeFontFamily}, Glyph=&#xED15;}"
Tapped="FeedbackItem_Tapped" />
<NavigationViewItem
Copy link
Contributor

Choose a reason for hiding this comment

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

@crutkas @ethan-fang-MS Not a fan of this as we're trying to declutter the NavView. I also don't have any other suggestions - any ideas from your side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be an ideal use of a File ribbon menu, however that doesn't exist unfortunately. I am open to any other ideas, but this was the best I could come up with (beyond placing the exit option randomly on one specific page, but that feels awkward and not easy to find).

@BLM16 BLM16 force-pushed the system-tray-hide-icon branch from 761ae41 to 9690c64 Compare February 14, 2024 22:23
@BLM16 BLM16 force-pushed the system-tray-hide-icon branch from 9690c64 to eab7772 Compare February 14, 2024 23:33
@BLM16
Copy link
Contributor Author

BLM16 commented Feb 15, 2024

Hi @BLM16 , indeed with recent changes to PowerToys, I think this PR can be reviewed. I'm assuming it would still be possible to Exit PowerToys / generate a Bug report with the changes here.

It is not possible to do either when the icon is hidden via shortcuts or anything, however another PR I made fixed the singleton issue, so running PowerToys again binds to the active process and people can exit/bug report from the UI or re-enable the icon.

Can you please merge main in and resolve conflicts? Would love to give this a test. Thank you!

I rebased this onto main and resolved the conflicts. For some reason I cannot build this locally as WinRT is using a different version (though VS says I have the correct version and SDKs installed). Any ideas on this? Does it work for you? I cannot see all the issues as the build exits early in places due to said error.

@BLM16
Copy link
Contributor Author

BLM16 commented Feb 15, 2024

Is anyone able to build and test this? I am still trying to diagnose my configuration/version issues and cannot build this to test if it functions as expected.

@jaimecbernardo
Copy link
Collaborator

Hi @BLM16 ,
Merging and building seems to work for me.
I've merged it in this PR as well.
Perhaps you have some temp build files that need to be cleared.
Have you tried running git clean -fxd on your local repo? (Careful, this will delete all files that are not added to git)

@trajano
Copy link

trajano commented Feb 29, 2024

In the screenshot it shows PowerToys.Runner can that be changed as well so it does not look so developer-y.

I'd assume that in the UI we'd show a CheckBox or ToggleSwitch with a label saying something like "Show PowerToys icon in system tray", and this would be turned on by default (like it always was), and can now be turned off.

This is inline with how W11 Settings does this: image

Comment on lines +208 to +214
if (json::has(general_configs, L"hide_tray_icon", json::JsonValueType::Boolean))
{
hide_tray_icon = general_configs.GetNamedBoolean(L"hide_tray_icon");
// Update tray icon visibility when setting is toggled
set_tray_icon_visible(!hide_tray_icon);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The addition of hide_tray_icon in apply_general_settings is well-implemented, and toggling the tray icon visibility based on this setting improves user control. Consider adding a brief comment to explain the specific use case or behavior, as it may help maintainers understand its purpose during future updates.

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 16 changed files in this pull request and generated no comments.

Files not reviewed (10)
  • src/runner/Resources.resx: Language not supported
  • src/runner/general_settings.cpp: Language not supported
  • src/runner/general_settings.h: Language not supported
  • src/runner/main.cpp: Language not supported
  • src/runner/resource.base.h: Language not supported
  • src/runner/tray_icon.cpp: Language not supported
  • src/runner/tray_icon.h: Language not supported
  • src/settings-ui/Settings.UI/SettingsXAML/Views/GeneralPage.xaml: Language not supported
  • src/settings-ui/Settings.UI/SettingsXAML/Views/ShellPage.xaml: Language not supported
  • src/settings-ui/Settings.UI/Strings/en-us/Resources.resw: Language not supported
@chenmy77 chenmy77 self-requested a review February 21, 2025 06:09
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.

Option to hide the tray icon