Closed ghost closed 3 years ago
also i'm totally gonna be that person and ask if you could add the hacktoberfest-accepted
label to this pr :p
Looking quite good!
I'm thinking, what do you think how evil - in terms of the different guarantees the two methods provide - would it be to instead have one method taking a Time::Span
and check whether its subsecond component is 0, picking g_timeout_add_seconds
if so and g_timeout_add
otherwise? I guess it would also need a second heuristic of picking g_timeout_add_seconds
if the duration would be too big to fit into an UInt32
as milliseconds.
But my biggest concern with your current proposal is that it very subtly breaks existing code, turning previous seconds long timeouts into milliseconds long ones.
i'm not sure i like the idea of using a Span
object, but i'm still learning crystal's idioms. i also think that would open up the possibility for some weird bugs if that second heuristic happened, since i've read that g_timeout_add
and g_timeout_add_seconds
work slightly differently
i agree that this change is a bit too breaking, and i think any changes to GLib.timeout
will be. perhaps we could add GLib.timeout_seconds
and GLib.timeout_milliseconds
methods, and very slowly deprecate GLib.timeout
with compile-time and run-time warnings?
I'm fine with breaking immediately as long as it throws a compile time error. The library is in 0.x
state after all :)
The advantage of a Time::Span
based API is that you could do thinks like GLib.timeout 1.minute
, which reads quite nicely I think :)
you're right, that does read nicely. maybe something like this could work?
def self.timeout(priority = ..., time : Time::Span, &block : -> Bool):
if time.milliseconds
# if this overflows, let the exception happen
ms = time.total_milliseconds.to_u32
LibGLib.timeout_add(...)
else
sec = time.total_seconds.to_u32
LibGLib.timeout_add_seconds(...)
end
end
apologies for any typos, I'm on a phone right now
Sorry, forgot to answer 🤦
Looks alright to me, I think we do should handle the overflow like I mentioned above if we combine both methods already. Also we should document these behaviors :)
no worries, my memory is finicky too :p
i really think handling overflow like that is a bad idea. g_timeout_add and g_timeout_add_seconds work slightly differently, so i think silently bumping the timeout to add_seconds goes against the principle of least surprise. i think it would be better to either fail on overflow (at compile time if possible) or add some kind of handle_overflow
flag to self.timeout
that defaults to false
Well, by that argument the original idea of combining both is a bad idea too already. I would argue both is ok, if properly documented. Maybe also adding explicit methods for either variant.
My argument for handling the overflow being okay is that it happens at such large values (an interval of many many days), that the precision loss is pretty irrelevant in that region.
My argument for handling the overflow being okay is that it happens at such large values (an interval of many many days), that the precision loss is pretty irrelevant in that region.
that's actually a really good point. realizing that i'm definitely fine handling overflow, but i think it might be a good idea to expose methods for the variants just in case someone has a weird specific edge case
apologies for leaving this silent for so long, been pretty busy. i'll try and finish this pr tonight
i opted for timeout_seconds
and timeout_milliseconds
, so that those two methods are grouped with timeout
in documentation or an editor's autocomplete
added some doc comments, and fixed some typos i saw. sorry if there are formatting issues; vim apparently hates crystal's indenting idioms but i think i caught them all
🙏
hooray! also i'm really really sorry that i just noticed this, but this line should say UInt32::MAX
and not UInt32::Max
:grimacing:
Duh. Maybe we should find some way to test this :D Maybe adding something samples
to make sure it compiles :) Wanna follow up with that?
breaking changes wrt the side effects of GLib.timeout now using milliseconds instead of seconds GLib.timeout has been renamed to Glib.timeoutseconds GLib::Priority has been added to map LibGLib::PRIORITY* constants GLib.timeout and GLib.timeout_seconds now take a priority argument that defaults to GLib::Priority::Default
i think that the most intuitive thing for GLib.timeout to call is g_timeout_add, with a new GLib.timeout_seconds calling g_timeout_add_seconds. the timeout, timeout_seconds, and idle_add methods have also been changed to take an optional priority argument that defaults to LibGLib::PRIORITY_DEFAULT, which is what g_timeout_add uses