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

[Dominik Csapak <d.csapak@proxmox.com>]

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


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