Implement config file #104
Conversation
Clipboard should probably be part of the screenshot directive.
The filesystem key feels very "vague" do you mean stdout vs writing to the fs?
In a recent pr we started encoding our file path using chrono. We should probably make sure this is constrainted to chrono supported templates.
I agree with this take.
Yikes serde is a big dependency and slow during compile times but I'm willing to make the sacrifice as this is a good feature. I initially felt flaky about it but now after seeing the ideation I think it will be a good addition.
We should support the |
|
I agree. Leaving turn-on switches for various outputs in [screenshot]
clipboard = true
stdout = true
fs = true
# other settings
[fs]
path = "~/images/screenshots"
# and so on
I think it'd be good to use same naming for all "turn-on" switches, so if we're using
The formatting will panic, if it encounters patterns, not supporteed by chrono, so we can use pub fn get_full_file_name(
dir: &PathBuf,
filename_format: &str,
encoding: EncodingFormat,
) -> PathBuf {
let format = Local::now().format(filename_format);
let mut file_name = String::new();
write!(file_name, "{format}.{encoding}").expect("Specified filename format pattern is unsupported");
dir.join(file_name)
}Would this be enough?
Ah, my fault. |
|
I still don't understand what the |
Looks good for the time being. The chrono template failure matches should be handled gracefully instead of just panicking but yes it's mostly fine imo. |
|
Also, feel free to introduce the rest of the config related changes in this PR but split across several commits. |
|
I've implemented some of proposed features (will push a bit later today or tomorrow). However, I've encountered a few issues that I'd like to discuss here, before continue further. Here are my proposals.
Reason: while I do understand these parameters talk about display or wayland output, from user perspective I find it confusing, since we have Cons: breaking for some users
Reason: right now the flow selects either Cons: partially breaks |
I'm referencing ability to write screenshot on drive as file here |
|
Implemented some features, discussed above. Please feel free to ask me to revert anything if you don't like. |
|
+1 |
|
@Decodetalkers Would you mind dropping a review? I'll do so too. |
|
Apologies for being late to this, I'll review this now 😄 |
wayshot/config.toml
Outdated
| # should copy screenshot to clipborad? | ||
| clipboard = true | ||
| # should write screenshot as file in filesystem? | ||
| fs = true |
There was a problem hiding this comment.
I think fs is not very user friendly. How about "save-to-disk"?
There was a problem hiding this comment.
I see, you are indeed correct. How about a file name then? The thing is, I want to keep config values naming as close as possible to cli arguments, so users will be able to know how to tweak config right away, if they were working with wayshot's cli args before.
There was a problem hiding this comment.
That's a very good point, let's go ahead with that.
There was a problem hiding this comment.
Apologies, somehow missed this in actual code. Will push a fix later today.
UPD. fixed
| /// Defaults to: | ||
| /// 1. `$XDG_CONFIG_HOME/wayshot/config.toml` | ||
| /// 2. `$HOME/wayshot/config.toml` -- if `$XDG_CONFIG_HOME` variable doesn't exist | ||
| /// 3. `None` -- if the config isn't found, the `Config::default()` will be used |
There was a problem hiding this comment.
I'm presuming Config::default is the current functionality inorder to not make it a breaking change?
Simply put, with the patch merged, wayshot will act just as it did but will now respect an optional configuration?
There was a problem hiding this comment.
With exception of some breaking changes (big comment below), yes.
As for precedence, it can be described as such: cli arguments > cli.config config's values > $XDG_CONFIG_HOME/wayshot/config.toml values (doesn't exist by default) > $HOME/wayshot/config.toml values (doesn't exist by default) > Config::default() values (hard-coded to have same behavior as cli default arguments). The repo's config.toml example file will also have the same default values, as current cli ones.
wayshot/src/config.rs
Outdated
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| pub struct Config { | ||
| pub screenshot: Option<Screenshot>, | ||
| pub fs: Option<Fs>, |
There was a problem hiding this comment.
Again not a fan of the fs terminology as it seems vague but I think we can get past this.
There was a problem hiding this comment.
I think we can name it file as with bool switch, if it's OK to you
wayshot/src/wayshot.rs
Outdated
| // config | ||
| let config = Config::load(&config_path).unwrap_or_default(); | ||
| let log = config.log.unwrap_or_default(); | ||
| let screenshot = config.screenshot.unwrap_or_default(); |
There was a problem hiding this comment.
"screenshot" sounds very vague when reading a screenshot apps codebase. Maybe something better can be used.
I understand if these nits are disturbing to deal with but I hope you see where I'm coming from, we keep the code standards high and unambiguous to help our contributors out ( I hope it was a nice experience for you ❤️ )
There was a problem hiding this comment.
Good point. I was thinking about that section of config as a basic properties. Maybe basic or base, something like that would do?
I understand if these nits are disturbing to deal with but I hope you see where I'm coming from, we keep the code standards high and unambiguous to help our contributors out ( I hope it was a nice experience for you ❤️ )
Don't have any objections, just want to implement a clean and useful feature for a great software 😊
wayshot/src/config.rs
Outdated
|
|
||
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| pub struct Log { | ||
| pub level: Option<String>, |
There was a problem hiding this comment.
A separate struct just for an optional string feels unecessary.
There was a problem hiding this comment.
Included it into screenshot (ugh, screenshot is indeed confusing, I mean base) config section as log_level.
There was a problem hiding this comment.
Haha I'm still a little lost with its need but I guess it'll be cleared up after reviewing the new changes.
|
At first I wanted to write this as a comment to review, but as this message became too big, so I thought it might be more useful if I just post it as a comment to PR. Sorry for that.
I must also note, that if Also, write to dirve can be ignored obly if you set the
I'm not sure if it's a desired behavior. Of course, I'd like not to break existing functionality for current users, but I don't see other option but to force this parameter this way. Let's say, we keep
New cli stuff
Example:
Example:
Example: Config-specific
|
|
Hi, thank you so much for the comprehensive overview. I am unable to go through it all right now but here's something that caught my eye: wayshot $HOME/file This was handled in a recent pr right? EDIT: nevermind, you are correct. |
|
From a preliminary view your decisions seem to be good and I agree with them. I think we can go ahead with this. Is the patch ready to be merged? @Gigas002 Since we have already broken user compat in freeze-feat, I think breaking it a little bit further to improve the UX is worth the cost. @Decodetalkers May this PR get your blessing? |
There was a problem hiding this comment.
This is quite a big PR. I think too many items are trying to be tackled at once in this PR:
- Changing
--stdoutand--filebehaviour - Allowing writing to multiple outputs at once
- Adding a config file option
I'd say, let us focus first on adding a config file option and then we could possibly think about the others.
Sidenote: this is a great example of why I think stacked diffs are awesome. It's right now impossible on GitHub to work on those 3 features at once (or it's hard) and thus we are kind of blocking the great work being done here. Thank you for the contributions!
| pub struct Base { | ||
| pub output: Option<String>, | ||
| pub cursor: Option<bool>, | ||
| pub clipboard: Option<bool>, | ||
| pub file: Option<bool>, | ||
| pub stdout: Option<bool>, | ||
| pub log_level: Option<String>, | ||
| } | ||
|
|
||
| impl Default for Base { | ||
| fn default() -> Self { | ||
| Base { | ||
| output: None, | ||
| cursor: Some(false), | ||
| clipboard: Some(false), | ||
| file: Some(true), | ||
| stdout: Some(false), | ||
| log_level: Some("info".to_string()), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| pub struct Base { | |
| pub output: Option<String>, | |
| pub cursor: Option<bool>, | |
| pub clipboard: Option<bool>, | |
| pub file: Option<bool>, | |
| pub stdout: Option<bool>, | |
| pub log_level: Option<String>, | |
| } | |
| impl Default for Base { | |
| fn default() -> Self { | |
| Base { | |
| output: None, | |
| cursor: Some(false), | |
| clipboard: Some(false), | |
| file: Some(true), | |
| stdout: Some(false), | |
| log_level: Some("info".to_string()), | |
| } | |
| } | |
| } | |
| pub struct Base { | |
| #[serde(default = "None")] | |
| pub output: Option<String>, | |
| #[serde(default = "false")] | |
| pub cursor: bool, | |
| #[serde(default = "false")] | |
| pub clipboard: bool, | |
| #[serde(default = "true")] | |
| pub file: bool, | |
| #[serde(default = "false")] | |
| pub stdout: bool, | |
| #[serde(default = "info")] | |
| pub log_level: String, | |
| } |
It seems unnecessary to me to have a struct that is entirely optional and have a Default implementation for it. I think you should do away with the Default implementation and leverage serde's default attributes: https://serde.rs/attr-default.html
The suggest example might not be entirely correct, but you get the gist.
There was a problem hiding this comment.
I've tested this out a bit, and I can't say I like how it looks. This attribute takes fn as value. For example, implementation for this struct would look like this:
#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct Base {
#[serde(default = "default_string_none")]
pub output: Option<String>,
#[serde(default = "default")]
pub cursor: bool,
#[serde(default = "default")]
pub clipboard: bool,
#[serde(default = "default_true")]
pub file: bool,
#[serde(default = "default")]
pub stdout: bool,
#[serde(default = "default_log_level")]
pub log_level: String,
}
fn default_true() -> bool {
true
}
fn default_log_level() -> String {
"info".to_string()
}
fn default_string_none() -> Option<String> {
None
}Which is pretty ugly IMO, less readable then implementing Default by our own.
Plus, doing so, we'll lose the ability to use Base::default(), since the provided values will work only if we'll be able to find the config file on disk.
Also, I don't understand how can we benefit from removing Options here. That way, if config lacks some values or some values are misspelled - the deserialization will fail and panic or call the default on unwrap, even if all the other options are correct. I don't think it's desired behavior, actually. I actually like alacritty's philosophy on this, saying "default config file is an empty config file", meaning every option is optional.
| pub struct File { | ||
| pub path: Option<PathBuf>, | ||
| pub format: Option<String>, | ||
| pub encoding: Option<EncodingFormat>, | ||
| } | ||
|
|
||
| impl Default for File { | ||
| fn default() -> Self { | ||
| File { | ||
| path: Some(env::current_dir().unwrap_or_default()), | ||
| format: Some("wayshot-%Y_%m_%d-%H_%M_%S".to_string()), | ||
| encoding: Some(EncodingFormat::Png), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same comment as the other struct.
| pub struct Config { | ||
| pub base: Option<Base>, | ||
| pub file: Option<File>, | ||
| } | ||
|
|
||
| impl Default for Config { | ||
| fn default() -> Self { | ||
| Config { | ||
| base: Some(Base::default()), | ||
| file: Some(File::default()), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same as below, remove the options.
| pub fn get_default_path() -> PathBuf { | ||
| dirs::config_local_dir() | ||
| .map(|path| path.join("wayshot").join("config.toml")) | ||
| .unwrap_or_default() |
There was a problem hiding this comment.
nit: I'd make this return a Result<PathBuf>. Convert the None to an Error. It will basically never happen, though seems slightly better practice to error here.
| if let Ok(checked_path) = checked_path { | ||
| PathBuf::from(checked_path.into_owned()) | ||
| } else { | ||
| env::current_dir().unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
Let's not fallback to the current yidrectory after shell expansion failed. The user should know about the failure and we should not magically use a different directory.
There was a problem hiding this comment.
Maybe I’m on the losing side here, but I prefer not to error where it's possible not to error no matter what. Warnings are good, but the error can cause a situation, when screenshot data is not created and it's hard to reproduce conditions for that screenshot. This situation could easily been avoided instead of throwing error.
IMO having inconsistent behavior is better, than losing data.
There was a problem hiding this comment.
Heavily disagree here. I want to know when I'm inputting incorrect data. Like I want to know when a Rust function errors and thus want it to return Result and not the Default. I don't think a warning suffices as sometimes you might even pipe the data and you might not even see stderr because it's running in the background.
There was a problem hiding this comment.
I'm not sure what would be optimal here, however, I agree with Andreas and there should definitely be some "indication" that we are choosing a separate path
There was a problem hiding this comment.
I don't think a warning suffices as sometimes you might even pipe the data and you might not even see stderr because it's running in the background.
This does make sense however which makes me slightly incline towards errors.
| let mut file_name = String::new(); | ||
| let write_result = write!(file_name, "{format}.{encoding}"); | ||
|
|
||
| if write_result.is_ok() { | ||
| file_name.into() | ||
| } else { | ||
| tracing::warn!( | ||
| "Couldn't write proposed filename_format: '{filename_format}', using default value." | ||
| ); | ||
|
|
||
| let format = now.format("wayshot-%Y_%m_%d-%H_%M_%S"); | ||
|
|
||
| format!("{format}.{encoding}").into() | ||
| } |
There was a problem hiding this comment.
Chrono's formatting API is so ridiculous. It's so unclear why this weird approach needs to be taken. I really hope they change their API: chronotope/chrono#1127
Regardless, again I don't think we should hide an error. We should not default to our default if there's an error.
| let mut file_name = String::new(); | |
| let write_result = write!(file_name, "{format}.{encoding}"); | |
| if write_result.is_ok() { | |
| file_name.into() | |
| } else { | |
| tracing::warn!( | |
| "Couldn't write proposed filename_format: '{filename_format}', using default value." | |
| ); | |
| let format = now.format("wayshot-%Y_%m_%d-%H_%M_%S"); | |
| format!("{format}.{encoding}").into() | |
| } | |
| let mut file_name = String::new(); | |
| write!(file_name, "{format}.{encoding}")?; | |
| file_name.into() |
There was a problem hiding this comment.
I wonder what @Shinyzenith thinks about this. It was proposed to not error here, and I do agree with that opinion.
There was a problem hiding this comment.
In hindsight I think erroring out is good because then the user gets to know about their mistakes in a meaningful manner ( thanks to the previous pipe example from Andreas)
| pub fn load(path: &PathBuf) -> Option<Config> { | ||
| let mut config_file = std::fs::File::open(path).ok()?; | ||
| let mut config_str = String::new(); | ||
| config_file.read_to_string(&mut config_str).ok()?; | ||
|
|
||
| toml::from_str(&config_str).ok()? | ||
| } |
There was a problem hiding this comment.
This function should return a default config if no config was defined as the default config should act as a centralised "default" for wayshot.
| pub fn load(path: &PathBuf) -> Option<Config> { | |
| let mut config_file = std::fs::File::open(path).ok()?; | |
| let mut config_str = String::new(); | |
| config_file.read_to_string(&mut config_str).ok()?; | |
| toml::from_str(&config_str).ok()? | |
| } | |
| pub fn load(path: &PathBuf) -> Result<Config> { | |
| let Some(config_file) = std::fs::File::open(path) else { | |
| return Config::default(); | |
| }; | |
| let config_contents = fs::read_to_string(config_file)?; | |
| toml::from_str(&config_str) | |
| } |
There was a problem hiding this comment.
Good point, though there are several places the program could fail with error in this function. I think we need to change the return type to Result, but still warn about the error and default on the outside, it'd just look cleaner:
pub fn load(path: &PathBuf) -> Result<Config, Box<dyn Error>> {
let mut config_file = std::fs::File::open(path)?;
let mut config_str = String::new();
config_file.read_to_string(&mut config_str)?;
toml::from_str(&config_str).map_err(|err| err.into())
}
let config = Config::load(&config_path).unwrap_or_else(|err| {
tracing::warn!("Couldn't load config. Fallback using default values. Original error: '{err}'");
Config::default()
});| use clap::builder::TypedValueParser; | ||
| use clap::Parser; | ||
| use std::path::PathBuf; | ||
| use tracing::Level; | ||
|
|
||
| #[derive(Parser)] | ||
| #[command(version, about)] | ||
| pub struct Cli { |
There was a problem hiding this comment.
I feel like we should have a look at this crate: https://crates.io/crates/clap-serde-derive
It merges the serde and CLI config and would mean that we don't have options here all over the place.
There was a problem hiding this comment.
Looks like a useful crate and I'd be happy to increase the dependency graph for it.
| /// Custom screenshot file path can be of the following types: | ||
| /// 1. Directory (Default naming scheme is used for the image screenshot file). | ||
| /// 2. Path (Encoding is automatically inferred from the extension). | ||
| /// 3. `-` (Indicates writing to terminal [stdout]). | ||
| #[arg(value_name = "OUTPUT", verbatim_doc_comment)] | ||
| /// 3. None (the screenshot file with default filename_format will be created in current directory) | ||
| #[arg(value_name = "FILE", verbatim_doc_comment)] | ||
| pub file: Option<PathBuf>, | ||
|
|
||
| /// Copy image to clipboard. Can be used simultaneously with [OUTPUT] or stdout. | ||
| /// Write screenshot to terminal/stdout. | ||
| /// Defaults to config value (`false`) | ||
| #[arg(long, verbatim_doc_comment)] | ||
| pub stdout: Option<bool>, |
There was a problem hiding this comment.
See my comment in #100. Additionally, let's not change the CLI in this PR because this is already a large change. If you want to change the CLI, please do this in a followup PR and allow for a separate discussion and keep this reviewable.
So I would prefer to see this reverted and have this discussed in a future PR.
There was a problem hiding this comment.
This makes sense, lets do CLI changes separately.
| # should copy screenshot to clipborad? | ||
| clipboard = false | ||
| # should write screenshot as file in filesystem? | ||
| file = true | ||
| # should write screenshot in stdout? | ||
| stdout = false |
There was a problem hiding this comment.
I disagree that we should be writing to all of these at once. There should only be one output. For example the screenshot tools on MacOS and Windows (and other Linux apps) also only allow saving to 1 location. I myself would be confused if it saved to a file and to my clipboard.
As my comment on #100 and a comment on this PR from me elsewhere, I would want to avoid overloading this PR with extra features. Let's first start with adding a config, then we can change behaviour.
There was a problem hiding this comment.
I cannot agree with you on this problem. All the software that I have used (flameshot, sharex, watershot) does this. Hence, I wouldn't even use the software that doesn't allow me do that. IMO it's an essential thing to do, especially considering how easy it's done and that's no additional burden for this feature is needed.
I always take screenshot in both clipboard and a file outputs. I share a screenshot, or a part of it in some chats and can review the backup file in any point of time I want on my drive.
The stdout is no different. If user wants to have screenshot data in both stdout and a file backup - why wouldn't let user do that? Remember, that it's a switch, not a forced feature.
The reasons to have multiple backups for screenshots are enough. Maybe the clipboard is broken and the screenshot is not saved anywhere? Maybe it's stdout being broken? The screenshot data itself could be critical to save at the moment of time (e.g. some screenshot of some stream that's not recorded anywhere, or a moment in online game that you won't be able to reproduce ever again?), and just throwing an error would be very annoying and absolutely not helpful from user perspective.
There was a problem hiding this comment.
Okay, if this is configurable, not the default, and very clear on the CLI, I agree this would be beneficial then. Could we actually in this PR however focus on making it understandable to the user that the CLI and config can be used nicely together? It would
mean temporarily having the same options from Clap in the config. Though we can properly think about how to handle multiple outputs and changing the CLI and config then? I primarily want to make sure wayshot is as or more user friendly than it is today.
|
@Shinyzenith I cannot entirely follow the the conversation here as I don't have much time though I've given the diff a review. I think the work here is great, though we're doing too much in 1 PR. See my review comment. |
|
@AndreasBackx Thank you for spending your time reviewing this PR! I haven't yet addressed all of the comments, but left some notes. |
Thank you so much for the review, I understand the PR is big and I also miss stacks right now. Will go through all your comments and address them with Gigas! Thank you once again. |
If you're free over email / discord, feel free to text me to discuss further or I can continue this PR if you're short on time. ❤️ Have a nice day! |
|
Hey, @Shinyzenith! Hope your doing well. Thank you for your review once again! From Andreas review I can see three places where errors are requested:
Would this be applicable? In terms of PR separation, let's summarize what's need to be done: Please correct me if I'm wrong here. The biggest "what can I do about it?" for me now is a Long story short, with proposed PR we would have to call This problem also makes it a bit hard for me to separate config PR from cli-behavior-changes PR -- I don't know how to properly load config values without these changes... |
This draft PR introduces config, which has been discussed in #97. I personally prefer using configs whenever possible because it makes programs configurations easy and usually easily redistributable.
Please, share your opinions on what to change and feel free to close the PR if it's not appropriate for a project or my implementation sucks.
The proposed config is the following:
For now, the PR contains the
config.tomlfile example andconfig.rsmodule, not yet included for build, as I thought the specs must be approved here first.In general, I think CLI args should take precedence over config values, which would serve as fallback and define the default values (via
unwrap_or/unwrap_or_default) in cases, when corresponding argument isNone.AFAIK, merging actually working config will require adding these dependencies:
toml,serde-- for config serializing/deserializingdirs-- for a safe approach to get system paths (e.g.dirs::config_local_dir()to get$HOME/.configon linux)And changing this behavior:
util::get_default_file_namemust takefilename_formatas parameter -> wait for feat: added time_stamp flag #93dirparameter forutil::get_default_file_nameor create a new fn?fileoption since we need to separatefileanddirpaths? Or we can keep the current args behavior and implement this separation only for config? (wait for accounting for directories in file path #96?)cli.cursoranOption<bool>, so we can useconfig.cursoras fallback valuefilename_format: Option<String>cli option (would default to what's proposed in feat: added time_stamp flag #93 ifunwrapfails)config: Option<PathBuf>cli option (config would default toConfig::default()ifunwrapfails)SerializeandDeserializefromserdeforEncodingFormatenum, so we can ser/de config values directly into enum rather than stringwayshot.rsfile to properly configurecli/configvalues, giving priority tocliSince implementing all of these features is out of scope for one PR and to keep at as small as possible, I've implemented these in my temporary branch (diff), so you can take a look of how these changes can actually be used.
Please, share your thoughts on this!