Uh oh!
There was an error while loading. Please reload this page.
Settings command - #436
Conversation
…pdate time and progress bar visual style. TODO: Localize strings, add settings readme and implement tests
| ### autoUpdateIntervalInMinutes | ||
| Positive integer that represents the interval in minutes on how often to update the WinGet source automatically. For no updates use 0. |
There was a problem hiding this comment.
Positive integer that represents the interval in minutes on how often to update the WinGet source automatically [](start = 0, length = 111)
Positive integer that represents the interval in minutes of how often to automatically check for updates to a WinGet source. The check for updates only happens when a source is used, and if no update is available the interval will be reset. #Closed
| std::unique_ptr<Command> command = std::make_unique<RootCommand>(); | ||
| // Load the settings file and warn if necessary. | ||
| auto warnings = Settings::UserSettings::Instance().GetWarnings(); |
There was a problem hiding this comment.
Settings::UserSettings::Instance() [](start = 24, length = 34)
I would suggest a standalone function that returns this, such as:
Settings::User()
Or similar. I really hate cluttered call sites. #Closed
There was a problem hiding this comment.
| ListVersions, // Used in Show command to list all available versions of an app | ||
| NoVT, // Disable VirtualTerminal outputs | ||
| PlainStyle, // Makes progress display as plain | ||
| RetroStyle, // Makes progress display as plain |
There was a problem hiding this comment.
plain [](start = 53, length = 5)
Your vendetta against plain is not complete. #Closed
| Accent, | ||
| Rainbow, | ||
| }; | ||
| using namespace Settings; |
There was a problem hiding this comment.
using namespace Settings; [](start = 4, length = 25)
Not in headers, unless it is a string literals namespace. #Closed
| std::string SettingsCommand::HelpLink() const | ||
| { | ||
| return "https://aka.ms/winget-settings"; |
There was a problem hiding this comment.
https://aka.ms/winget-settings [](start = 16, length = 30)
Did someone create this? #Closed
There was a problem hiding this comment.
I did, is not pointing to docs right now, but will update it once settings.md is on master
In reply to: 442413601 [](ancestors = 442413601)
| { | ||
| // Settings file was loaded correctly, create backup. | ||
| UserSettings::Instance().CreateBackup(); | ||
| } |
There was a problem hiding this comment.
This whole block feels like it should be in:
UserSettings::PrepareToShellExecuteFile()
or someting. I don't think the command should be the one responsible for understanding this process. #Closed
| std::string filePathUTF8Str = UserSettings::Instance().SettingsFilePath().u8string(); | ||
| // Kudos to the terminal team for this work around. |
There was a problem hiding this comment.
// Kudos to the terminal team for this work around. [](start = 8, length = 51)
Does one have to do something special to invoke the "choose which app to use" dialog? I expected that would just be automatic. #Closed
There was a problem hiding this comment.
It really depends on the version of Windows. Previous version will fail and that's where notepad will be open. Newer versions will show the dialog.
In reply to: 442416924 [](ancestors = 442416924)
| return result; | ||
| } | ||
| std::string ToString(const std::string_view key) |
There was a problem hiding this comment.
ToString [](start = 16, length = 8)
this is 2 chars less than std::string, but a bit more confusing as one doesn't know exactly what is going to happen without reading it.
Please remove and just use std::string() #Closed
| #include <optional> | ||
| #include <string> | ||
| namespace AppInstaller::Utility |
There was a problem hiding this comment.
Why does this file need to be public? Probably fine for it to be for potential future use, but it shouldn't be need right now. #Closed
| #include "AppInstallerStrings.h" | ||
| #include "winget/settings/Source.h" | ||
| #include "winget/settings/Visual.h" | ||
| #include <json.h> |
There was a problem hiding this comment.
A consumer of this header should not need to include JSON. That should all be implementation in the cpp file. #Closed
| { | ||
| static UserSettings& Instance() | ||
| { | ||
| // If we go multi-threaded secure this. |
There was a problem hiding this comment.
// If we go multi-threaded secure this. [](start = 12, length = 39)
It already is secured by what STL (the person) likes to call magic statics. Basically auto-thread safe init of function local statics. #Closed
| private: | ||
| UserSettings(); | ||
| ~UserSettings() {}; |
There was a problem hiding this comment.
{}; [](start = 23, length = 4)
= default; #Closed
| } | ||
| UserSettings(const UserSettings&) = delete; | ||
| UserSettings& operator=(const UserSettings&) = delete; |
There was a problem hiding this comment.
Also delete the moves, the only one should be Instance. #Closed
| // Representation of the parsed settings file. | ||
| struct UserSettings | ||
| { | ||
| static UserSettings& Instance() |
There was a problem hiding this comment.
UserSettings& [](start = 15, length = 13)
const&? We don't really want anyone changing anything, do we? #Closed
| UserSettings& operator=(const UserSettings&) = delete; | ||
| UserSettingsType GetType() const { return m_type; } | ||
| std::vector<std::string> GetWarnings() const { return m_warnings; } |
There was a problem hiding this comment.
std::vectorstd::string [](start = 8, length = 24)
const& #Closed
| UserSettings(); | ||
| ~UserSettings() {}; | ||
| std::optional<Json::Value> ParseFile(const std::string_view& fileName); |
There was a problem hiding this comment.
std::optionalJson::Value ParseFile(const std::string_view& fileName); [](start = 8, length = 71)
Move to unnamed namespace in cpp.
Use std::filesystem::path. #Closed
| UserSettingsType GetType() const { return m_type; } | ||
| std::vector<std::string> GetWarnings() const { return m_warnings; } | ||
| void Reload(); |
There was a problem hiding this comment.
Reload [](start = 13, length = 6)
When would this be used? #Closed
| void Reload(); | ||
| void CreateFileIfNeeded(); | ||
| void CreateBackup(); |
There was a problem hiding this comment.
These feel like they should be static. #Closed
There was a problem hiding this comment.
| void CreateBackup(); | ||
| std::filesystem::path SettingsFilePath(); | ||
| std::filesystem::path SettingsBackupFilePath(); |
| static constexpr std::string_view s_SettingBackupFileName = "settings.json.backup"sv; | ||
| static constexpr std::string_view s_SettingEmpty = | ||
| R"""({ |
There was a problem hiding this comment.
"" [](start = 10, length = 2)
" is an interesting choice for a raw string delimiter. #Closed
| #include "winget/settings/Source.h" | ||
| #include "winget/settings/Visual.h" | ||
| #include <iostream> |
There was a problem hiding this comment.
If not in the pch, add it there. #Closed
| // Settings | ||
| inline const Source& GetSource() const { return *m_source; } | ||
| inline const Visual& GetVisual() const { return *m_visual; } |
There was a problem hiding this comment.
I'm not a fan of having to create classes and functions for every setting. I would prefer a data driven model, thus requiring probably two files to be touched. Will look at the code for parsing and see if that still seems like a reasonable position. #Closed
There was a problem hiding this comment.
So you kind of already do this, just not to the level that would make it easier to add new things.
There are many ways that it could be done, but for type safety I think it would need to be similar to the Execution::Context.
enum for settings
templated struct with specializations per enum
templated get and parse functions
To add a new setting:
add new enum value
add new specialization
add parse call in main Parse method
Templated struct contains:
json "path"
json data type
internal data type
default value
parse function to turn json type into internal type
Parsed data gets stored in a map similar to Execution::Context. Get() looks there first, or returns struct::default if not present.
In reply to: 442441037 [](ancestors = 442441037)
| std::stringstream settingsContent; | ||
| settingsContent << stream->rdbuf(); | ||
| std::string settingsContentStr = settingsContent.str(); |
There was a problem hiding this comment.
There is a utility function to read the entire stream already (ReadEntireStream). I don't know if its implementation is better than this, but you should just use that function and replace the body if warranted. #Closed
| THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND), !std::filesystem::exists(SettingsFilePath())); | ||
| auto from = SettingsFilePath(); | ||
| auto to = GetPathTo(PathName::UserFileSettings) / s_SettingBackupFileName; |
There was a problem hiding this comment.
GetPathTo(PathName::UserFileSettings) / s_SettingBackupFileName [](start = 18, length = 63)
SettingsBackupFilePath? #Closed
| const char* const Value = " Value: "; | ||
| const char* const InvalidFieldValue = "Invalid field value."; | ||
| const char* const InvalidFieldFormat = "Invalid field format."; | ||
| const char* const LoadedBackupSettings = "Loaded settings from backup file."; |
There was a problem hiding this comment.
constexpr std::string_view #Resolved
| auto warnings = Settings::UserSettings::Instance().GetWarnings(); | ||
| for (const auto& warning : warnings) | ||
| { | ||
| context.Reporter.Warn() << warning << std::endl; |
There was a problem hiding this comment.
context.Reporter.Warn() << warning << std::endl; [](start = 12, length = 48)
Print only a warning message and only print the syntax error when winget settings #Resolved
| void SettingsCommand::ExecuteInternal(Execution::Context& context) const | ||
| { | ||
| // Show warnings only when the setting command is executed. |
There was a problem hiding this comment.
// Show warnings only when the setting command is executed. [](start = 8, length = 59)
You might want to have some text beforehand that will set up the warnings that are about to come out, rather than just showing them. #Closed
| // Some versions of windows will fail if no file extension association exists, other will pop up the dialog | ||
| // to make the user pick their default. | ||
| // Kudos to the terminal team for this work around. | ||
| HINSTANCE res = ShellExecuteA(nullptr, nullptr, filePathUTF8Str.c_str(), nullptr, nullptr, SW_SHOW); |
There was a problem hiding this comment.
ShellExecuteA [](start = 24, length = 13)
I don't think we have done anything to set our process to UTF-8 defaults, enabling UTF-8 *A apis. So this should probably be use the W function. Or potentially figure out how we do set ourself as UTF-8 aware (although I suspect that older OS versions would still need to use W). #Closed
There was a problem hiding this comment.
Changed. We are using ShellExecuteExA in ShellExecuteInstallerHandler.cpp, is that expected?
In reply to: 443074390 [](ancestors = 443074390)
There was a problem hiding this comment.
That has its own issue, and yes, it should be changed (but not in the PR).
In reply to: 443093477 [](ancestors = 443093477,443074390)
| <value>Open settings</value> | ||
| </data> | ||
| <data name="SettingsWarnings" xml:space="preserve"> | ||
| <value>Unexpected error while loading settings. Please verify your settings by running settings command </value> |
There was a problem hiding this comment.
by running settings command [](start = 80, length = 28)
by running the settings command. #Closed
| using value_t = VisualStyle; | ||
| static constexpr value_t DefaultValue = VisualStyle::Accent; | ||
| static constexpr std::string_view Path = ".visual.progressBar"; |
There was a problem hiding this comment.
static constexpr std::string_view Path = ".visual.progressBar" [](start = 12, length = 62)
Do you actually get constexpr from this constructor? It claims to, but I suppose I'm wary of that. That's why I always use the string literal tag (sv). But maybe it isn't necessary. #Closed
There was a problem hiding this comment.
| static constexpr value_t DefaultValue = VisualStyle::Accent; | ||
| static constexpr std::string_view Path = ".visual.progressBar"; | ||
| static std::optional<value_t> Validate(json_t value); |
There was a problem hiding this comment.
json_t [](start = 51, length = 6)
const json_t& as a general rule. #Resolved
| struct SettingMapping<Setting::AutoUpdateTimeInMinutes> | ||
| { | ||
| using json_t = uint32_t; | ||
| using value_t = uint32_t; |
There was a problem hiding this comment.
uint32_t [](start = 28, length = 8)
std::chrono::minutes? #Closed
| Validate< | ||
| Setting::AutoUpdateTimeInMinutes, | ||
| Setting::ProgressBarVisualStyle | ||
| >(settingsRoot, m_settings, m_warnings); |
There was a problem hiding this comment.
Should probably just make Validate take a single template param and have multiple lines. #Resolved
| User().PrepareToShellExecuteFile(); | ||
| auto filePathUTF16 = ConvertToUTF16(UserSettings::SettingsFilePath().u8string()); |
There was a problem hiding this comment.
ConvertToUTF16(UserSettings::SettingsFilePath().u8string()); [](start = 29, length = 60)
UserSettings::SettingsFilePath().c_str() [I think... the point is there is a method that just gives back the wide version] #Resolved
| { | ||
| // Use folding to call each setting validate function. | ||
| // Do not change this expression without understanding the implications to the bind order. | ||
| // See: https://en.cppreference.com/w/cpp/language/fold for more details. |
There was a problem hiding this comment.
These lines are not relevant; order should not matter here and it certainly doesn't have anything to do with binding (that is SQLite related, no C++). #Resolved
This changes introduce the settings command and two settings that can be configured.
When the setting command is executed the first time a settings file with empty values will be created. It will also open in the default text editor that is configured for json extensions. When there is no file type association for json files, older versions of Windows will fail to open. In this scenario notepad will be used as the text editor. Newer version of windows will show the "choose app" dialog where the user can select the text editor of their preference.
progressBar
This settings allows to modify the default color of the progress bar. Currently it supports three values, but it can be expanded to include more colors.
autoUpdateIntervalInMinutes
This is the value in minutes in which the source will auto update. The default value is 5 minutes. A user can set 0 to disable auto updates.
See this spec for more information.
Validated manually and created several unittest for new functionality.
Microsoft Reviewers: Open in CodeFlow