Re: [pve-devel] [PATCH] fix #6223: fit terminal after 'OK' message

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 26.03.25 um 09:11 schrieb Dominik Csapak:
> On 3/25/25 19:44, Thomas Lamprecht wrote:
>> Am 18.03.25 um 10:09 schrieb Dominik Csapak:
>>> instead of simply waiting 250ms after we send the credentials, wait
>>> until after the server responded with 'OK' to fit the terminal size.
>>> Still keep the timeout to not do that in the onmessage handler itself,
>>> but rather at a later point in time.
>>
>> potential dumb question, but what's the reason to keep the 250ms in
>> that case?
> 
> not a dumb question at all, and you're right: the exact value of 250ms is strictly not necessary.
> I wanted to keep the code in a timeout, so it does not block the 'onmessage' handler,
> but rather that it runs later when the browser has idle cycles.
> 
> We could of course reduce the timeout, but in my experience, sometimes browsers behave unexpected
> when it's too short (e.g., it then runs immediatly after the JS code, without a render cycle in
> between, which is what i want to avoid here)

any reference for that, especially w.r.t. unexpected behavior, as that
rather just sounds like expected behavior as nothing in the setTimeout
function is designed for being able to order with (re)paint events.


> 
> In practice, omitting the timeout here would naturally work too here, but possibly delay the content
> of the terminal in favor of resizing.
I mean, lowering to something between 20 an 50 ms would be IMO a better
heuristic with less latency, as if the tab is active repaints will happen
at display rate if anything changes and assuming 50 Hz (20 ms period) as
lower bound seems OK, if we want to play it safe then 50 ms would be OK
to.

Alternatively, if what you actually want is to wait one paint we could also
use requestAnimationFrame [0] for that, e.g. something like the following

// wait at least one or more frames
function callbackAfterRepaint(callbackFn) {
    let firstTimestamp;
    let wrapperFn;
    wrapperFn = timestamp => {
        if (firstTimestamp === undefined) {
            firstTimestamp = timestamp;
            requestAnimationFrame(wrapperFn);
        } else if (firstTimestamp === timestamp) {
            requestAnimationFrame(wrapperFn);
        } else {
            callbackFn();
        }
    };
    requestAnimationFrame(wrapperFn);
}


I think comparing the timestamp isn't even a requirement, as nesting this
will lead to two calls for separate frames, but that would need checking
more closely. And the time comparison was based on the following docs:

> When multiple callbacks queued by requestAnimationFrame() begin to fire
> in a single frame, each receives the same timestamp even though time has
> passed during the computation of every previous callback's workload.

But again, probably not required as requisting an animation frame from
inside the callback allways gives the next one already anyway.

[0]: https://developer.mozilla.org/en-US/docs/Web/API/Window/requestAnimationFrame

Using requestAnimationFrame is not a must, I just stumbled upon this again
and wanted to try it, and it feels a bit nicer than waiting some arbitrary
amount if letting pass at least one paint cycle is the goal; I can also
apply as is and just lower the wait period to 50 ms, just tell me what you
think after reading this.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel