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

client: the default value for the "suspend_if_no_recent_input" pref should to 0, not 60 #5251

Merged
merged 2 commits into from
May 25, 2023

Conversation

davidpanderson
Copy link
Contributor

Otherwise BOINC will stop computing after 60 minutes of idleness.

Is this a show-stopper? Quite possibly.

BTW, this should have been called "suspend_if_no_recent_input_min" (times are in seconds unless otherwise specified)

…hould to 0, not 60!

Otherwise BOINC will stop computing after 60 minutes of idleness.

BTW, this should be called "suspend_if_no_recent_input_min"
(times are in seconds unless otherwise specified)
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #5251 (7f4f598) into master (6c91e7f) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5251      +/-   ##
============================================
- Coverage     10.87%   10.86%   -0.01%     
  Complexity     1064     1064              
============================================
  Files           279      279              
  Lines         35977    35969       -8     
  Branches       8255     8275      +20     
============================================
- Hits           3911     3909       -2     
+ Misses        31674    31668       -6     
  Partials        392      392              
Impacted Files Coverage Δ
lib/prefs.cpp 0.00% <0.00%> (ø)

... and 8 files with indirect coverage changes

@RichardHaselgrove
Copy link
Contributor

That change was introduced as part of the mass revision of preferences in #4871 - specifically, by commit 7d24c02. Since that PR was the primary driver for the v7.22 release cycle, we should probably ask testers to be especially vigilant for other unforced errors in previously-working areas of the client.

@davidpanderson
Copy link
Contributor Author

I know where it was introduced.
I'm not sure why I made that change; there's no comment or commit note,
and it wasn't caught in the code review.

@RichardHaselgrove
Copy link
Contributor

Exactly. That's why we need to check for 'errors like that' during user testing.

@CharlieFenton
Copy link
Contributor

Isn't the default also for the checkbox to be off for that option? If so, then the value for the number of seconds doesn't matter, because it won't be used.

@CharlieFenton
Copy link
Contributor

Assuming that the checkbox is off by default, a value of 60 minutes seems like a reasonable default for that field since it will be ignored unless the user turns the checkbox on. It makes no sense to have the default be 0 when the user sets the checkbox on, as then setting the checkbox on would have no effect until the user also changed the numeric value.

@davidpanderson davidpanderson changed the title client: the default value for the "suspend_if_no_recent_input" pref should to 0, not 60! client: the default value for the "suspend_if_no_recent_input" pref should to 0, not 60 [WIP] May 23, 2023
@davidpanderson
Copy link
Contributor Author

If there is no global_prefs.xml or global_prefs_override.xml file,
and you run the client, it will suspend computing after 60 idle minutes.

That's without the Manager. I'm not sure that what the Manager is doing is correct either,
so I'm marking this WIP for now.

@AenBleidd AenBleidd marked this pull request as draft May 23, 2023 19:30
@AenBleidd
Copy link
Member

AenBleidd commented May 23, 2023

@davidpanderson, converted to 'Draft'. Please press 'Ready for review' button when it's ready for review :)

@CharlieFenton
Copy link
Contributor

@davidpanderson When a user attaches to their first project, doesn't it get their web preferences and then create the global_prefs.xml? If so, then the situation you describe can't happen once they get their first tasks from the project.

@davidpanderson
Copy link
Contributor Author

When you create an account on a project, it initially doesn't have any global prefs.

@davidpanderson davidpanderson changed the title client: the default value for the "suspend_if_no_recent_input" pref should to 0, not 60 [WIP] client: the default value for the "suspend_if_no_recent_input" pref should to 0, not 60 May 23, 2023
@davidpanderson davidpanderson marked this pull request as ready for review May 23, 2023 23:24
@davidpanderson
Copy link
Contributor Author

I see my mistake; I initialized it to 60 in defaults() rather than enabled_defaults().

@CharlieFenton
Copy link
Contributor

When you create an account on a project, it initially doesn't have any global prefs.

Doesn't it have the web prefs defaults? Also, I seem to remember that at least some of the projects take you to their web page to set up your prefs as soon as yo attach, but i could be wrong.

In any case, if we need to build 7.22.3, I won't be able to do that until after I return from my trip on June 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants