[Toybox] [patch] watch cleanup

[Rob Landley]

On 10/13/2013 08:41:33 AM, Elie De Brauwer wrote:
> Hello Rob, all,
> 
> Attached is a patch which performs some minor cleanup for watch.

Applied, but I need to do vi/less/top/watch infrastructure to print out  
data that is:

A) Incrementally produced.

B) Going off the right and/or bottom edge of the screen without causing  
the display to scroll. (Optional wordwrap flag would be good though. Or  
maybe a callback function that determines where in the string to wrap  
at?)

C) Potentially full of ansi escape sequences and other nonprintable  
characters. (Either expand everything or let through color changes but  
clean up after them and expand everything else? And how do you handle  
tabs in the input, excape them or display them or making the "visible  
whitespace" somehow...?)

D) Full of unicode characters where multiple input bytes become one  
output character. (I am assuming fixed width font even for unicode; any  
non-english speaker think that's a really bad assumption?)

Anyway: design issues, but once the infrastructure's there it should  
have lots of uses. So I'm not too worried about interstitial cleanups  
in the meantime if we're just throwing them out again once the full  
solution's there.

> - it adds support for the -e flag to stop the while loop when the
> command executed runs into an error. (but I replaced the 'press a key'
> by a 'press enter', i don't think there's anything generic in toybox
> yet which waits for a single keypress ?)

You'd have to put the input into raw mode and I haven't written stty  
yet (although I need to do that for microcom), so that's good for now  
but there's a todo item...

> - the real watch has some intelligence where the width of the terminal
> influences the contents of the header, I implemented a basic version
> of this (it doesn't get triggered when the window gets changed and it
> doesn't support the ... notation for partial commandline printing).

Less has the ability to scroll left and right; once I implement that I  
can just trigger the same functionality in watch (with it rememering  
current position between redraws)

> - i modified the formatting of the time to just using ctime() which is
> also what upstream does.
> - i removed a memory leak in the creation of the 'cmd' buffers.
> 
> Some things which crossed my mind but I didn't add in this commit:
> - the real watch supports non-integer arguments e.g. watch -n 0.1 ls
> to perform ls every 100 msecs, how would this be done with the
> config_float ?

I added support for that to sleep, so there's an example. A  
read_timeout() function doing select (and wrapped around a  
multi-filehandle version, but _not_ messing with global signal state  
unnecessarily) is on my todo list too.

The end goal of this would do a read_timeout() on stdin put into raw  
mode, which would require signal handlers to take it back _out_ of raw  
mode on exit.

In fact, fixing raw mode leakage is a shell job control problem. If you  
ctrl-Z a program the shell should work, and if you then "fg" that  
program again the _program_ should work. So doing this right is a toysh  
problem...

(It's sort of sad that my proposed crowdfunding thing was so thoroughly  
ignored it didn't seem worth trying and I just got a new day job  
instead. Oh well, dink away with weekends like I've been doing...)

> Ideally we would be able to do something with
> CFG_TOYBOX_FLOAT() which falls back to integer operation when its not
> set. Is this doable ?

It's what sleep does.

> - I'm tempted to replace the entire xmsprintf() story by using toybuf,
> but I'd fear running into a buffer overflow there, so I just left it
> there.

I had a partial cleanup on watch.c in slush pile but I've just started  
deleting those sort of things to check in stuff other people with more  
time feel like touching. (I can redo the cleanup later.)

One of the issues I was addressing was that "watch ls -l" does not  
actually need quotes around ls -l.

Doing an snprintf with sizeof(toybuf)-1 is probably fine, if your  
terminal width is bigger than 4k you get a truncated command line and  
that's ok. Here, I can do that... Why on _earth_ is terminal_size()  
looping over TIOCGWINSZ? (I'm sure I had a reason at one point. Closest  
I can grep out of my blog in 15 seconds is:

   http://landley.net/notes-2007.html#19-10-2007

Right, stop doing it and see what breaks...

Oh yeah, design issue of not setting anything we can't actually find vs  
repeating the default values everywhere. Hence passign in pointers and  
not modifying the default values (which are thus repeated in every  
caller), vs trying to signal "they had $COLUMNS set but not $ROWS and  
it's otherwise a serial console in a context I can't do ansi probes  
because stdin is a pipe" in the return code. Which is why I erred on  
the side of repeating the default values, but really, does any caller  
so far _care_ that we couldn't probe and just wants a value they can  
use? Yes, ls does, because you only default to -C behavior when you can  
tell how big the screen is.

Sigh. Fiddling with it...

Rob