fix: initialize terminal with correct size #10369
Conversation
This does not fix any bugs (that I know of) but we only need to fit once when the terminal is created, not every time we reconnect. Granted, currently we do not support reconnecting without refreshing anyway so it does not really matter, but this just seems more correct. Plus now we will not have to pass the fit addon around.
I think this will solve an issue where screen does does not correctly handle an immediate resize. It seems to ignore the resize, but even if you send it again nothing changes, seemingly thinking it is already at that size?
Decoding a JSON message does not touch omitted (or null) fields so once a message with a resize comes in, every single message from that point will cause a resize. I am not sure if this is an actual problem in practice but at the very least it seems unintentional.
1ab2f22 to
eb2afb6
Compare
| window.addEventListener("resize", listener); | ||
|
|
||
| // Terminal is correctly sized and is ready to be used. | ||
| setTerminal(terminal); |
There was a problem hiding this comment.
Is moving the setTerminal call just for clarity? Just making sure – because of how/when React flushes state changes, moving the call in this case doesn't actually change anything
There was a problem hiding this comment.
Yup exactly, I just felt it flowed better but that is probably rather subjective.
There was a problem hiding this comment.
To be honest part of me also wanted to move it just because I do not necessarily trust what React does behind the scenes hahaha, this way I think the intent is clearer at least even if it is functionally identical.
| // We have to fit twice here. It's unknown why, but the first fit will | ||
| // overflow slightly in some scenarios. Applying a second fit resolves this. | ||
| fitAddon.fit(); | ||
| fitAddon.fit(); |
There was a problem hiding this comment.
I don't know much about xterm, but moving the calls does seem like a good change.
Just making sure for my own understanding – is the terminal expected to resize only when the browser/window viewport does?
There was a problem hiding this comment.
Yep, exactly. We set the size on page load, send the current size on connect, and we only need resize events on subsequent size changes of the viewport.
Currently we spawn the terminal at 80x80 (the default when no initial size is provided) and then resize shortly afterward.
It appears that
screensometimes ignores this resize, but at the same time it seems to remember it because trying to resize it again with that size does nothing.The solution seems to be to start
screenwith the right size from the get-go. From my testing, this fixes #9806. It can be done by providing the size as query parameters on the web socket URL.I also tweaked the
fitAddoncalls and fixed an issue with every message causing a resize (more info in the commit messages).