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

Better autostart state display and handling #293

Closed
8 tasks done
Spiritreader opened this issue Oct 8, 2021 · 14 comments
Closed
8 tasks done

Better autostart state display and handling #293

Spiritreader opened this issue Oct 8, 2021 · 14 comments
Labels
enhancement New feature or request in progress

Comments

@Spiritreader
Copy link
Member

Spiritreader commented Oct 8, 2021

Describe the enhancement or feature you'd like
Enhancement derived from #259 (comment) by @tooomm

Idea is to create a settings section with the title "Autostart" that

  • informs the user which mode is active (logon task, normal autostart)
  • provides a checkbox to disable/enable it
  • provides fine grained control about the autostart state, without affecting the default behavior of Auto Dark Mode

How to implement

  • create a service api method that checks which (or if any) autostart mode is active
  • add a section to the settings UI
  • provide autostart validation if the entries are wrong / missing
  • remove dependency of autostart from AutoThemeSwitching
  • globally enable autostart for auto dark mode unless disabled

@Armin2208, please let me know what you think. Close the issue if you don't want sth like this.

@Spiritreader Spiritreader added the enhancement New feature or request label Oct 8, 2021
@Spiritreader
Copy link
Member Author

Progress Update
image

Spiritreader added a commit that referenced this issue Oct 9, 2021
@tellmewhy12
Copy link

image
It just updated and the "Autostart central" looks like this for me ... even though I didn't disable it anywhere

@Spiritreader Spiritreader reopened this Oct 10, 2021
@Spiritreader
Copy link
Member Author

Spiritreader commented Oct 10, 2021

image I
t just updated and the "Autostart central" looks like this for me ... even though I didn't disable it anywhere

please take a screenshot of the entries in this regkey section:

Computer\HKEY_CURRENT_USER\SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\StartupApproved\Run

And as always, include the service.log.
Please, please, please, please, everyone, always include the service.log file for ANY kind of issue.

@tooomm
Copy link
Contributor

tooomm commented Oct 10, 2021

With this change, will disabling autostart also remove any registry or scheduler entries?

You did mention that @Armin2208 intentionally did not allow this for some reason in #259 (comment).
I wonder why this behavior is needed or wanted.


Maybe the Refresh Autostart button can be considered redundant and disabling & re-enabling the setting has the same effect?


Edit: Other than that I like it!!

@Armin2208
Copy link
Member

A disable button for Autostart isn't necessary, because User can just deactivate it in Windows Settings or Task-Manager.
@Spiritreader integration is great, with a refresh button. I like it this is why we will and want to keep it :)
Just the Login Task instead of Autostart entry checkbox should move below the Toggle Switch.

@Spiritreader
Copy link
Member Author

Spiritreader commented Oct 10, 2021

With this change, will disabling autostart also remove any registry or scheduler entries?

You did mention that @Armin2208 intentionally did not allow this for some reason in #259 (comment). I wonder why this behavior is needed or wanted.

Maybe the Refresh Autostart button can be considered redundant and disabling & re-enabling the setting has the same effect?

Edit: Other than that I like it!!

If you want a technical explanation, I can give you one. But as with all design decisions, they're not that simple.

The new autostart handler supports the following methods:

  • GetAutostartState, which returns if any auto start entry is set (either via task scheduler or regkey)
  • AddAutostart, which will override any current setting and just create a new autostart entry
  • RemoveAutostart, which will remove all autostart entries cleanly
  • ValidateAutostart, which will check if the AutomaticThemeSwitch flag is set, and perform a GET, then a MODIFY if the autostart entries do not fulfill the following criteria:
    • the autostart registry key is missing, or does not point to the current App Base directory (this will auto-migrate the entry)
    • the autostart task scheduler entry is missing, or does not point to the current App Base directory
    • for all other scenarios where the autostart entries are valid, no set operation is performed. Auto Dark Mode will only perform writes when necessary

As you can see, Validate != Set/Remove.
The Refresh Autostart button will validate and modify the entries, whereas toggling the switch overwrites them.
Calling a blind set is no longer a good idea, since so many people mess it up. Thus there is a validation in place that will log exactly what has been done from now on.

Toggling the switch thus will remove all entries when set to off.
However, due to numerous issues dealing with people accidentally disabling autostart even without the toggle, you will get a message box popup asking you if you are really really really sure you want to disable it

In a addition to that:

  • selecting a different radio button option on the time page will automatically call AddAutostart
  • starting the app will automatically perform ValidateAutostart
