Open Gigas002 opened 4 months ago
# 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 isNone
.
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
.
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
-> ✅.
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.
I still don't understand what the filesystem
configuration is for.
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 usestd
andfs
in that case. Or, if it's preferred to usewrite-to-std
, then it should bewrite-to-clipboard
andwrite-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.
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.
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
clipboard
, fs
and stdout
regardless of each optionReason: 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
I still don't understand what the filesystem configuration is for.
I'm referencing ability to write screenshot on drive as file here
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.
@Decodetalkers What's your opinion on the above? Should we rename it?
@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
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
Can you rebase to remove the merge conflict? Thanks.
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.
+1
@Decodetalkers Would you mind dropping a review? I'll do so too.
Apologies for being late to this, I'll review this now 😄
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
wayshot-%Y_%m_%d-%H_%M_%S.png
-> no changeswayshot -
-- -
character
stdout
-.png
wayshot file
-- file without encoding
Error: The image format could not be determined
file.png
wayshot file.jpg
-- file with jpg
encoding
file.jpg
-> no changeswayshot $HOME/file
-- full directory path with file without encoding
Error: The image format could not be determined
$HOME/file.png
wayshot $HOME/file.jpg
-- full directory path with file with jpg
encoding
$HOME/file.jpg
-> no changeswayshot $HOME/dir
-- full directory path
$HOME/dir/wayshot-%Y_%m_%d-%H_%M_%S.png
-> no changeswayshot dir
-- relative directory path
dir/wayshot-%Y_%m_%d-%H_%M_%S.png
-> no changeswayshot dir/file
-- relative directory path with file without encoding
Error: The image format could not be determined
dir/file.png
wayshot dir/file.jpg
-- relative directory path with file with jpg
encoding
dir/file.jpg
-> no changesI 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
wayshot --clipboard
:
clipboard
error: a value is required for '--clipboard <CLIPBOARD>' but none was supplied
wayshot --clipboard true
:
Error: The image format could not be determined
(because treats true
as filename)clipboard
, the screenshot file with default filename is createdwayshot --clipboard false
:
Error: The image format could not be determined
(because treats false
as filename)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
wayshot-%Y_%m_%d-%H_%M_%S.png
-> no changeswayshot file
-- file path without encoding
Error: The image format could not be determined
file.png
wayshot file.jpg
-- file with jpg
encoding
file.jpg
-> no changeswayshot file --encoding webp
Error: The image format could not be determined
file.webp
wayshot file.jpg --encoding webp
file.jpg
-> no changescli.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.
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? If possible CC: @AndreasBackx ( I hope your health is doing well andreas ).
@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. 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 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.
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!
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:
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 featureWould 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 Option
s (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...
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.toml
file example andconfig.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 (viaunwrap_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/.config
on linux)And changing this behavior:
util::get_default_file_name
must takefilename_format
as parameter -> wait for #93dir
parameter forutil::get_default_file_name
or create a new fn?file
option since we need to separatefile
anddir
paths? Or we can keep the current args behavior and implement this separation only for config? (wait for #96?)cli.cursor
anOption<bool>
, so we can useconfig.cursor
as fallback valuefilename_format: Option<String>
cli option (would default to what's proposed in #93 ifunwrap
fails)config: Option<PathBuf>
cli option (config would default toConfig::default()
ifunwrap
fails)Serialize
andDeserialize
fromserde
forEncodingFormat
enum, so we can ser/de config values directly into enum rather than stringwayshot.rs
file to properly configurecli
/config
values, giving priority tocli
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!