Fix PseudoTerminal events

[thegecko]

Signed-off-by: thegecko [email protected]

What it does

Well, this is embarrassing. The fix merged for opening terminals and the subsequent revert have both failed to sufficiently fix the terminal opening issues.

It transpires that opening custom terminals via tasks and by manually creating them both have different code paths ending up with the pseudoterminal map tracking the terminals being keyed with different types of IDs.

PR #12005 actually partially fixed this and led me to this PR (thanks @zvzuola), however the resize events and close events weren't fixed.

This PR allows process-based terminals (used by tasks and keyed by process ID) to be re-keyed by the tracked ID (from the widget ID), so all events correctly work for both types of terminal.

How to test

I've created a test extension over at https://github.com/thegecko/web-extension-tasks and the built VSIX can be found in this zip: web-extension-tasks-1.0.0.vsix.zip

It exposes two commands:

• Text: Run task which executes a task using a custom terminal and writes out some text
• Text: Show text which creates a custom terminal and writes out some text

The extension also shows messages when the custom terminal is resized, closed or text is input.

To test, run both these commands and observe the text being displayed and events being raised when resized/closed.

Without this PR, only one command works and not all events are raised.

The issue has been confirmed and fixed on both the electron and browser examples.

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

cc @arekzaluski

[tsmaeder]

Hi Rob, chiming in here, since I've recently dabbled in terminals.

This PR allows process-based terminals (used by tasks and keyed by process ID) to be re-keyed by the tracked ID (from the widget ID), so all events correctly work for both types of terminal.

The progress around this issue worries me a bit. We have "terminal id", "id" and "widget id" and "process id", but to me it's not clear what entities are designated by the various ids and what their lifecycle is. Could you explain your thinking around this in a paragraph or two? When we have to redefine an "id", there's usually a problem with the concepts, not the implementation, in my experience.

[thegecko]

to me it's not clear what entities are designated by the various ids and what their lifecycle is. Could you explain your thinking around this in a paragraph or two?

It's a mess. In the backend (e.g. tasks) it seems the terminal ID comes from the process ID (and I think they remain identical for tasks). As the terminal is created before the widget, widget ID can't be used at this point, but is used later for keying the pseudoterminal. One is a number, one a string.
The custom task terminal is created with a UUID in the frontend and persists this ID instead (as it should IMO). The fix here is to perhaps use a UUID instead of the process ID for tasks, but is still gets created too early and would have to replace the widget ID at som point (with unknown side effects). With limited time, I traced the first point/event where we known about the terminal ID and (widget)ID in the frontend and resolved the relationship.

Happy to see this as a short-term fix with further refactoring required.

[tsmaeder]

I think a lot of the confusion stems from the fact that terminal processes and pseudoterminals are both handled via the shell terminal server: there are numerous places where the front end or back end just does nothing when a pseudoterminal (from a custom task execution or from extension terminal options) is involved. In the end, the terminal widget is only interested in reading and writing text from/to some entity (and the lifecycle of the underlying entity, like a process or task execution). IMO having different back ends for process terminals and pseudo terminals and an abstraction of the two in the front end would clarify things a lot: the id namespaces for back-end entities would be separate for processes and pseudos.
I kinda got started on that when doing the terminal profile stuff, but noticed I didn't really need the refactoring and didn't really have time budget for it.

[thegecko]

In the interests of continuous improvement, shall we consider this fix now and create an issue for refactoring terminal support?

[thegecko]

bump @tsmaeder

[tsmaeder]

@thegecko I have a bit of a work queue. I won't be able to look at this before late this week/early next.

[tsmaeder]

Looks good to me and seems to do the job in test.

[thegecko]

Thanks @tsmaeder I'm going to merge it. Hopefully I won't break master again!