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 "Show in system tray" option #142

Merged
merged 5 commits into from Dec 26, 2018
Merged

Add "Show in system tray" option #142

merged 5 commits into from Dec 26, 2018

Conversation

@ericcornelissen
Copy link
Contributor

@ericcornelissen ericcornelissen commented Nov 24, 2018

As requested in #137, added an option to the settings to enable or disable the tray icon. The current implementation requires the user to restart FluentTerminal in order for the option to take affect (and they're informed about this in the UI)

system_tray_option_preview

Ideally the system tray icon should just appear/disappear as the option is toggled, but I couldn't get that to work. The SettingsService in Program.cs does not respond to the ApplicationSettingsChanged event because the Settings views use the SettingsService in App.xaml.cs, so I couldn't start/stop the tray icon from there. And in App.xaml.cs I do not have access to Application in order to start/stop the tray icon, as far as I know.

@hanskokx
Copy link
Contributor

@hanskokx hanskokx commented Nov 24, 2018

@ericcornelissen
Copy link
Contributor Author

@ericcornelissen ericcornelissen commented Nov 24, 2018

I think we should re-think the copy on this. "Requires restart to take affect" implies that the computer needs to be rebooted, to me. (...). My suggestion: "Relaunch required to apply setting"

Fair point, perhaps including the word "program" or "process" helps with clarity as well 👍 I'll wait for @felixse's review before updating it though.

Also, it should be effect, not affect.

Oops 😅

@felixse
Copy link
Owner

@felixse felixse commented Nov 28, 2018

Code looks great 👍 , but @skipmeister123 is right, we should improve the wording. Another thing we could add is a 'Quit' in the hamburger menu, right now there is no way to kill the Backend process besides Task Manager

@ericcornelissen
Copy link
Contributor Author

@ericcornelissen ericcornelissen commented Dec 1, 2018

Updated the text to "Relaunch required to apply setting" as @skipmeister123 suggested

Another thing we could add is a 'Quit' in the hamburger menu, right now there is no way to kill the Backend process besides Task Manager.

I would prefer that closing the last window kills the backend process, somehow I hadn't noticed the program kept running with the tray disabled 😅

But still then, adding that to the hamburger menu is not a bad idea.

@ericcornelissen
Copy link
Contributor Author

@ericcornelissen ericcornelissen commented Dec 18, 2018

After trying to kill the background process using Application.Current.Exit(); and CoreApplication.exit(); but failing, I did a little research and it seems that Microsoft doesn't want programmers to kill background processes (for more info see App lifecycle or search on e.g. "uwp kill own process") and instead let the process continue in the background as long as there is enough resources available...

Unless you want to do some special trickery it seems that the current implementation is the best we can do (also, no hamburger menu item), and to be honest I'm okay with that since the CLIs themselves are terminated.

What do you think @felixse?

@felixse felixse merged commit 49e09f3 into felixse:master Dec 26, 2018
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@ericcornelissen ericcornelissen deleted the ericcornelissen:options/dont-use-tray branch Dec 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants