tenox7 / ttyplot

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

Fix clock display (regression from conflict resolution for #106) #128

Closed hartwork closed 7 months ago

hartwork commented 7 months ago

Regression from merge commit 95467bf56dc23af61013c6ccc21791aba0a97569 related to #106.

# git diff 4901969fcebb0518f01932e7c7a2d38961d68b99 95467bf56dc23af61013c6ccc21791aba0a97569 | tail -n6

    redraw_and_continue:
-           time(&t1);  // to animate the clock display
            redraw_screen(errstr);
        }
edgar-bonet commented 7 months ago

Thinking about this after a good night... it is not a good idea to depend on execution timing for a CI test to succeed. A better approach would be to remove the display of the time altogether, or replace it with a fixed, dummy string. For example, replace commit af9827ce4afbbe287b943770aa1b4346ae8068c3 with something like this:

// In paint_plot:
if (!keep_time_fixed)
    asctime_r(lt, ls);

// In main:
keep_time_fixed = getenv("FIXED_TIME") != NULL;
if (keep_time_fixed)
    strcpy(ls, "Thu Jan 1 00:00:00 1970");
hartwork commented 7 months ago

it is not a good idea to depend on execution timing for a CI test to succeed

@edgar-bonet I would like to agree but:

I'm happy to sleep over this more, but right now I'm in favor of keeping a moving clock display. But maybe we can make the UI testing CI more robust some other way in a follow-up pull request? What do you think?

hartwork commented 7 months ago

Anyway, this PR is really not disruptive. Let's get it merged, even though we may have to rethink it later.

@edgar-bonet thanks!

On the other hand, I am planning to break this again in the future by making the clock display more “correct”, i.e. updating on the edges of system clock seconds.

We can keep this for later, but if you feel like elaborating already now, I'd be curious to learn more about it.

edgar-bonet commented 7 months ago

My idea is to set the timeout as the time remaining until the next full second. This way pselect would timeout only at the most appropriate moment: right after the “seconds” digits of the system clock has changed. The clock will never be late, not even 0.2 seconds late, and there will be no “useless” redraws at all.

But yes, this will be for later...

hartwork commented 7 months ago

@edgar-bonet that is an interesting idea, but it seems to assume that the process runtime between pselect returning and the redraw of the screen would be exactly zero. Maybe it's close enough. it would allow us to sleep for up to a second, e.g. <=500ms more than currently. I saw the same idea as a bug in stresstest.c ealier but considered it good enough: The rate will be more exact the same way.