feat: added time_stamp flag by rachancheet · Pull Request #93 · waycrate/wayshot

rachancheet · 2024-02-29T11:37:38Z

currently default filename is "$UNIX_TIMESTAMP-wayshot.$EXTENSION" . This pr adds flag --time_stamp to use human read-able timestamp instead of seconds from UNIX_EPOCH .

Decodetalkers

+1
@Shinyzenith what do you think about? I think it is good

wayshot/src/utils.rs

Shinyzenith · 2024-02-29T14:07:37Z

@Decodetalkers I'm wondering if we should make this the default, UNIX_TIME time isn't a great naming convention. What do you think?

CC: @AndreasBackx Hi apologies for pinging but would like some of your input if possible -- would changing our naming scheme from wayshot-UNIX_TIME.encoding to wayshot-HUMANIZED_TIME.encoding be considered a breaking change?

AndreasBackx · 2024-02-29T14:22:45Z

@Shinyzenith if you mean an ISO8601 timestamp, then yes. If you mean like "1 day ago", then I'd definitely say no.

Also would make the former the default. I'd also make it wayshot-TIME.EXT so they're grouped if sorted by name. It's technically a breaking change, though we're in 0.x so I think this is fine.

I'm away from a PC, though I'd let people override the name how they want and avoid any further name customisations. Though I think we already have this, cannot check rn.

rachancheet · 2024-02-29T14:47:15Z

yes user can provide output file name . if no name is provided it saves file in $UNIX_TIMESTAMP-wayshot.$EXTENSION format which is a bit confusing hence this flag.

Shinyzenith · 2024-02-29T14:54:09Z

@Shinyzenith if you mean an ISO8601 timestamp, then yes. If you mean like "1 day ago", then I'd definitely say no.

Yes I am indeed referring to ISO8601 although this PR isn't formatted with respect to it. I will request that change if everyone agrees on this feature req.

Also would make the former the default. I'd also make it wayshot-TIME.EXT so they're grouped if sorted by name. It's technically a breaking change, though we're in 0.x so I think this is fine.

~~Please correct me if I am mistaken but at the moment we already do wayshot-TIME.EXT.~~ I am mistaken. This should be the new default.

PS: I don't think changing the naming conventions after we hit 1.x would be acceptable so this will only be entertained for the 0.x period but I will still try to keep it to a low frequency.
We won't hit 1.x until the screencopy protocol either gets accepted in ext namespace of wayland-protocols or zwlr is stabilized to wlr. I think we are fine for now

@Decodetalkers Can I get an ack from you to make wayshot-ISO8601_ENCODED_TIME.EXT the new default?

rachancheet · 2024-02-29T14:59:01Z

rachancheet · 2024-03-04T23:55:28Z

@Shinyzenith made the requested changes please review.

AndreasBackx

Looks good, I think we simply need the date as well as that's important. Then --timestamp and use chrono to make it all easier. Once that's done, I think this is good to go.

AndreasBackx · 2024-03-06T23:25:25Z

wayshot/src/cli.rs

+
+    /// Uses time stamp(%H:%M:%S) as file name
+    #[arg(short, long)]
+    pub time_stamp: bool,


Suggested change

pub time_stamp: bool,

pub timestamp: bool,

Timestamp is 1 word, --timestamp is also better than --time-stamp.

AndreasBackx · 2024-03-06T23:26:19Z

wayshot/src/utils.rs

