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

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

[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.

This fixes an issue with not properly fitted area, when it takes longer
than 250ms to establish the connection and the first fit would be before
we could send the client size to the server.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 xterm.js/src/main.js | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xterm.js/src/main.js b/xterm.js/src/main.js
index 289032c..b4b5c43 100644
--- a/xterm.js/src/main.js
+++ b/xterm.js/src/main.js
@@ -222,6 +222,12 @@ function runTerminal() {
 	    if (answer[0] === 79 && answer[1] === 75) { // "OK"
 		updateState(states.connected);
 		term.write(answer.slice(2));
+
+		// initial focus and resize
+		setTimeout(function() {
+		    term.focus();
+		    fitAddon.fit();
+		}, 250);
 	    } else {
 		socket.close();
 	    }
@@ -247,12 +253,6 @@ function runTerminal() {
     });
 
     socket.send(PVE.UserName + ':' + ticket + "\n");
-
-    // initial focus and resize
-    setTimeout(function() {
-	term.focus();
-	fitAddon.fit();
-    }, 250);
 }
 
 function getLxcStatus(callback) {
-- 
2.39.5



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

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

[Thomas Lamprecht]

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?


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

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

[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)

In practice, omitting the timeout here would naturally work too here, but possibly delay the content
of the terminal in favor of resizing.

Does that make sense to you?


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

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

[Thomas Lamprecht]

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

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

[Dominik Csapak]

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

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

[Thomas Lamprecht]

Am 26.03.25 um 11:42 schrieb Dominik Csapak:
> On 3/26/25 11:19, Thomas Lamprecht wrote:
>> 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?

would make sense to me.


> I'll send a v2 if that's less work for you

Yeah, that would simplify it a bit for me.


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