Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added time_stamp flag #93

Merged

Conversation

rachancheet
Copy link

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 .

Copy link
Collaborator

@Decodetalkers Decodetalkers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
@Shinyzenith
Copy link
Member

@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
Copy link
Member

@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
Copy link
Author

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
Copy link
Member

Shinyzenith commented Feb 29, 2024

@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
Copy link
Author

image

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

@Shinyzenith made the requested changes please review.

Copy link
Member

@AndreasBackx AndreasBackx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub time_stamp: bool,
pub timestamp: bool,

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
String::from("")
String::from("unknown")

nitpick

Comment on lines 145 to 161
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)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

image
np problem does this look fine ?

@Shinyzenith
Copy link
Member

image np problem does this look fine ?

CC: @AndreasBackx

@AndreasBackx
Copy link
Member

AndreasBackx commented Mar 15, 2024

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
Copy link
Member

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
Copy link
Author

rachancheet commented Mar 20, 2024

image
@Shinyzenith
@AndreasBackx
Done

Copy link
Member

@AndreasBackx AndreasBackx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -43,4 +43,8 @@ pub struct Cli {
/// Present a fuzzy selector for output/display selection
#[arg(long, alias = "chooseoutput", conflicts_with_all = ["slurp", "output"])]
pub choose_output: bool,

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Shinyzenith
Copy link
Member

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

@rachancheet
Copy link
Author

image

agreed. What should i rename the flag ??

@AndreasBackx
Copy link
Member

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

wayshot/src/wayshot.rs Outdated Show resolved Hide resolved
wayshot/src/utils.rs Outdated Show resolved Hide resolved
wayshot/src/cli.rs Outdated Show resolved Hide resolved
@rachancheet
Copy link
Author

rachancheet commented Mar 21, 2024

Done. @Shinyzenith @AndreasBackx Thanks for the guidance

@Shinyzenith Shinyzenith force-pushed the freeze-feat-andreas branch from d472c71 to 11b07bc Compare March 23, 2024 15:51
Signed-off-by: Shinyzenith <aakashsensharma@gmail.com>
@Shinyzenith Shinyzenith force-pushed the freeze-feat-andreas branch from 11b07bc to 0c531f1 Compare March 23, 2024 15:55
@Shinyzenith
Copy link
Member

Hi, I resolved the merge conflicts.

@Shinyzenith Shinyzenith merged commit 5370c08 into waycrate:freeze-feat-andreas Mar 23, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants