feat: added time_stamp flag #93

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

@@ -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


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 <[email protected]>

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

Hi, I resolved the merge conflicts.

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 Outdated 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

Open

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 <[email protected]>

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
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added time_stamp flag #93

feat: added time_stamp flag #93

rachancheet commented Feb 29, 2024

Decodetalkers left a comment

Shinyzenith commented Feb 29, 2024

AndreasBackx commented Feb 29, 2024

rachancheet commented Feb 29, 2024

Shinyzenith commented Feb 29, 2024 •

edited

Loading

rachancheet commented Feb 29, 2024

rachancheet commented Mar 4, 2024

AndreasBackx left a comment

AndreasBackx Mar 6, 2024

AndreasBackx Mar 6, 2024

AndreasBackx Mar 6, 2024

Shinyzenith Mar 7, 2024

rachancheet commented Mar 7, 2024

Shinyzenith commented Mar 8, 2024

AndreasBackx commented Mar 15, 2024 •

edited

Loading

Shinyzenith commented Mar 19, 2024

rachancheet commented Mar 20, 2024 •

edited

Loading

AndreasBackx left a comment

AndreasBackx Mar 20, 2024

Shinyzenith commented Mar 20, 2024

rachancheet commented Mar 21, 2024

AndreasBackx commented Mar 21, 2024

rachancheet commented Mar 21, 2024 •

edited

Loading

Shinyzenith commented Mar 23, 2024

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

feat: added time_stamp flag #93

feat: added time_stamp flag #93

Conversation

rachancheet commented Feb 29, 2024

Decodetalkers left a comment

Choose a reason for hiding this comment

Shinyzenith commented Feb 29, 2024

AndreasBackx commented Feb 29, 2024

rachancheet commented Feb 29, 2024

Shinyzenith commented Feb 29, 2024 • edited Loading

rachancheet commented Feb 29, 2024

rachancheet commented Mar 4, 2024

AndreasBackx left a comment

Choose a reason for hiding this comment

AndreasBackx Mar 6, 2024

Choose a reason for hiding this comment

AndreasBackx Mar 6, 2024

Choose a reason for hiding this comment

AndreasBackx Mar 6, 2024

Choose a reason for hiding this comment

Shinyzenith Mar 7, 2024

Choose a reason for hiding this comment

rachancheet commented Mar 7, 2024

Shinyzenith commented Mar 8, 2024

AndreasBackx commented Mar 15, 2024 • edited Loading

Shinyzenith commented Mar 19, 2024

rachancheet commented Mar 20, 2024 • edited Loading

AndreasBackx left a comment

Choose a reason for hiding this comment

AndreasBackx Mar 20, 2024

Choose a reason for hiding this comment

Shinyzenith commented Mar 20, 2024

rachancheet commented Mar 21, 2024

AndreasBackx commented Mar 21, 2024

rachancheet commented Mar 21, 2024 • edited Loading

Shinyzenith commented Mar 23, 2024

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