Use a struct to store config #647
Conversation
|
I really like this and thought about this as well, but the implementation with a macro is really clever. Just FYI from another issue from @aristocratos:
Since the next release of 1.3.0 will feature GPU support every other big change will probably be on hold. We might want to create a dev branch for things like this (??) and worry about merge conflicts later 😆 |
|
Also I have some ideas for this |
lvxnull
force-pushed
the
config
branch
7 times, most recently
from
2352d98
to
86f1953
Compare
|
@lvxnull that is some nifty macro magic :) The new config model uses a vector of ConfigEntry structs, which brings together all the current dispersed maps and lists of values and descriptions in to one entity. Will create a PR when I've adapted it back to btop, so it can be compared and either discarded or merged with ideas from this. One issue regarding the configuration is handling the config in the secondary thread, since it needs to be the same between "collection" and "drawing". This "small_config" then gets passed along to the secondary thread, the draw function calls gets moved in to the main thread and only after drawing is done, will any changed values from the "small_config" get synced to the main config. Some examples of how the code from new config model looks: enum class Types {
Bool,
Int,
String
};
struct ConfigEntry {
string name{};
string category{};
Types type{};
string cfg_description{};
vector<string> menu_description{};
std::variant<int64_t, bool, string> value{};
vector<string> valid_values{};
std::array<int64_t, 2> valid_range = { 0, 0 };
std::variant<int64_t, bool, string> real_value{};
bool has_override = false;
};
extern vector<ConfigEntry> configuration;
//* Return value for config key <name>
template <typename T>
T get(const string& name) requires (std::is_same_v<T, int64_t> or std::is_same_v<T, bool> or std::is_same_v<T, string>) {
auto e = std::ranges::find_if(configuration, [&name](const ConfigEntry& entry) { return entry.name == name; });
if (e == configuration.end()) {
throw std::runtime_error("Config::get() -> key: [" + name + "] not found!");
}
try {
return std::get<T>(e->value);
}
catch(const std::bad_variant_access&) {
throw std::runtime_error("Config::get() -> requesting wrong type from key: [" + name + "]!");
}
}
//* Return value as string for key <name>
inline string getAsString(const string& name) {
auto e = std::ranges::find_if(configuration, [&name](const ConfigEntry& entry) { return entry.name == name; });
if (e == configuration.end()) {
throw std::runtime_error("Config::getAsString() -> key: [" + name + "] not found!");
}
if (e->type == Types::String)
return std::get<string>(e->value);
else if (e->type == Types::Bool)
return std::get<bool>(e->value) ? "True" : "False";
else if (e->type == Types::Int)
return std::to_string(std::get<int64_t>(e->value));
return "";
}
inline ConfigEntry& getRawEntry(const string& name) {
auto e = std::ranges::find_if(configuration, [&name](const ConfigEntry& entry) { return entry.name == name; });
if (e == configuration.end()) {
throw std::runtime_error("Config::getRawEntry() -> key: [" + name + "] not found!");
}
return *e;
}
//* Set config key <name> to <value>
template <typename T>
inline void set(const string& name, const T& value) requires (std::is_convertible_v<T, int64_t> or std::is_same_v<T, bool> or std::is_convertible_v<T, string>) {
auto e = std::ranges::find_if(configuration, [&name](const ConfigEntry& entry) { return entry.name == name; });
if (e == configuration.end()) {
throw std::runtime_error("Config::set() -> key: [" + name + "] not found!");
}
write_new = true;
e->has_override = false;
e->value = value;
}
//* Set override for config key <name> to <value>
template <typename T>
inline void setOverride(const string& name, const T& value) requires (std::is_convertible_v<T, int64_t> or std::is_same_v<T, bool> or std::is_convertible_v<T, string>) {
auto e = std::ranges::find_if(configuration, [&name](const ConfigEntry& entry) { return entry.name == name; });
if (e == configuration.end()) {
throw std::runtime_error("Config::set() -> key: [" + name + "] not found!");
}
e->real_value = e->value;
e->value = value;
e->has_override = true;
}
//* Flip bool value for config key <name>
inline void flip(const string& name) {
auto e = std::ranges::find_if(configuration, [&name](const ConfigEntry& entry) { return entry.name == name; });
if (e == configuration.end()) {
throw std::runtime_error("Config::flip() -> key: [" + name + "] not found!");
}
if (e->type != Types::Bool) {
throw std::runtime_error("Config::flip() -> key: [" + name + "] is not a bool!");
}
write_new = true;
e->value = not std::get<bool>(e->value);
}Example of ConfigEntry: vector<ConfigEntry> configuration = {
//* General
{
.name = "server_address",
.category = "general",
.type = Types::String,
.cfg_description = "#* xxx http server address including port, defaults to: \"http://localhost:2020\". Only accepts \"http://[hostname][:port]\".\n"
"#* Will use environment variable \"xxx_SERVER_ADDRESS\" if set.",
.menu_description = {"Server address",
"xxx http server address including",
"port.",
"",
"Defaults to: \"http://localhost:2020\"",
"",
"Only accepts \"http://[hostname][:port]\".",
"",
"Will use environment variable",
"\"xxx_SERVER_ADDRESS\" if set."},
.value = "http://localhost:2020"
},
{
.name = "update_ms",
.category = "general",
.type = Types::Int,
.cfg_description = "#* Update time in milliseconds, recommended 2000 ms or above for better sample times for graphs.",
.menu_description = {"Update time",
"Update time in milliseconds.",
"",
"Recommended 2000 ms or above for better",
"sample times for graphs.",
"",
"Min value: 100 ms",
"Max value: 86400000 ms = 24 hours."},
.value = 2000,
.valid_range = { 100, 86400000 }
},
{
.name = "theme_background",
.category = "general",
.type = Types::Bool,
.cfg_description = "#* If the theme set background should be shown, set to False if you want terminal background transparency.",
.menu_description = { "Theme background",
"Set theme background.",
"",
"Set to False if you want terminal",
"background transparency." },
.value = true
},
{
.name = "log_level",
.category = "general",
.type = Types::String,
.cfg_description = "#* Set loglevel for \"~/.config/xxx/xxx.log\" levels are: \"ERROR\" \"WARNING\" \"INFO\" \"DEBUG\".\n"
"#* The level set includes all lower levels, i.e. \"DEBUG\" will show all logging info.",
.menu_description = {"Log level",
"Set loglevel for \"xxx.log\".",
"",
"Levels are: \"ERROR\" \"WARNING\" \"INFO\" \"DEBUG\".",
"",
"The level set includes all lower levels,",
"i.e. \"DEBUG\" will show all logging info.",
"",
"Log file is usually located in:",
"\"~/.config/xxx/xxx.log\""},
.value = "INFO",
.valid_values = Logger::log_levels
}
}And then in use something like this: const auto single_graph = Config::get<bool>("overview_single_graph");
const auto graph_up_field = Config::get<string>("overview_graph_upper");
Config::set("update_ms", new_val);
Config::setOverride("tty_mode", true);
auto old_val = Config::get<int64_t>("update_ms");
if (old_val + 100l > Config::getRawEntry("update_ms").valid_range.back())
return;
int64_t new_val = old_val + (old_val >= 2000 and last_press >= time_ms() - 200 and rng::all_of(Input::history, [](const auto& str){ return str == "+"; }) ? 1000 : 100);
Config::set("update_ms", new_val); |
|
I don't like the idea of storing range and possible values alongside data. These are only useful in certain places and needlessly bloat the structure. Instead, I would use a generic function that takes some value unique to a field(e.g name, with my implementation it could be the offset) and the value itself and then did validation. template<typename T>
inline bool validate(const size_t offset, const T& value) {
return true;
}
template<>
inline bool validate<int>(const size_t offset, const int& value) {
switch(offset) {
case offsetof(ConfigSet, update_ms):
return value >= 100 && value <= 86400000;
}
return true;
}
template<>
inline bool validate<string>(const size_t offset, const string& value) {
switch(offset) {
//...
}
return true;
}In it's current form, your structure requires 256 bytes of memory. And that's for each field, so 256 * 77 = 19712 bytes of memory, just for these structures for every option. It might be even more if the vector pre-allocates more of these structures than necessary. And for each read or write, it must do 77 string comparisons in the worst case. I also think that the menu should be defined separately from the config entries. The config and the menu present their contents in different order, if you don't want to reshuffle either then you would have to hard code the order of one of them. I'd rather change the current implementation of menu to work based on offsets rather than string names. |
I would argue it's the opposite of bloat since it's all values related to specific config entry. If there is a worry about keeping the struct small, couldn't this just be fixed by having the bigger and less frequently accessed values be either reference_wrappers or actual pointers to data stored somewhere else?
It could be switched to a std::array since the size won't be changing.
As I mentioned: "I'm sure your code will probably more efficient [...]" :)
Where the menu is defined isn't really important, that just how it ended up in the other project (which also has alot less options than btop). The ordering of the config file doesn't really matter, but it would probably not be a negative if it was ordered and categorized like the menu is. The main point of the example code above is ease of use. Just add a new config entry and you're done. And less likely to get unintended bugs if you see the struct have members to define constraints which you might not have thought about otherwise. |
|
I removed string based access to options from the codebase except the config loader. Now that I think of it, maybe themes could be rewritten like this as well? |
|
@aristocratos should I open another pull request for the theme rewrite or use this one? |
Depends, if any code from this PR will be reused it's probably better to have both in the same PR, otherwise create a new one. |
Unlike the previous hashmap based approach, this ensures there are no hash collisions(by not utilizing any sort of hashing function) on read and write. Dynamic access to fields will still utilize hashmaps to look up the offset and type of the field. By using a struct, your IDE can now suggest config fields, and access to a non-existent field will result in a compile time error.