search

tenox7 / ttyplot

a realtime plotting utility for terminal/console with data input from stdin
Apache License 2.0
956 stars 42 forks source link

Extract function `wait_for_events` (follow-up to #98 and #115) #149

Closed hartwork closed 7 months ago

hartwork commented 7 months ago

Follow-up to #98 and #115

@edgar-bonet this is just an idea. I'm happy to rebase this onto #148 after its merge. Also, it doesn't block anything about your idea of extracting function time_till_next_full_second a la https://github.com/tenox7/ttyplot/pull/146#pullrequestreview-1762721004 before or after. Just an idea :beers:

hartwork commented 7 months ago

Well, why not. Could you please rebase on top of current devel. I wonder how it will come out.

@edgar-bonet sure, done.

EVENT_TIMEOUT will probably have to be renamed (it's more like a “displayed clock is out of date” event)

wait_for_events ideally does not have any knowlegde of a clock as that would violate the single responsibility principle of SOLID. I was about to write "the clock display semantic is something that wait_for_events knows nothing about and that's great" and then realized that the timeout calculation does have clock business to it. So the logical consequence was to separate the two and extract another function calculate_clock_refresh_timeout_from and now the two have one business each and that only.

and given a non-zero value that could be or'ed with others.

There is no true need for that since EVENT_TIMEOUT only ever comes in isolation and that is documented, but I have made it use fully disjoint bit patterns now anyway for consistency and also to avoid the next generation from misusing it by accident.

Also, this function may have to inherit the responsibility of updating the global now.

That would be too much when "waiting for events" is the official contract (and would also get it into the need-for-redrawing business where it has no place). Updating now is just right at home in main if you ask me.

Pushed just now, let me know what you think.

hartwork commented 7 months ago

It feels weird that EVENT_TIMEOUT is set but never used. But then, from the point of view of separation of concerns, it makes sense that wait_for_events doesn't care whether the reported events are ever used.

@edgar-bonet I feel the exact same way.

LGTM!

Cool! Merging…

Is there anything left before a handover from us to Antoni?

edgar-bonet commented 7 months ago

I was thinking of supporting negative values. However, I just got a flu, and my available energy is pretty low now. I think it is more reasonable to let that for another round. Let's get all this into Antoni's hands!

PS: Negative values are already somewhat supported, one just has to explicitly ask for them. For instance, seq -10 0.2 10 | ttyplot -M -10 works just fine.

hartwork commented 7 months ago

I was thinking of supporting negative values. However, I just got a flu, and my available energy is pretty low now. I think it is more reasonable to let that for another round. Let's get all this into Antoni's hands!

@edgar-bonet my best wishes for a smooth recovery! Sounds fair, I'll reply to the related mail saying we'd like to hand over, you'll be in CC. It has less of a chance to go under in GitHub notification noise then, probably.

PS: Negative values are already somewhat supported, one just has to explicitly ask for them. For instance, seq -10 0.2 10 | ttyplot -M -10 works just fine.

Good to know!