2021-10-10 15:45:40 | Debug | ZeroMQServer.Start: received message: --alive 
2021-10-10 15:45:40 | Info | MessageParser.Parse: signal received: request for running status 
2021-10-10 15:45:41 | Debug | ZeroMQServer.Start: received message: --validate-autostart 
2021-10-10 15:45:41 | Info | MessageParser.Parse: signal received: validate autostart entries 
2021-10-10 15:45:41 | Debug | AutoStartHandler.AddAutostart: autostart mode selected 
2021-10-10 15:45:41 | Warn | AutoStartHandler.ValidateAutostart: auto start validation failed, wrong path. fixing autostart

The validation behavior is especially useful, because that way auto-validation can be optionally disabled by setting the Config.Autostart.AutoValidate value to false.
Without this possibility I would lose my mind because I use three different instances of Auto Dark Mode when testing.

@Spiritreader
Copy link
Member Author

@Armin2208 Done!!

autostartUI2

Pushed to beta 🚀

@tellmewhy12
Copy link

service.log
image
@Spiritreader Alright I will keep it in mind!

@Spiritreader
Copy link
Member Author

Spiritreader commented Oct 10, 2021

service.log image @Spiritreader Alright I will keep it in mind!

That does look like it is disabled tho.
What does it say when you open task manager and go to the startup tab?

Auto Dark Mode did not incorrectly detect the startupapproved value, a binary value starting with "03" means that the entry has been disabled!

enabled

image

disabled

image

@tellmewhy12
Copy link

tellmewhy12 commented Oct 10, 2021

image
@Spiritreader yes It seemed it was disabled - I didn't notice I had that many Autostart entries left. I forgot I put it in shell's Autostart which probably opened it.
image
It works now 👍 ❤️

@tooomm
Copy link
Contributor

tooomm commented Oct 11, 2021

@Spiritreader Thanks for your detailed explanation! That helped a lot.
I did also install the beta now to better understand the new autostart UI/UX alongside the details you provided.

A validate method makes sense and the button has an important value in that case. Well thought out!
The app calling validate on start and re-applying autostart settings is an interesting approach and should help with many issues.

The only thing I do not like about it is that the user is not informed that something he might have changed on purpose at on one point is now reversed in secret without him knowing.
A toast notification or an in-app banner (better than a disruptive pop-up) or some other Autostart re-enabled hint would be nice.

Toggling the switch thus will remove all entries when set to off.

That's all I wanted to hear, perfect! 👍

@Spiritreader
Copy link
Member Author

The only thing I do not like about it is that the user is not informed that something he might have changed on purpose at on one point is now reversed in secret without him knowing.

Hmm I believe there is something I can do that prevents this scenario altogether.

@Spiritreader Spiritreader reopened this Oct 11, 2021
@Spiritreader Spiritreader changed the title Provide better info section about autostart state Better autostart state display and handling Oct 11, 2021
@Spiritreader
Copy link
Member Author

Spiritreader commented Oct 11, 2021

Alright, Auto Dark Mode now behaves in the following way:

  • When the Auto Dark Mode App is launched for the first time (user has never installed ADM before), it will automatically add itself to autostart, regardless of what setting is configured via the Time page. The default behavior of the application and the service is to run in the background anyway, no reason to wait until people enable automatic theme switching
  • When Auto Dark Mode App is launched subsequently (as in, not a first time launch), it will perform validate

So how does that get rid of silent autostart adds?

Because this way I was able to modify the "Start Auto Dark Mode with Windows" toggle:

  • Disabling Autostart via the toggle will now also disable validation, which means on all subsequent starts of the App, it will no longer attempt to correct missing autostart entries (and therefore, not try to set them)
  • Enabling Autostart via the toggle will now enable validation automatically, such that subsequent app starts will trigger validation again

Validation can also be disabled via the yaml config file, and as long as noone touches the toggle setting in the app, it will stay disabled. (This is useful for people like me who run Auto Dark Mode for personal use as standalone, and debug on another build)

This results in returning true governance over the autostart behavior to the user as it can be permanently disabled now, and will auto-validate again when enabled!

How does that conform with the "make it impossible to accidentally disable autostart" policy?

Since Autostart is still enabled by default, and disabling it comes with significant resistance (found on the bottom of the settings page + message box telling people not to), it is pretty much impossible to do this unknowingly.

This solution should fulfill the two requirements that have been drafted here:

  • enable full control over autostart entries
  • ensure auto start is enabled by default and well, self aware 😄 if it needs to correct itself

Thanks for your input @tooomm!
You've been essential in designing a meaningful improvment for the autostart behavior
My outlined changes here have been pushed to beta (10.0.0.32).
You might have to toggle autostart to off again in order for the new behavior to kick in.

@tooomm
Copy link
Contributor

tooomm commented Oct 12, 2021

I like it a lot.
You turned my input into a great solution @Spiritreader. 😉👏

One tiny nitpick:
Once autostart is disabled, the refresh button has no use and should be greyed out as well. Similar to the logon task checkbox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress
Projects
None yet
Development

No branches or pull requests

4 participants