-
Notifications
You must be signed in to change notification settings - Fork 20
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
Allow user to configure update checking during setup #256
Conversation
1bed00b
to
54e0422
Compare
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.
Looks good, couple of minor comments but nothing super important.
shared/update_check_settings_file.h
Outdated
bool getAutoCheckEnabled(); | ||
bool setAutoCheckEnabled(bool enabled); |
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've lost count of the number of times I've used a bool for a flag and later needed an enum, so it might be worth:
enum class CheckFrequency {
ON_STARTUP,
NEVER
};
As it's extensible and self-documenting (you don't have to look at the variable name or use a qualifed method name to understand what the bool means)
I think the getter should probably be marked const
.
I have a personal preference for overloaded get/setters but I'm sure we have no consistency in that regard for this project so feel free to ignore:
CheckFrequency autoUpdate() const;
void autoUpdate(CheckFrequency status);
just because I think the resulting
if(settings.autoUpdate() == CheckFrequency::NEVER)
and settings.autoUpdate(CheckFrequency::ON_STARTUP)
look quite nice. Even better with a C++20 using enum CheckFrequency;
as it becomes if(settings.autoUpdate() == NEVER)
and settings.autoUpdate(ON_STARTUP);
.
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.
Whilst I agree, this has quite deep changes throughout the code (from UpdateCheckerSettingsFile up through UpdateChecker and then up in to extension menu code and setup application code). We would also have to support 2 different data types in the settings file since this is stored as a bool in previous versions. Feels like a lot of additional changes for something we might never use. I think best tackle it only if we need it (not doing it now won't make that any more difficult).
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.
Actually, the settings file wouldn't be a problem as we could just split it out to the appropriate attribute in the XML to maintain compatibility. This is how the config is stored;
<?xml version="1.0" encoding="UTF-8"?>
<UpdateCheck>
<LastReportedVersion VersionMajor="0" VersionMinor="0" VersionRevision="0"/>
<AutoCheck OnStartUp="1"/>
</UpdateCheck>
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.
Ah, no worries if it's a big change, I'd not followed it that far down.
if (!settingsFileExists()) { | ||
// No settings file - perhaps first run? | ||
if (saveSettings() && settingsFileExists()) { |
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 try to factor this sort of thing into an
bool ensureSettingsFileExists() {
bool success = settingsFileExists();
if (!success) {
success = saveSettings() && settingsFileExists();
}
return success;
as it makes the resulting if(ensureSettingsFileExists())
more clear at a glance that a negative return is probably something untoward (permission error or whatever) rather than having to trace through nested if
s.
If it fails what should the extension do?
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 could change line 10 in to if(ensureSettingsFileExists())
but we will still need the outer condition because we don't want to override autocheck state unless this is the first time we're creating this file, otherwise we just end up overwriting the users preference.
Failing silently is OK here. We're only interested in failures to save when the user tries to set. This is picked up by the bool return value of each of the setters (because we pass through the result of saveSettings()). this is picked up in the extension code, e.g
displayMessageBox(messageBoxTitles, "Error: Failed to save preferences.", MB_ICONEXCLAMATION); |
previous didn't load swell-types
Means we avoid pulling in the UpdateChecker dialog code into setup. We don't use any of the actual update checking code or user feedback in setup anyway, and so this change means we don't need to link swell at all
Its function is specific to the extension
ce36c79
to
b438b8a
Compare
Force push was rebase on main with conflicts resolved - prep for merge |
As title. This configuration option is in addition to the existing "Extensions" menu options in Reaper.
Implemented by making the UpdateChecker code shared between targets and splitting the UpdateChecker class into two separate classes - one to do the actual update checking and reporting back to the user (
UpdateChecker
), and one to handle the user settings file (UpdateCheckerSettingsFile
- which is the only bit the Setup target needs).UpdateCheckerSettingsFile can read and write to the relevant user setting file. Final screen of the setup process updated - UI shows a custom checkbox component to make it nice and big, and uses UpdateCheckerSettingsFile to store the user preference.