Fix terminals in editor area not reloading#151852
Conversation
- Closes microsoft#140429 - This isn't perfect because it just reopens then without setting the original location, but at least it reloads them.
| seenPersistentProcessIds.push(term.terminal); | ||
| } | ||
| } | ||
| const otherInstances = this.instances.filter(instance => typeof instance.persistentProcessId === 'number' && instance.shouldPersist && seenPersistentProcessIds.indexOf(instance.persistentProcessId) === -1); |
There was a problem hiding this comment.
It would be simpler to get the terminals in the editor area from terminalEditorService
There was a problem hiding this comment.
tried and that doesn't work, but should.
I believe it doesn't work because of this, which happens when you close the window and then the state is saved with no terminals
https://github.com/ssigwart/vscode/blob/a084f63f4d3e5477efbcd8851008e8d996a45681/src/vs/workbench/contrib/terminal/browser/terminalEditorService.ts#L92-L103
|
Thank you for reviewing this and making the updates, @meganrogge. Please let me know if you need me to do anything more on this. |
|
I see the CI failure is safely ignored, we're tracking the flake already |
|
Looks like this doesn't play well with reconnection and it ends up printing the content twice: Still looking into it. |
Tyriar
left a comment
There was a problem hiding this comment.
@ssigwart thanks for the PR, after looking a little closer at this I think it's the wrong approach. Instead of expanding the tabs like we do for terminal groups in the panel we want to rely on the editor's existing serialization as that will already get handled for us and it will then prevent focus/tabs appearing after start up problems.
It's been a while since I looked at this but I think there's a boolean somewhere near the serializer that says close the tab so it doesn't restore on quit (when process revive would be used) but allow is on reload (when process reconnect would be used).
- The issue is `_onWillShutdown` is called before `TerminalInputSerializer` serializes, so I added `shutdownPersistentProcessId`. - When restoring the editor, it was using the wrong ID, so I added `getRevivedPtyNewId`.
|
Thanks for the tip, @Tyriar. It took me quite a while to come up with this new fix, but I don't think I would have gotten there without your suggestion. The issue is After that, when restoring the editor, it was using the ID from before the shutdown, which is no longer valid. So I added I wasn't able to test the remote terminal settings because I wasn't really sure what "remote terminals" meant. I assumed it had something to do with https://code.visualstudio.com/docs/remote/ssh so I tried installing the extension and connect to SSH, but it failed after trying to install a VS Code Server on the server. |
|
@ssigwart for remote you can create a test resolver window for the remote side of things:
|
Tyriar
left a comment
There was a problem hiding this comment.
I tested it out again, revive seems to work great but reconnect appears to have broken.
Repro:
- Create a terminal editor on left and right
- Type something in each
- Reload the window
Expected: What was typed in step 2 should show up (gif off main)
Actual: New processes are created because reconnect fails:
|
Thanks, @Tyriar. I think I got that working now. |
Tyriar
left a comment
There was a problem hiding this comment.
Thanks, it works great 👍, it should land in v1.70
This PR fixes #140429
This isn't perfect because it just reopens then without setting the original location, but at least it reloads them.
If you have any thought on how to do that, I can try to implement it. I think people would probably appreciate them at least reloading though.
Testing
terminal.integrated.persistentSessionReviveProcesssetting is enabled.