[pve-devel] [PATCH] fix #6223: fit terminal after 'OK' message
Dominik Csapak
d.csapak at proxmox.com
Wed Mar 26 11:42:44 CET 2025
On 3/26/25 11:19, Thomas Lamprecht wrote:
> 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.
>
>
Not really. It's maybe also just unexpected to me. I happened to stumble
over similar behavior a few times in the past in extjs, where e.g. a
setTimeout callback was triggered before the browser would update the
dom from the extjs changes immediately before.
>>
>> 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
I'd interpret this in the same way, so a simple
requestAnimationFrame(() => requestAnimationFrame(() => { .. my callback code .. }));
should also work?
>
> 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.
Sure, I can do that, but after thinking a bit, it probably does not really matter either way
and I'm over complicating things. I'm fine with a reduced timeout or omitting it altogether.
I'll send a v2 if that's less work for you
More information about the pve-devel
mailing list