-
Notifications
You must be signed in to change notification settings - Fork 178
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
Config backend fixups #2006
Config backend fixups #2006
Conversation
be9361a
to
a9b84a3
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.
I tested this and it seems to work. I left two small suggestions, otherwise we can merge.
src/default-config-backend.cpp
Outdated
@@ -53,7 +53,7 @@ static int handle_config_updated(int fd, uint32_t mask, void *data) | |||
|
|||
const auto last_slash = config_file.find_last_of('/'); |
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 guess we can use the built-in std::filesystem functions here to get the base file name? https://en.cppreference.com/w/cpp/filesystem/path/filename
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.
This sounds reasonable.
{ | ||
inotify_rm_watch(fd, wd_cfg_file); | ||
wd_cfg_file = | ||
inotify_add_watch(fd, (config_dir + "/" + cfg_file_basename).c_str(), IN_CLOSE_WRITE); |
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 wonder, do we need to concatenate the two files here, isn't this basically config_file.c_str()
here?
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.
It would be, except in the case where the config file is specified as a relative path. I wanted to make sure the file path we're watching is complete and not rely on relative paths.
…nfig file When running wayfire with or by setting WAYFIRE_CONFIG_FILE, the config directory wasn't being set, causing instances pointing to a config file that did not exist when wayfire started to have no effect on config reload.
We should only need to add a watch when the config file is created in the config directory and once for the directory.
a9b84a3
to
9245f29
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.
Thanks :)
No description provided.