Make region, sizing, and positioning data structs reusable #78
Conversation
|
Changes in commit: 1a0dd42 seems good to me! Approved! |
|
This PR stack is good to go for me. The PPM regression seems to exist in master too so I won't be marking that as a blocker for this PR stack. @AndreasBackx Would you have the time to rebase this PR to some recent changes in libwayshot? there's a merge conflict. @Decodetalkers This pr is green for me, I'll be merging this soon unless there's any further comments on this. |
|
Ok! Please |
|
Will try to rebase this week and post back. Thanks! :) |
* Renames `clap.rs` to `cli.rs` because otherwise there's confusion between the `clap` crate and local module. * `Cli` struct added that almost identically represents the current state of the CLI with no logical changes. --- ## `--help` Comparison Before: https://gist.github.com/AndreasBackx/5945b366e989159f4669e7ba30c13239 After: https://gist.github.com/AndreasBackx/8929c8bde080eac0cafd33128210b0cc Diff (ignoring whitespace changes due to table alignment): ```diff 1c1 < Screenshot tool for compositors implementing zwlr_screencopy_v1. --- > Screenshot tool for wlroots based compositors implementing the zwlr_screencopy_v1 protocol. 9d8 < -c, --cursor Enable cursor in screenshots 10a10 > -c, --cursor Enable cursor in screenshots 12c12 < -l, --listoutputs List all valid outputs --- > -l, --list-outputs List all valid outputs 14c14 < --chooseoutput Present a fuzzy selector for outputs --- > --choose-output Present a fuzzy selector for outputs 16a17 > ``` You can see that the only changes are: - About is longer, this is now using the value from Cargo.toml instead of a duplicate text that was shorter. - Some have a dash where the English words would have a space, e.g: "list-outputs" instead of "listoutputs". I've also made the old still work via an alias: https://gist.github.com/AndreasBackx/6025e91844e3d766d4264a01ae4d1a71 This seems like a tiny improvement? I plan to make further changes later, but I want to keep PRs separate.
This "improves" (and that is subjective) the design of the CLI. I am aiming to get some feedback on what people think of the new design:
```
Screenshot tool for wlroots based compositors implementing the zwlr_screencopy_v1 protocol.
Usage: wayshot [OPTIONS] [OUTPUT]
Arguments:
[OUTPUT]
Where to save the screenshot, "-" for stdout. Defaults to "$UNIX_TIMESTAMP-wayshot.$EXTENSION"
Options:
--log-level <LOG_LEVEL>
Log level to be used for printing to stderr
[default: info]
[possible values: trace, debug, info, warn, error]
-s, --slurp <SLURP_ARGS>
Arguments to call slurp with for selecting a region
-c, --cursor
Enable cursor in screenshots
--encoding <FILE_EXTENSION>
Set image encoder, if output file contains an extension, that will be used instead
[default: png]
[aliases: extension, format, output-format]
Possible values:
- jpg: JPG/JPEG encoder
- png: PNG encoder
- ppm: PPM encoder
- qoi: Qut encoder
-l, --list-outputs
List all valid outputs
-o, --output <OUTPUT>
Choose a particular output/display to screenshot
--choose-output
Present a fuzzy selector for output/display selection
-h, --help
Print help (see a summary with '-h')
-V, --version
Print version
```
The main changes are:
1. `--debug` is now `--log-level` because this makes it easy to select more specifically what log level to use. I considered using `-v`, `-vv`... to increase verbosity but the `clap-verbosity-crate` uses `log` and not `tracing`. We could use it somewhat, but we'd pull in `log` (which seems fine) and we'd need to map from `log`'s Level to `tracing`'s Level enums (they use inverse ordering).
2. `--stdout` and `--file` has been made an optional positional argument. This because it's what other CLIs often do and I wasn't sure what to call the option otherwise because `--output` and `-O`/`-o` is often what others use but we use it here to refer to displays/monitors/Wayland outputs. This avoids that confusion hopefully and I've also clarified this in the documentation.
* Additionally if a path is given, its extension will always be used. So you cannot save `jpg` to `foo.png`. Perhaps this behaviour can be changed, though I don't see a reason to support this weird edge case? When is someone saving `png` to `jpg`?
3. `--extension` is `--encoding` with aliases like `extension`.
Again, let me know what you think.
Let's bubble up errors instead of panicking and also not use `exit(...)` because it does not call destructors which might give issues.
We use different structs/fields to store the same three types of data everywhere: - Position (x,y) - Size (width,height) - Region(position, size) This makes it so they all reuse the same information.
|
Hi @Decodetalkers. Andreas has rebased this pr. Would you mind reviewing it one last time before the merge? Known issues:
|
|
Ok, I will try it tomorrow |
|
Emm. the problem scale is not fixed now.. and I do not think now it need any args |
|
@Decodetalkers from what @Shinyzenith says, the scaling issue is also on master? |
Emm, what I concerned is that it will always ask you to select again, so the args will be needless |
|
@Decodetalkers yes, it will ask you to select again. Those are the arguments that will be given to slurp from wayshot, not the output from a slurp call. |
but you still need to resize the picture to the size of xdgoutput, and , the args need should be the output of slurp, why select again. I think at first, it should shoot all screens, then use slurp to select and cut the picture, right? |
These arguments are given after the freeze has happened. Slurp is called when the screen is frozen. This is how it goes when freeze is enabled:
|
Emm, it need args |
|
I'm a little confused with the slurp interaction. However, that is my fault due to not reading the thread completely. Give me some time. Maybe a lot of this slurp interaction could be removed by integrating waysip into wayshot. I will do that after #78 is complete. |
|
Technically slurp shouldn't be required, it should be made optional. I think if we plan on integrating waysip, could we land with the issue? As long as we don't tag a new version, that should hopefully be okay? Otherwise, it's relatively easy to make it optional. We can fix it after landing all of these PRs and maybe have a clearer head. |
My main trouble still be, the picture won't be full of the screen when scale is 1.5, other thing will fine. I still think the picture should be resized to the size of xdg output |
|
@Decodetalkers considering there are also scaling issues on the master branch. Can this be fixed in a later version? This is already a really big change and pushing even more stuff into it is going to make this even harder to land. |
Emm, ok, but another problem is that --slurp still need an arg, but it need to be selected again, so is it designed to this kind? it do not use the area passed by arg, I will glad to make this pr go, but only one I am confused is still this |
|
@Decodetalkers I am having a hard time understanding what you mean exactly. Though |
|
@AndreasBackx I think it's best to work on the waysip integration in that case, I'd want the scaling issue to land in another patch ( I'll have it taken care of ). Should I go ahead and try to land a patch stack on top of pr78? I'll send a patch over Discord to you if that's fine? |
|
It's up to you. I'd say it would make everyone's life easier to merge and branch from master afterwards. If we don't roll out a new version in the meantime and iron out more kinks, that should be okay? |
I think that would be fine. There are some |
|
@Decodetalkers The slurp issue will be tackled by introducing waysip in another branch and merged into master. Apart from scaling and slurp, is everything okay according to you? I'd love to have your approval before going forth with the merge. |
|
@Shinyzenith can it be merged now? |
Make region, sizing, and positioning data structs reusable
We use different structs/fields to store the same three types of data everywhere:
This makes it so they all reuse the same information.
Stack created with Sapling. Best reviewed with ReviewStack.