Refactor the handling of input data (+ add minimal support for negative input values)

[edgar-bonet]

Draft pull request

The purpose of this pull request is to simplify the non-blocking handling of input data. The relevant code is taken out of the main loop and split into three small functions. The main loop now strictly follows this simple pattern:

while (1) {
    wait_for_next_event();
    handle_the_events_we_got();
}

The data events are fully handled within the loop iteration that caught them, even if they provide more than one data record. The data is now tokenized and parsed using strtok()/strtod() instead of sscanf(). Tokens that do not parse to numbers are interpreted as zeroes: no NANs are generated.

This PR is marked as “draft” because it is intended to be merged in a future “development” branch, that integrates previous pull requests. A such, it sits on top of my branch “next”, which merges PRs #98, #99, #106, #110 and #114. Note that, since NANs are not generated, PR #112 becomes irrelevant to this branch. This PR proper is a single commit that shrinks ttyplot.c by 40 lines. See the diff.

[edgar-bonet]

once we start accepting half-garbage there will be no way going back without breaking existing usage. How about we remain strict for now and add a command line argument for a tolerant mode later (not in the this pull request) if we find its needed? I would vote for being strict by default, for now. What do you think?

I think if we later add a command-line argument for a more tolerant mode, there would be no reason to not let that mode permanently enabled. Don't you think?

[hartwork]

once we start accepting half-garbage there will be no way going back without breaking existing usage. How about we remain strict for now and add a command line argument for a tolerant mode later (not in the this pull request) if we find its needed? I would vote for being strict by default, for now. What do you think?

I think if we later add a command-line argument for a more tolerant mode, there would be no reason to not let that mode permanently enabled. Don't you think?

@edgar-bonet as I said, there will be no way going back without breaking existing usage. I'd be happy with all strict or all strict plus an option for tolerance. What value do you see in being tolerant by default?

[edgar-bonet]

What value do you see in being tolerant by default?

Ease of use. Never mind, I changed it to a non-tolerant mode.

I noticed there is a CI failure on the asciinema test. I don't understand what went wrong: frame number 3 is kind of repeated, with only the clock being updated. Maybe the timings are not so reliable?

I also tried record.sh on my box, and it failed because asciinema does not recognize the options --cols and --rows. It turns out these are not mentioned on the asciinema documentation.

[hartwork]

What value do you see in being tolerant by default?

Ease of use. Never mind, I changed it to a non-tolerant mode.

@edgar-bonet thank you!

I noticed there is a CI failure on the asciinema test. I don't understand what went wrong: frame number 3 is kind of repeated, with only the clock being updated. Maybe the timings are not so reliable?

I was thinking that it made sense since your code processes multiple values per call to pselect while current development only processes one but then your code should be back at the shell faster. Pull request #123 also has timing issues but here it seems clear: The sanitizers make the code much slower. Let me think about options for a fix, we definitively need some and I got us into this so… :smiley:

I also tried record.sh on my box, and it failed because asciinema does not recognize the options --cols and --rows. It turns out these are not mentioned on the asciinema documentation.

Interesting! I got it from this:

# asciinema --version
asciinema 2.3.0

# asciinema rec --help | grep -E 'cols|rows'
                     [--cols COLS] [--rows ROWS] [-y] [-q]
  --cols COLS           override terminal columns for recorded process
  --rows ROWS           override terminal rows for recorded process

We could try playing with explicit values for variables COLUMNS and LINES instead and see if we get the same effect.

[hartwork]

We could try playing with explicit values for variables COLUMNS and LINES instead and see if we get the same effect.

@edgar-bonet that only got me in trouble, one way to fix it for everyone would be pull request #127.

[hartwork]

I noticed there is a CI failure on the asciinema test. I don't understand what went wrong: frame number 3 is kind of repeated, with only the clock being updated. Maybe the timings are not so reliable?

I was thinking that it made sense since your code processes multiple values per call to pselect while current development only processes one but then your code should be back at the shell faster. Pull request #123 also has timing issues but here it seems clear: The sanitizers make the code much slower. Let me think about options for a fix, we definitively need some and I got us into this so… 😃

@edgar-bonet I think I know know what's going on. Two things actually:

• For the sanitizers just giving them a bit more time seems to solve the problem, I fixed #123 just now accordingly. That's good new but also the boring part.
• For the screenshots not matching the CI turns out is doing exactly what it's supposed to do — uncovering a bug that none of us humans noticed: On current development the clock is broken, it's not moving. We're sleeping more than a second total during UI testing and yet all the screenshots show time "00:00:00" — that's not right. Your changes here seem to fix the clock "as a side effect" and now that the clock is moving again the changes in clock produce both more and different images. Would be interesting to see where the clock broke on the way. Damn! :smiley:

Does that make sense?

[hartwork]

@edgar-bonet PS: Found the regressing commit and an easy fix, new pull request #128.

[hartwork]

@edgar-bonet PPS: I have five yet-to-reply to replies from you from review further up, they are not forgotten, I have five e-mails pointing to just them, replies likely coming up Monday evening.

[edgar-bonet]

Rebased on development, added Co-authored-by Sebastian Pipping to the commit message.

[edgar-bonet]

The CI failure seems spurious to me: a timing issue again.

[hartwork]

Rebased on development, added Co-authored-by Sebastian Pipping to the commit message.

@edgar-bonet thanks!

The CI failure seems spurious to me: a timing issue again.

I'll see what I can do.