-
Notifications
You must be signed in to change notification settings - Fork 71
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
Fix Internet Detector Background/Taskbar Issues #1221
Conversation
if not load_default_settings(): | ||
# If not found, get the current settings and save them as defaults | ||
default_color_prevalence = get_current_color_prevalence() | ||
default_color = get_current_taskbar_color() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should check before saving the color that it is not pink and use a default gray if it is. This way we ensure that there can never be an error, as these bug show how tricky implementing it correct is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end, with this implementation we are setting a default (the one the user had before the tool started running), as the user can't change the color anymore. So I wonder, if it would be simpler and more reliable to just default to black and pink.
If we keep it this way, we should document somewhere (not necessary in this PR, maybe in the wiki) how the user could change the color. In the long term we could move this tool to its own project with a readme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested locally and it seems to work well 👍 I think we can merge it and continue discussing if we need further improvements afterwards. I expect our community to share feedback too. 😉
This fixes both #1216 and #1217
The issue with the wallpaper (#1216) was actually that
internet_detector
ended up in a broken state after theinstaller.vm
performed aVM-Refresh-Desktop
, which madeinternet_detector
continue to run but unable to modify the background and also display an indicator in the taskbar. This has been fixed by having a detection for ifexplorer.exe
is restarted and/or if the taskbar icon is removed, and if so, it attempts to restart itself or die gracefully so that the scheduled task can start it back up again properly.The Taskbar issue (#1217) has been resolved by storing the old/default taskbar color scheme into a registry key that will be checked if it exists before setting a new default. It then reads this value for when it adjusts back to the state of when it does not detect internet.
I have also adjusted the package to re-enable the scheduled task and also made the time to kick off every 1 minute instead of 2 minutes.