Implement config file

[Gigas002]

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:

# base screenshot properties
[screenshot]
# display to take screenshot
display = "default"
# should contain cursor?
cursor = false

# properties, related to
# clipboard copy
[clipboard]
# should copy resulting screenshot
# to clipborad?
clipboard = true

# properties related to
# writing screenshot to filesystem
[filesystem]
# should write resulting screenshot
# to filesystem?
filesystem = true
# output directory
path = "~/images/screenshots"
# output filename format
format = "%Y-%m-%d_%H:%M:%S"
# output file encoding
encoding = "png"

For now, the PR contains the config.toml file example and config.rs module, 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 is None.

AFAIK, merging actually working config will require adding these dependencies:

• toml, serde -- for config serializing/deserializing
• dirs -- for a safe approach to get system paths (e.g. dirs::config_local_dir() to get $HOME/.config on linux)

And changing this behavior:

• util::get_default_file_name must take filename_format as parameter -> wait for #93
• add dir parameter for util::get_default_file_name or create a new fn?
• add/rework file option since we need to separate file and dir paths? Or we can keep the current args behavior and implement this separation only for config? (wait for #96?)
• make cli.cursor an Option<bool>, so we can use config.cursor as fallback value
• add filename_format: Option<String> cli option (would default to what's proposed in #93 if unwrap fails)
• add config: Option<PathBuf> cli option (config would default to Config::default() if unwrap fails)
• derive Serialize and Deserialize from serde for EncodingFormat enum, so we can ser/de config values directly into enum rather than string
• update the wayshot.rs file to properly configure cli/config values, giving priority to cli
• maybe more I'm missing here?

Since 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!

[Shinyzenith]

# base screenshot properties
[screenshot]
# display to take screenshot
display = "default"
# should contain cursor?
cursor = false

# properties, related to
# clipboard copy
[clipboard]
# should copy resulting screenshot
# to clipborad?
clipboard = true

Clipboard should probably be part of the screenshot directive.

# properties related to
# writing screenshot to filesystem
[filesystem]
# should write resulting screenshot
# to filesystem?
filesystem = true

The filesystem key feels very "vague" do you mean stdout vs writing to the fs? Can we use write-to-stdout as a key in that case?

# output directory
path = "~/images/screenshots"
# output filename format
format = "%Y-%m-%d_%H:%M:%S"

In a recent pr we started encoding our file path using chrono. We should probably make sure this is constrainted to chrono supported templates.

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 is None.

I agree with this take.

AFAIK, merging actually working config will require adding these dependencies:

• toml, serde -- for config serializing/deserializing

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.

• dirs -- for a safe approach to get system paths (e.g. dirs::config_local_dir() to get $HOME/.config on linux)

We should support the XDG_CONFIG_HOME env var before falling back to $HOME/.config.

[Shinyzenith]

• util::get_default_file_name must take filename_format as parameter -> wait for feat: added time_stamp flag #93 -> ✅ Sounds good.

• add dir parameter for util::get_default_file_name or create a new fn? -> ❎ Considering the suggested changes, it should be a new fn with a name such as construct_output_path.

• add/rework file option since we need to separate file and dir paths? Or we can keep the current args behavior and implement this separation only for config? (wait for accounting for directories in file path #96?) -> ❎ We should have a conditional flow for the config behavior. Lets wait to see some code and make further decisions if needed.

• make cli.cursor an Option<bool>, so we can use config.cursor as fallback value -> ✅ Sounds good.

• add filename_format: Option<String> cli option (would default to what's proposed in feat: added time_stamp flag #93 if unwrap fails) -> ✅ I suggested this in my previous comment too, yes.

• add config: Option<PathBuf> cli option (config would default to Config::default() if unwrap fails) -> ✅ Sounds good.

• derive Serialize and Deserialize from serde for EncodingFormat enum, so we can ser/de config values directly into enum rather than string -> ✅.

• update the wayshot.rs file to properly configure cli/config values, giving priority to cli -> ✅.

[Gigas002]

Clipboard should probably be part of the screenshot directive.

I agree. Leaving turn-on switches for various outputs in screenshot directive, sounds good. Maybe something like this would do?

[screenshot]
clipboard = true
stdout = true
fs = true
# other settings

[fs]
path = "~/images/screenshots"
# and so on

The filesystem key feels very "vague" do you mean stdout vs writing to the fs? Can we use write-to-stdout as a key in that case?

I think it'd be good to use same naming for all "turn-on" switches, so if we're using clipboard, it'd be good to use std and fs in that case. Or, if it's preferred to use write-to-std, then it should be write-to-clipboard and write-to-fs, IMO. What do you think?

In a recent pr we started encoding our file path using chrono. We should probably make sure this is constrainted to chrono supported templates.

The formatting will panic, if it encounters patterns, not supporteed by chrono, so we can use std::fmt::Write and expect on result:

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?

We should support the XDG_CONFIG_HOME env var before falling back to $HOME/.config.

Ah, my fault. dirs crate does just that, my bad I didn't mentioned it above.

[Shinyzenith]

I still don't understand what the filesystem configuration is for.

[Shinyzenith]

I think it'd be good to use same naming for all "turn-on" switches, so if we're using clipboard, it'd be good to use std and fs in that case. Or, if it's preferred to use write-to-std, then it should be write-to-clipboard and write-to-fs, IMO. What do you think?

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.

[Shinyzenith]

Also, feel free to introduce the rest of the config related changes in this PR but split across several commits.

[Gigas002]

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.

1. Rename list_outputs -> list_displays, output -> display and choose_output -> choose_display

Reason: while I do understand these parameters talk about display or wayland output, from user perspective I find it confusing, since we have OUTPUT that describes output file and some descriptions, that are bound to that OUTPUT and not output. Hence, we have OUTPUT argument and output option that doesn't have anything in common

Cons: breaking for some users

2. Allow writing for clipboard, fs and stdout regardless of each option

Reason: right now the flow selects either fs or stdout depending on cli.file value. I think we should give users more freedom to choose outputs, so we can support scenarios when user wants to get screenshot in clipboard, fs and stdout at the same time, or not save screenshot anywhere at all (don't see the actual usecase, but still)

Cons: partially breaks cli.file since requires separating stdout from it

[Gigas002]

I still don't understand what the filesystem configuration is for.

I'm referencing ability to write screenshot on drive as file here

[Shinyzenith]

Rename list_outputs -> list_displays, output -> display and choose_output -> choose_display

Not too keen on this change because a display in the wayland context pertains to the wayland connection. Outputs is fairly clear in my opinion.

[Shinyzenith]

@Decodetalkers What's your opinion on the above? Should we rename it?

[Decodetalkers]

@Decodetalkers What's your opinion on the above? Should we rename it?

I think output is output, display is display.. they are different , there are only one wl_display, but many wl_outputs, and I do not think name of display is good enough.. if use display, display what? It maybe more confusing

[Gigas002]

Fair enough. I just thought, that having both output describing wl_output and OUTPUT/output-format describing screenshot file at the same time a bit confusing

[Shinyzenith]

Can you rebase to remove the merge conflict? Thanks.

[Gigas002]

Implemented some features, discussed above. Please feel free to ask me to revert anything if you don't like. I also noticed, that $HOME/~ isn't treated properly, when read in config, so I had to add a lightweight shellexpand dependency to handle such cases. It depends only on dirs with default features, so shouldn't add much complexity.

[Decodetalkers]

+1

[Shinyzenith]

@Decodetalkers Would you mind dropping a review? I'll do so too.

[Shinyzenith]

Apologies for being late to this, I'll review this now 😄

[Gigas002]

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. Here are some tested examples and differencies in behavior with current freeze-feat-andreas version and overall final overview of PR.

cli.file:

• wayshot -- no args
  • creates wayshot-%Y_%m_%d-%H_%M_%S.png -> no changes
• wayshot - -- - character
  • current: prints screenshot to stdout
  • this pr: creates -.png
• wayshot file -- file without encoding
  • current: panics Error: The image format could not be determined
  • this pr: creates file.png
• wayshot file.jpg -- file with jpg encoding
  • creates file.jpg -> no changes
• wayshot $HOME/file -- full directory path with file without encoding
  • current: panics Error: The image format could not be determined
  • this pr: creates $HOME/file.png
• wayshot $HOME/file.jpg -- full directory path with file with jpg encoding
  • creates $HOME/file.jpg -> no changes
• wayshot $HOME/dir -- full directory path
  • creates $HOME/dir/wayshot-%Y_%m_%d-%H_%M_%S.png -> no changes
• wayshot dir -- relative directory path
  • creates dir/wayshot-%Y_%m_%d-%H_%M_%S.png -> no changes
• wayshot dir/file -- relative directory path with file without encoding
  • current: panics Error: The image format could not be determined
  • this pr: creates dir/file.png
• wayshot dir/file.jpg -- relative directory path with file with jpg encoding
  • creates dir/file.jpg -> no changes

I must also note, that if config.path value is not empty, all relative path calls (wayshot, wayshot file, wayshot dir, etc) uses config.path directory as base path. If full path is provided - it's respected as it should.

Also, write to dirve can be ignored obly if you set the config.fs to false. It will only take effect for wayshot runs without file arguments, e.g. wayshot --clpboard true --stdout true will copy screenshot to clipboard and write data in stdout, but won't write file on disk.

cli.clipboard:

• wayshot -- no args
  • doesn't copy the screenshot to clipboard -> no changes
• wayshot --clipboard:
  • current: copies the screenshot data to clipboard
  • this pr: panics error: a value is required for '--clipboard <CLIPBOARD>' but none was supplied
• wayshot --clipboard true:
  • current: panics Error: The image format could not be determined (because treats true as filename)
  • this pr: copies the screenshot data to clipboard, the screenshot file with default filename is created
• wayshot --clipboard false:
  • current: panics Error: The image format could not be determined (because treats false as filename)
  • this pr: doesn't copy the screenshot to clipboard, the screenshot file with default filename is created

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 clipboard: bool instead of Option<bool>. Then, we'll need to check if it's value is false and then fallback to config's value. However, if user had set the clipboard in config to true and now wants to ignore the clipboard option, the wayshot --clipboard false won't work as expected, instead it'll create false.png file and also copy the data to clipboard. It may be confusing, plus, the only actual way to force clipboard to be false is to tweak config, which isn't great IMO.

cli.log_level: No behavior changes. info by default.

cli.clurp: No behavior changes. None by default.

cli.cursor: Same problems and reasons, as with clipboard, due to changing from bool to Option<bool>. false by default.

cli.encoding: png by default. Encoding is still decided in order: screenshot file path arg encoding > encoding arg > config's value encoding > config's default

• wayshot -- no args
  • creates wayshot-%Y_%m_%d-%H_%M_%S.png -> no changes
• wayshot file -- file path without encoding
  • current: panics Error: The image format could not be determined
  • this pr: creates file.png
• wayshot file.jpg -- file with jpg encoding
  • creates file.jpg -> no changes
• wayshot file --encoding webp
  • current: panics Error: The image format could not be determined
  • this pr: creates file.webp
• wayshot file.jpg --encoding webp
  • creates file.jpg -> no changes

cli.list_outputs: No behavior changes and corresponding option in config.

cli.output: No behavior changes. None by default.

cli.choose_output: No behavior changes and corresponding option in config.

New cli stuff

cli.stdout: replacement for - value of cli.file in current version. It's now possible to write screenshot in stdout and on disk. false by default.

Example: wayshot --stdout true file.webp

cli.config: path for a custom config file. Defaults to $XDG_CONFIG_HOME/wayshot/config.toml > $HOME/wayshot/config.toml > default values (behavior described above)

Example: wayshot --config config.toml file.webp

cli.filename_format: allows specifying the format for screenshot file name with respect to chrono templates. wayshot-%Y_%m_%d-%H_%M_%S by default.

Example: wayshot --filename-format %Y-%m-%d_%H-%M-%S

Config-specific

config.fs.path: directory to save screenshots. Doesn't exist and by default decided in runtime as current directory.

[Shinyzenith]

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.

[Shinyzenith]

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? If possible CC: @AndreasBackx ( I hope your health is doing well andreas ).

[AndreasBackx]

@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.

[Gigas002]

@AndreasBackx Thank you for spending your time reviewing this PR! I haven't yet addressed all of the comments, but left some notes. I don't quite understand some parts of the proposed implementation, so I need to test it and may question you again, sorry... Hope we can find a good way to resolve these

[Shinyzenith]

@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.

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.

[Shinyzenith]

don't quite understand some parts of the proposed implementation, so I need to test it and may question you again, sorry... Hope we can find a good way to resolve these

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. Thank you again for the hardwork. This is a solid PR and as Andreas mentioned, we only want to make it user friendly and very clear when things go wrong as now we have two vectors of failure -- CLI & Config -- Their interactions need to be well thought out and tested.

❤️ Have a nice day!

[Gigas002]

Hey, @Shinyzenith! Hope your doing well. Thank you for your review once again! I think we should close this PR and re-create portions of it in a separate follow-up PRs, since this one indeed is already too complicated. I can't agree with your vision of erroring instead of inconsistent behavior, which is a bit frustrating. I would be glad if you'll tell me where to error exactly.

From Andreas review I can see three places where errors are requested:

1. Config's loading (load and get_default_path fns): IMO, we should change return type to Result<..> and shout a warning, but keep current non-panicking behavior. Config is intended to be a completely optional feature, so I think it would be incorrect to panic, if something's wrong with this feature
2. Fallback screenshot's file path to current directory or error: that's been discussed above, I think we can error here instead, though personally I still do not agree we should do that
3. Output filename's formatting: same as number 2

Would this be applicable?

In terms of PR separation, let's summarize what's need to be done: PR1 -> provide config struct, example file and config variable to cli. Also add config loading in wayshot.rs PR2 -> fix webp option (my bad) PR3 -> change cli behavior and add conditional flow for screenshot outputs

Please correct me if I'm wrong here.

The biggest "what can I do about it?" for me now is a Options (I still think it'd be a cleaner solution to have this) and fallbacking process for bool values in config, such as clipboard. Let me explain the problem just in case:

Long story short, with proposed PR we would have to call wayshot --clipboard true instead of wayshot --clipboard, or wayshot --clipboard false instead of wayshot. That is indeed VERY ugly and not UNIXy, but I don't know how else can we command to set precedence of cli options over config and let config values to act as fallback. For now I'm assuming, that if --clipboard does not presists -- it's None, meaning we should take value from config. Now let's assume we keep current behavior: clipboard can be eithier true or false (if not specified). That means, we should fallback to config's clipboard value, when cli's clipboard is false. But what if user wants to force clipboard to be false from CLI, but it's set in config to true and user experiences an inconsistent behavior. I don't know what's the best way to outcome this. Logically I'd prefer this PR's proposed syntax even though it's ugly, but more explicit on the other hand.

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...