+        Ok(n) => get_hour_minute_from_unix_seconds(n.as_secs()),
+        Err(_) => {
+            tracing::error!("SystemTime before UNIX EPOCH!");
+            String::from("")


Suggested change

String::from("")

String::from("unknown")

nitpick

AndreasBackx · 2024-03-06T23:36:53Z

wayshot/src/utils.rs

+fn get_hour_minute_from_unix_seconds(seconds: u64) -> String {
+    let total_minutes = seconds / 60;
+    let mut current_hour = (((total_minutes / 60) % 24) + 5) % 24;
+    let mut current_minute = (total_minutes % 60) + 30;
+
+    if current_minute > 60 {
+        current_hour += 1;
+    }
+
+    current_minute = current_minute % 60;
+
+    if current_hour == 24 {
+        current_hour = 0;
+    }
+
+    format!("{}:{}:{}", current_hour, current_minute, seconds % 60)
+}


This is only printing time HH:MM:SS when we need the date and time. Though this actually made me think this format might be quite annoying to use because of the colons and possibly spaces: DD-MM-YY HH:MM:SS. Could we go with 2024-12-31 23:59:59? @Shinyzenith

Additionally, this function can be removed and we could use the chrono package. Considering this is a binary, adding a dependency for time stuff is fine.

See the docs here: https://docs.rs/chrono/latest/chrono/format/index.html

This is only printing time HH:MM:SS when we need the date and time. Though this actually made me think this format might be quite annoying to use because of the colons and possibly spaces: DD-MM-YY HH:MM:SS. Could we go with 2024-12-31 23:59:59? @Shinyzenith

That is fine by me, doesn't seem to invasive and is pretty easy to read / reason through.

Additionally, this function can be removed and we could use the chrono package. Considering this is a binary, adding a dependency for time stuff is fine.

I initially had suggested not to increase the dependency graph but if you feel so, we can definitely go forth with chrono. Apologies for the extra work 😅 @rachancheet .

rachancheet · 2024-03-07T19:34:48Z

np problem does this look fine ?

Shinyzenith · 2024-03-08T04:51:44Z

np problem does this look fine ?

CC: @AndreasBackx

AndreasBackx · 2024-03-15T00:20:30Z

Apologies for the late reply! (In the future, feel free to ping me on Discord as well.) Got a new phone and hadn't setup GitHub yet and I don't check github.com too often.

And again further apologies, I noticed that my format DD-MM-YY HH:MM:SS was not what I intended. I also posted 2024-12-31 23:59:59 which would be YYYY-MM-DD HH:MM:SS. The problem with using wayshot-DD-MM-YYYY HH:MM:SS.ext is that when sorted alphabetically, it's not sorted by time (which is very much desired imo). Doing YYYY-MM-SS does have this benefit which is why I was advocating for it.

Also, now that I see it, should we use wayshot-2024_03_08-01_01_43.png instead? This would have the additional benefit that when using this from the terminal, you don't have to quote it.

So do you peeps agree wayshot-YYYY_MM_DD-HH_MM_SS.ext (wayshot-2024_03_08-01_01_43.png) would be ideal?

Shinyzenith · 2024-03-19T15:54:38Z

wayshot-YYYY_MM_DD-HH_MM_SS.ext

I think I would go ahead with this due solely due to the _. Quoting files in terminals can be a huge mess.

rachancheet · 2024-03-20T10:23:25Z

@Shinyzenith
@AndreasBackx
Done

AndreasBackx

Looks good, thank you! I would say we should make this the default, but we can land this.

AndreasBackx · 2024-03-20T11:27:24Z

wayshot/src/cli.rs

    #[arg(long, alias = "chooseoutput", conflicts_with_all = ["slurp", "output"])]
    pub choose_output: bool,
+
+    /// Uses time stamp(%H:%M:%S) as file name


Suggested change

/// Uses time stamp(%H:%M:%S) as file name

/// Uses `wayshot-YYYY_MM_DD-HH_MM_SS.ext` as a filename.

Shinyzenith · 2024-03-20T13:53:30Z

I think default would be best. @rachancheet do you agree? If yes we can do so and land the merge.

rachancheet · 2024-03-21T06:58:47Z

agreed. What should i rename the flag ??

AndreasBackx · 2024-03-21T12:31:02Z

@rachancheet it should be removed your new function should be the body of the old default function.

wayshot/src/wayshot.rs

wayshot/src/utils.rs

wayshot/src/cli.rs

rachancheet · 2024-03-21T15:54:40Z

Done. @Shinyzenith @AndreasBackx Thanks for the guidance

Signed-off-by: Shinyzenith <aakashsensharma@gmail.com>

Shinyzenith · 2024-03-23T15:57:29Z

Hi, I resolved the merge conflicts.

* 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 <aakashsensharma@gmail.com> * docs: Document clipboard interaction Signed-off-by: Shinyzenith <aakashsensharma@gmail.com> * chore: Update Cargo.lock Signed-off-by: Shinyzenith <aakashsensharma@gmail.com> * 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 <aakashsensharma@gmail.com> --------- Signed-off-by: Shinyzenith <aakashsensharma@gmail.com> Authored-by: rachancheet <raxxsngh37@gmail.com> Co-authored-by: Shinyzenith <aakashsensharma@gmail.com> * chore: Update Cargo.lock Signed-off-by: Shinyzenith <aakashsensharma@gmail.com> * feat: Add support for webp (#98) Signed-off-by: Shinyzenith <aakashsensharma@gmail.com> --------- Signed-off-by: Shinyzenith <aakashsensharma@gmail.com> Co-authored-by: Shinyzenith <aakashsensharma@gmail.com> * 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 <aakashsensharma@gmail.com> --------- Signed-off-by: Shinyzenith <aakashsensharma@gmail.com> Authored-by: rachancheet <raxxsngh37@gmail.com> Co-authored-by: Shinyzenith <aakashsensharma@gmail.com> * 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 <aakashsensharma@gmail.com> * 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 <aakashsensharma@gmail.com> Co-authored-by: Shinyzenith <aakashsensharma@gmail.com> * 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 <60808802+Shinyzenith@users.noreply.github.com> --------- Co-authored-by: Aakash Sen Sharma <60808802+Shinyzenith@users.noreply.github.com> * 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 <nico@nico.ni> --------- Signed-off-by: Shinyzenith <aakashsensharma@gmail.com> Co-authored-by: Access <ShootingStarDragons@protonmail.com> Co-authored-by: Andreas Backx <andreas@backx.org> Co-authored-by: Andreas Backx <AndreasBackx@users.noreply.github.com> Co-authored-by: Shivang K Raghuvanshi <136506971+shivkr6@users.noreply.github.com> Co-authored-by: Sooraj S <94284954+CheerfulPianissimo@users.noreply.github.com> Co-authored-by: rachancheet <55895940+rachancheet@users.noreply.github.com> Co-authored-by: Gigas002 <24297712+Gigas002@users.noreply.github.com> Co-authored-by: Sergey A <7361274+murlakatamenka@users.noreply.github.com> Co-authored-by: id3v1669 <57532211+id3v1669@users.noreply.github.com> Co-authored-by: id3v1669 <nico@nico.ni>

feat: added time_stamp flag

5e31306

Decodetalkers reviewed Feb 29, 2024

View reviewed changes

Shinyzenith requested changes Feb 29, 2024

View reviewed changes

wayshot/src/utils.rs Outdated Show resolved Hide resolved

wayshot/src/utils.rs Outdated Show resolved Hide resolved

wayshot/src/utils.rs Outdated Show resolved Hide resolved

wayshot/src/utils.rs Outdated Show resolved Hide resolved

wayshot/src/utils.rs Outdated Show resolved Hide resolved

Syntax Refactor and filename format fixed

63a89af

rachancheet force-pushed the freeze-feat-andreas branch from f8b3f93 to 63a89af Compare March 1, 2024 15:40

AndreasBackx suggested changes Mar 6, 2024

View reviewed changes

file name format fixes

44b11d7

Decodetalkers approved these changes Mar 20, 2024

View reviewed changes

AndreasBackx approved these changes Mar 20, 2024

View reviewed changes

AndreasBackx reviewed Mar 21, 2024

View reviewed changes

wayshot/src/wayshot.rs Outdated Show resolved Hide resolved

wayshot/src/utils.rs Show resolved Hide resolved

wayshot/src/cli.rs Outdated Show resolved Hide resolved

wayshot-{timestamp}.{extension} as default filename

80c1f86

Decodetalkers approved these changes Mar 21, 2024

View reviewed changes

AndreasBackx approved these changes Mar 21, 2024

View reviewed changes

Gigas002 mentioned this pull request Mar 22, 2024

Implement config file #104

Closed

Shinyzenith force-pushed the freeze-feat-andreas branch from dcf7364 to d472c71 Compare March 23, 2024 15:50

Shinyzenith force-pushed the freeze-feat-andreas branch from d472c71 to 11b07bc Compare March 23, 2024 15:51

chore: Update Cargo.lock

0c531f1

Signed-off-by: Shinyzenith <aakashsensharma@gmail.com>

Shinyzenith force-pushed the freeze-feat-andreas branch from 11b07bc to 0c531f1 Compare March 23, 2024 15:55

Shinyzenith approved these changes Mar 23, 2024

View reviewed changes

Shinyzenith merged commit 5370c08 into waycrate:freeze-feat-andreas Mar 23, 2024

	/// Uses time stamp(%H:%M:%S) as file name
	/// Uses `wayshot-YYYY_MM_DD-HH_MM_SS.ext` as a filename.

Uh oh!

Conversation

rachancheet commented Feb 29, 2024

Uh oh!

Decodetalkers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Shinyzenith commented Feb 29, 2024

Uh oh!

AndreasBackx commented Feb 29, 2024

Uh oh!

rachancheet commented Feb 29, 2024

Uh oh!

Shinyzenith commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rachancheet commented Feb 29, 2024

Uh oh!

rachancheet commented Mar 4, 2024

Uh oh!

AndreasBackx left a comment

Choose a reason for hiding this comment

Uh oh!

AndreasBackx Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

AndreasBackx Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

AndreasBackx Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

Shinyzenith Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

rachancheet commented Mar 7, 2024

Uh oh!

Shinyzenith commented Mar 8, 2024

Uh oh!

AndreasBackx commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shinyzenith commented Mar 19, 2024

Uh oh!

rachancheet commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndreasBackx left a comment

Choose a reason for hiding this comment

Uh oh!

AndreasBackx Mar 20, 2024

Choose a reason for hiding this comment

Uh oh!

Shinyzenith commented Mar 20, 2024

Uh oh!

rachancheet commented Mar 21, 2024

Uh oh!

AndreasBackx commented Mar 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rachancheet commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shinyzenith commented Mar 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Shinyzenith commented Feb 29, 2024 •

edited

Loading

AndreasBackx commented Mar 15, 2024 •

edited

Loading

rachancheet commented Mar 20, 2024 •

edited

Loading

rachancheet commented Mar 21, 2024 •

edited

Loading