waycrate / wayshot

Mirrored at https://git.sr.ht/~shinyzenith/wayshot | screenshot tool for wlroots based compositors implementing zwlr_screencopy_v1
https://crates.io/crates/wayshot
BSD 2-Clause "Simplified" License
114 stars 20 forks source link

feat: added time_stamp flag #93

Closed rachancheet closed 6 months ago

rachancheet commented 7 months ago

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 .

Shinyzenith commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

image

rachancheet commented 7 months ago

@Shinyzenith made the requested changes please review.

rachancheet commented 7 months ago

image np problem does this look fine ?

Shinyzenith commented 7 months ago

image np problem does this look fine ?

CC: @AndreasBackx

AndreasBackx commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

image @Shinyzenith @AndreasBackx Done

Shinyzenith commented 6 months ago

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

rachancheet commented 6 months ago

image

agreed. What should i rename the flag ??

AndreasBackx commented 6 months ago

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

rachancheet commented 6 months ago

Done. @Shinyzenith @AndreasBackx Thanks for the guidance

Shinyzenith commented 6 months ago

Hi, I resolved the merge conflicts.