[Toybox] [landley/toybox] Simple implementation of less (#39)

Wed Jul 27 04:11:43 PDT 2016

In response to the feedback, an alternative implementation of scr_* that
doesn't pre-allocate too much and all redraw optimizations have been
removed except complete lines.

On Wed, Jul 27, 2016 at 7:42 AM, Toni Spets <toni.spets at iki.fi> wrote:

> On Wed, Jul 27, 2016 at 12:47 AM, Rob Landley <rob at landley.net> wrote:
>
>> On 07/26/2016 02:22 AM, Toni Spets wrote:
>> > @hifi <https://github.com/hifi> pushed 1 commit.
>> >
>> >   * 79ca172 <https://github.com/landley/toybox/commit/79ca172> Less
>> >     magic, more printf
>>
>> It is so weird that github shows that commit as living in my tree
>> instead of yours. I'm glad I only use that site for distribution of
>> masters I keep elsewhere. (Is every commit in the whole of github in a
>> single giant tree with a flat sha1hash collision namespace? I wonder how
>> git catches that, or if the don't care until
>> http://valerieaurora.org/hash.html goes red?)
>>
>
> Well, that's odd.
>
>
>>
>> Ok, looking at git diff 7918d9ff8..79ca172eb0d0
>>
>> The patch adds scr_init() allocating a screen buffer with 4 bytes per
>> character... not an array of wide characters, but a char * with a width
>> you multiply by 4. Ok? I have no idea how you're going to fit combining
>> characters into there. You also don't appear to be storing color
>> information, which is something less -R outputs...
>>
>
> So the first reasoning was that the longest UTF-8 sequence that apparently
> is possible is 4 bytes so I threw a dice that at the worst possible case
> you draw 4 byte sequences all over and it would fit in the buffer. This
> didn't take into account TTY escapes yet which are in the same buffer as
> well.
>
>
>>
>> (So when you scroll the screen back up, you're not just outputting ESC[T
>> and writing a new line of data at the top, but instead doing a memmove
>> on your internal screen buffer? Why?)
>>
>
> Simplification of the implementation. The screen is double buffered and
> the app/toy can update all or only some parts of the backbuffer and doesn't
> need to care how the update is sent to TTY.
>
>
>>
>> Next function: the patch adds scr_linediff()... which has no comment
>> explaining what it does. It's doing a memcmp on an an array of char
>> pointers... scr_init() allocated exacty 2 char pointers, where does this
>> array with a length come from...
>>
>> Ok, let's jump down to the last function added to this file and see if
>> it calls the earlier ones... scr_printf() is printing into SCR_PTR().
>> What's SCR_PTR? It's added to lib.h as
>>
>>   #define SCR_PTR(scr) (scr->buf[0])
>>
>> And this is the only user of that macro anywhere in the code so why is
>> it a macro?
>>
>
> The idea was that a toy can access the current backbuffer with SCR_PTR if
> it wants to draw things directly instead of scr_printf(). Accessing it as
> scr->buf[0] is an ugly implementation details so I hid it.
>
>
>>
>> If you try to write to >x or >y it returns with no error. Otherwise,
>> this function is a wrapper around:
>>
>>   vsnprintf(SCR_PTR(scr) + (scr->l * y), scr->l - x, fmt, va);
>>
>> That finds the start of the y line (scr->l is scr->w*4 and I don't know
>> why you needed to save that to avoid a <<2 in the users), does _not_
>> advance x bytes (or bytes*4) into it, but truncates what we're printing
>> to what presumably _would_ be the end of the line if we had advanced.
>>
>> Again, our screen width is *4 but we're not printing wide characters
>> here. (Did you mean vswprintf?)
>>
>
> It's still incomplete but yes, the only point of it is to safely wrap
> drawing lines on the screen so that it will truncate them properly keeping
> in mind UTF-8 sequences and TTY escapes. It also makes the toy code a bit
> more clean.
>
>
>>
>> ah, the second buffer _isn't_ to store color information, it's so you
>> can diff what you think is on the screen (before the xterm was resized?)
>> with what is now on the screen.
>>
>
> Both buffers contain the whole screen and application always draws to the
> swapped backbuffer.
>
>
>>
>> So, next function up: scr_update() and THAT calls scr_linediff.
>> Iterating through scr->h. What is scr->h? In lib.h it's "unsigned".
>> Great. Back to look at scr_init()... Ah, it's screen height. Ok...
>>
>> +    char *line = scr_linediff(scr->buf[1] + (scr->l * i),
>> +                              scr->buf[0] + (scr->l * i),
>> +                              scr->l,
>> +                              i + 1);
>>
>> This is the only user of scr_linediff() in the code. Why it's a
>> function, I couldn't tell you.
>>
>
> It's hinted inline but it could be unrolled into the loop itself. It's
> mostly to keep the logic separate so I could focus on how would I do the
> diff and not look at the loop itself.
>
> So scr_linediff tries to find the optimal sequence of TTY escapes and
> screen data to update a single line. It isn't complete but it does work
> with ASCII at this point and can draw something like a single or two
> character change anywhere on the screen quite efficiently. When you scroll
> the screen in less/vi it can have a huge impact on the speed if you're on
> 9600 or slower.
>
> If you don't see it being useful enough, this can all be dumbed down a lot
> to the full line update. The good thing about the way I did it is it makes
> it dead simple for the toy to update the screen and it's also efficient on
> the TTY but it has the memory impact of double buffering.
>
> The buffer size could be adjusted on-the-fly if someone is trying to draw
> something that fits on the screen but doesn't fit in the buffer but it
> would require a complete memcpy of every old line at that point for both
> buffers.
>
>
>>
>> Ah, I see! Those for() loops at the start of scr_linediff() are
>> open-coding strnlen(). A non-wide-character version, which will stop at
>> the first zero byte (which wide characters tend to be full of).
>>
>
> I assumed we worked with UTF-8 which doesn't have null bytes, right?
>
>
>>
>> And that's the chunk of time I had, I have a conference call in 12
>> minutes so need to go prepare for that...
>>
>> Rob
>>
>
>
>
> --
> Toni Spets
>

-- 
Toni Spets
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20160727/fa8b74e9/attachment-0003.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Simple-implementation-of-less.patch
Type: text/x-diff
Size: 7226 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20160727/fa8b74e9/attachment-0004.patch>