resurrecting-open-source-projects / scrot

SCReenshOT - command line screen capture utility
Other
495 stars 49 forks source link

Revise scrotWaitUntil interface #323

Closed N-R-K closed 1 year ago

N-R-K commented 1 year ago

The timespec argument for nanosleep is awkward because it uses ns when we want to sleep for some ms instead. And scrotWaitUntil is awkward because it uses absolute time, instead of relative time.

Add scrotWaitFor which uses a relative time in ms and uses scrotWaitUntil internally.

CC: @guijan

guijan commented 1 year ago

This API could probably replace scrotWaitUntil() at a later point, would be less lines of code with no difference in behavior.

N-R-K commented 1 year ago

This API could probably replace scrotWaitUntil() at a later point, would be less lines of code with no difference in behavior.

Hmm, hadn't thought about it but you're probably right. I'll try to do this tomorrow with a fresher mind.

guijan commented 1 year ago

Hmm, using the interface as is to replace scrotWaitUntil() means that with 32-bit int, we can wait a maximum of 24 days to take the screenshot. I wonder if that's too close for comfort? Making that into long long doesn't help because then it assumes time_t is 64-bit which isn't necessarily the case on i386 glibc.

N-R-K commented 1 year ago

we can wait a maximum of 24 days to take the screenshot. I wonder if that's too close for comfort?

opt.delay is limited to INT_MAX. If someone needs more, they can use sleep 69d && scrot or something along those line.

N-R-K commented 1 year ago

Ah, wait. opt.delay is in seconds, but this function uses ms. Should also reduce opt.delay to INT_MAX/1000 to avoid mul overflow.

guijan commented 1 year ago

If you provide a version of the function that takes a timespec, and then implement the one that uses milliseconds by calling the timespec version, we don't need the 24 day limit on scrot --delay. I like pushing limits high, we just did that with filenames, but now the delay limit is falling.

guijan commented 1 year ago

Also, with 32-bit timespec (i.e. i386 glibc), it's possible to specify an int large enough to make struct timespec's tv_sec member overflow on master, if you write a timespec version, check for overflow.

N-R-K commented 1 year ago

I like pushing limits high, we just did that with filenames, but now the delay limit is falling.

I think that energy is better spent on issues like #320 or feature requests like #82 that actually concerns scrot's primary purpose - to take screenshots. There's also too many other low hanging fruits with more practical issues (e.g #228) to be bothering with impractical things like 24 day+ delay.

(Honestly this entire sleep saga to me seems like wasted effort fixing a non-issue that would've affected no-one.)