Fix Issue #106: Clipboard flag ignored if path is qualified to file #108

CheerfulPianissimo · 2024-03-23T18:15:02Z

No description provided.

…h is qualified to file

Shinyzenith · 2024-03-24T06:10:33Z

Hi, please rebase your changes onto freeze-feat-andreas as there's a merge conflict.

Shinyzenith · 2024-03-24T10:40:04Z

Hi it seems you included a commit which already exists in freeze branch. Why?

… feature flag Signed-off-by: Shinyzenith <[email protected]>

Shinyzenith · 2024-03-24T11:18:09Z

wayshot/src/wayshot.rs

@@ -123,40 +124,42 @@ fn main() -> Result<()> {
        }
    };

+    let mut buffer = Cursor::new(Vec::new());


Lets only create the buffer if clipboard is specified. Else this leads to an extra allocation which we can skip. Lets just call image.save here and create the buffer in the clipboard conditional block.

I considered that but the stdout block also uses the buffer and I didn't want it to do the buffer write twice in the case of --clipboard + stdout. Should I create the buffer there too?

Maybe a function can help

Shinyzenith · 2024-03-24T14:35:41Z

wayshot/src/wayshot.rs

        }
    }

+    if !stdout_print && cli.clipboard {
+        let mut buffer = Cursor::new(Vec::new());


Do we need the cli.clipboard conditional in the stdout conditional? Is this block outside of the conditional not enough?

let image_buf: Option<Cursor<Vec<...> = None; if Some(path) = file { /// save } else { /// to stdout image_buf = ...; } if cli.clipboard && image_buf = Some(image_buffer) { /// Daemonize / create buf if needed }

Shinyzenith · 2024-03-25T20:08:59Z

wayshot/src/wayshot.rs

-        image_buffer.write_to(&mut buffer, requested_encoding)?;
+    if cli.clipboard {
+        let buffer = match image_buf {
+            Some(buf) => buf,


Sorry for the last and final nitpick, just do the match inside of the clipboard daemonize call

fix: Clipboard flag ignored if path is qualified to file (waycrate#108)

* Refactor: introduce FrameGuard, ScreenCapturer, and Logical/EmbeddedRegion * Add freeze functionality * Move to clap_derive so CLI arguments are typed * 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. * Improve CLI design 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. * Remove explicit panics and exits Let's bubble up errors instead of panicking and also not use `exit(...)` because it does not call destructors which might give issues. * Make region, sizing, and positioning data structs reusable 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. * First version of scaling fix. (#85) * chore: remove useless roundtrip (#105) * feat(clipboard): implement clipboard integration (#91) * feat(clipboard): implement clipboard integration Add the --clipboard flag and implement functionality to make image available on the clipboard using wl-clipboard-rs. * style(format): apply code formatting to cli.rs * feat(clipboard): implement fork wait for clipboard Add functionality offering image on clipboard persistently in the background * feat(clipboard): inform user about wayshot persisting in background with --clipboard * feat(clipboard): use tracing::warn instead of print if fork fails * feat(clipboard): Switch from the fork crate to the nix crate for daemonization * style(format): code formatting to wayshot.rs * style(typo): corrected a typo in the comments * docs(ManPage): Fix capitalization issue Signed-off-by: Shinyzenith <[email protected]> * docs: Document clipboard interaction Signed-off-by: Shinyzenith <[email protected]> * chore: Update Cargo.lock Signed-off-by: Shinyzenith <[email protected]> * feat: added time_stamp flag (#93) * feat: added time_stamp flag * Syntax Refactor and filename format fixed * file name format fixes * wayshot-{timestamp}.{extension} as default filename * chore: Update Cargo.lock Signed-off-by: Shinyzenith <[email protected]> --------- Signed-off-by: Shinyzenith <[email protected]> Authored-by: rachancheet <[email protected]> Co-authored-by: Shinyzenith <[email protected]> * chore: Update Cargo.lock Signed-off-by: Shinyzenith <[email protected]> * feat: Add support for webp (#98) Signed-off-by: Shinyzenith <[email protected]> --------- Signed-off-by: Shinyzenith <[email protected]> Co-authored-by: Shinyzenith <[email protected]> * refactor(libwayshot): Reduce allocations (#99) * refactor: remove unnecessary to_string() * refactor: fix clippy warnings * refactor: remove 1 level of indirection via &Vec<T> -> &[T] Typically `&Vec<T>` isn't something you want when you need a read-only view over a Vec, because it adds another level of indirection: Vec already stores a pointer to heap-allocated buffer internally. Using slices `&[T]` removes such unnecessary level of indirection and is considered a cleaner design. It is cache friendlier and can be better optimized by the compiler (not that it should matter in this case). * feat: account for directories in file path (#96) Signed-off-by: Shinyzenith <[email protected]> --------- Signed-off-by: Shinyzenith <[email protected]> Authored-by: rachancheet <[email protected]> Co-authored-by: Shinyzenith <[email protected]> * fix: Clipboard flag ignored if path is qualified to file (#108) * feat(clipboard): fix issue #106 clipboard flag ignored if path is qualified to file * feat(clipboard): clarify flag description of --clipboard * feat(clipboard): enable multiline comment description for --clipboard feature flag Signed-off-by: Shinyzenith <[email protected]> * feat(clipboard): reduce buffer allocations for encoding image * feat(clipboard): improve code quality * feat(clipboard): code style change: perform match inside function call --------- Signed-off-by: Shinyzenith <[email protected]> Co-authored-by: Shinyzenith <[email protected]> * chore: make clippy happy (#111) * feat(clipboard): fix interaction with freeze and select feature: Issue #109 (#110) * feat(clipboard): fix interaction with freeze and select feature: Issue #109 * feat(clipboard): handle callback fails * Destroy layer shell surfaces * [fix] unmap layer shell surfaces before destroying them * Implement missing conversion from string to webp enum (#121) * Replace nix with rustix (#120) * Update documentation with changes in freeze-feat (#116) * [docs] update manpage (1) with changes in freeze-feat * [fix] make arguments to slurp optional * [docs] update manpage(7) with cli changes * [docs] update README with new cli * Add screencopy dmabuf backend (#122) * [feat] rough MVP for screencpy+dmabuf * [feat] refactor MVP into libwayshot - create new constructor * [refactor] first draft of dmabuf API * [refactor] - Add error handling - Add example/demo for wayshot dmabuf API * [feat] import wayland-egl-ctx for use as MVP for dmabuf import functionality * [fix] correct modifier_lo parameter in waymirror-egl MVP * [feat] get waymirror-egl dmabuf MVP demo working * [feat] refactored dmabuf->eglImage API into libwayshot * [feat] - Implemented clean dropping of EGLImage - Improved API docs for dmabuf functions * [fix] libwayshot build error fixed * [fix] remove hardcoded GPU path from libwayshot constructor * [fix]] remove reduntant dmabuf_to_texture call from waymirror-egl * [docs] document WayshotConnection dmabuf constructor * [feat] Added helper/wrapper function to convert screencapture EGLImages into GL textures * [fix] improved logging in dmabuf API code * [doc] update waymirror-egl Readme * [fix] change logging level in waymirror-egl to Debug * [fix] remove unnecessary .gitignore in waymirror-egl * [ci/cd] attempting to fix the build * [ci\cd] add libegl system deps to fix github CI * [fix] remove unused egl_image struct field in Waymirror demo * fix: I find out a smart way to fix scale problem (#128) * fix: I find out a smart way to fix scale problem * Update libwayshot/src/lib.rs Co-authored-by: Aakash Sen Sharma <[email protected]> --------- Co-authored-by: Aakash Sen Sharma <[email protected]> * Fix for 'cargo test' (#114) * fixes for 'cargo test' & bump flake.lock due to outdated cargo * commit to pass `cargo fmt -- --check` test * add `cargo test` to Makefile --------- Co-authored-by: id3v1669 <[email protected]> --------- Signed-off-by: Shinyzenith <[email protected]> Co-authored-by: Access <[email protected]> Co-authored-by: Andreas Backx <[email protected]> Co-authored-by: Andreas Backx <[email protected]> Co-authored-by: Shivang K Raghuvanshi <[email protected]> Co-authored-by: Sooraj S <[email protected]> Co-authored-by: rachancheet <[email protected]> Co-authored-by: Gigas002 <[email protected]> Co-authored-by: Sergey A <[email protected]> Co-authored-by: id3v1669 <[email protected]> Co-authored-by: id3v1669 <[email protected]>

CheerfulPianissimo added 2 commits March 23, 2024 23:36

feat(clipboard): fix issue waycrate#106 clipboard flag ignored if pat…

ca315d8

…h is qualified to file

feat(clipboard): clarify flag description of --clipboard

70976dc

Shinyzenith linked an issue Mar 24, 2024 that may be closed by this pull request

Bug: Clipboard flag ignored if path is qualified to file. #106

Closed

feat(clipboard): enable multiline comment description for --clipboard…

588e2d4

… feature flag Signed-off-by: Shinyzenith <[email protected]>

Shinyzenith force-pushed the fix-clipboard-independence branch from 70a8b85 to 588e2d4 Compare March 24, 2024 11:16

Shinyzenith requested changes Mar 24, 2024

View reviewed changes

Shinyzenith and others added 2 commits March 24, 2024 16:49

Merge branch 'freeze-feat-andreas' into fix-clipboard-independence

fae2bc5

feat(clipboard): reduce buffer allocations for encoding image

fede62e

Shinyzenith requested changes Mar 24, 2024

View reviewed changes

feat(clipboard): improve code quality

7dcfaab

Shinyzenith requested changes Mar 26, 2024

View reviewed changes

feat(clipboard): code style change: perform match inside function call

ff8f394

Shinyzenith merged commit 7381159 into waycrate:freeze-feat-andreas Mar 26, 2024
3 checks passed

Gigas002 added a commit to Gigas002/wayshot that referenced this pull request Mar 26, 2024

Merge pull request #3 from waycrate/freeze-feat-andreas

0faca2f

fix: Clipboard flag ignored if path is qualified to file (waycrate#108)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue #106: Clipboard flag ignored if path is qualified to file #108

Fix Issue #106: Clipboard flag ignored if path is qualified to file #108

CheerfulPianissimo commented Mar 23, 2024

Shinyzenith commented Mar 24, 2024

Shinyzenith commented Mar 24, 2024

Shinyzenith Mar 24, 2024

CheerfulPianissimo Mar 24, 2024

Shinyzenith Mar 24, 2024

Shinyzenith Mar 24, 2024 •

edited

Loading

Shinyzenith Mar 25, 2024

CheerfulPianissimo Mar 26, 2024

Fix Issue #106: Clipboard flag ignored if path is qualified to file #108

Fix Issue #106: Clipboard flag ignored if path is qualified to file #108

Conversation

CheerfulPianissimo commented Mar 23, 2024

Shinyzenith commented Mar 24, 2024

Shinyzenith commented Mar 24, 2024

Shinyzenith Mar 24, 2024

Choose a reason for hiding this comment

CheerfulPianissimo Mar 24, 2024

Choose a reason for hiding this comment

Shinyzenith Mar 24, 2024

Choose a reason for hiding this comment

Shinyzenith Mar 24, 2024 • edited Loading

Choose a reason for hiding this comment

Shinyzenith Mar 25, 2024

Choose a reason for hiding this comment

CheerfulPianissimo Mar 26, 2024

Choose a reason for hiding this comment

Shinyzenith Mar 24, 2024 •

edited

Loading