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

[WIP] Check for idle time settings that preventing computing #4634

Closed
wants to merge 3 commits into from

Conversation

Vulpine05
Copy link
Contributor

@Vulpine05 Vulpine05 commented Feb 14, 2022

Description of the Change
As I worked on #4600 I found it was possible for a user to set their computing preferences in a way that does not allow any computing. If "suspend when not idle" is enabled, and "suspend if idle for" is set to >0, it is possible for the computer to not process any tasks if the first setting is greater than the second. This commit checks for this condition, and if true, will generate an error message. The user will have to modify one of the values in order to save their preferences.

Suspend min-max error message

This PR is a work in progress. The commit only checks for local preferences in BOINC Manger. There are a few things I am not familiar with and could use some assistance:

  • This does not fix web preferences. I feel the bug is a bigger concern with web settings since someone can make an accidental change and affect multiple computers. I see there is code in the html directory, but I don't know if revising the code here will make the correction for all projects to follow. I can make an attempt to fix this on the web side, I just don't know the relation of this code to the separate projects.
  • I am not familiar with localization. I'm not sure if anything needs to be done to get the new error message translated. I'd be happy to help if someone could point me in the right direction.
  • This doesn't check existing settings in global_prefs(_override).xml when loading BOINC. Someone could manually set this, or could have had this setting accidentally and not know it. I don't know how critical it is to check for this.

Also, there are a few small items I would like a second opinion on:

  1. The warning I generated is called a validation error. I think technically this is incorrect, it should be an error. Both numbers are validated prior to the comparison. I can make a different dialogue box, but I was trying to keep the code simple.
  2. I am open to suggestions to changes to the warning message I created. I'm not sure I am happy with how it reads. I'm trying to keep the message short and simple, not long and complicated.

Alternate Designs
This could be modified so the user can accept the settings as-is with an OK/Cancel dialogue box. I don't feel this is needed, if someone wanted to suspend a computer they could do so locally.

Release Notes
Fixed indefinite suspension when times were not set correctly.

@AenBleidd AenBleidd marked this pull request as draft February 15, 2022 13:35
wxString buffer;
wxString bufferPIF; //variable to store Processor Idle For string.
wxString bufferNRI; //variable to store No Recent Input string.
double startTime, endTime;

Choose a reason for hiding this comment

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

I recommend to move these two variables close to using - to line 816 (844) to avoid redundant creation if code will hit one of the return false;. And I suggest to initialize them by default

Suggested change
double startTime, endTime;
double startTime{0.0}, endTime{0.0};

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 understand. Because the "suspend when no mouse/keyboard input in last" variable is always enabled, I should just use the usual "buffer" variable for "bufferNRI". BufferPIF can be initialized in the conditional statement.

Your suggested change is not related to the code I modified, but I see your point, I can initialize these variables.

clientgui/DlgAdvPreferences.cpp Outdated Show resolved Hide resolved
@Vulpine05
Copy link
Contributor Author

There have been significant changes to the preferences settings with #4871. This PR will need significant changes. I'm closing this and will create an issue to track separate PRs. I'll write up the PR in the next couple of days.

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.

2 participants