Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open and shut down terminals #12

Open
jtpio opened this issue May 30, 2024 · 5 comments
Open

Open and shut down terminals #12

jtpio opened this issue May 30, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@jtpio
Copy link
Member

jtpio commented May 30, 2024

Problem

Currently it's possible switch to a running terminal if the widget is already opened in the main area. However it's not possible to re-open a closed terminal widget, or shut down a terminal:

jupyterlite-terminal-open-shutdown.webm

Proposed Solution

Implement these functionalities.

Additional context

Tested with the changes from #11.

@jtpio jtpio added the enhancement New feature or request label May 30, 2024
@ianthomas23
Copy link
Member

Related is that we need to handle an exit command in cockle to shutdown the terminal from within it. Presumably this will need some sort of callback from cockle to terminal.

@ianthomas23
Copy link
Member

I've looked at this and dealing with the shutdown will be fine. But reopening a closed terminal widget is problematic as in JupyterLite this sends an actual REST API request rather than a mocked one.

In JupyterLab the code that gets the list of running terminals is:
https://github.com/jupyterlab/jupyterlab/blob/f8cdbba3b8ac64c80f2c7701f35db4c568716f3f/packages/services/src/terminal/restapi.ts#L70-L71
and it takes serverSettings or creates a new one if none is supplied.

The standard code path to get the list of terminals is
https://github.com/jupyterlab/jupyterlab/blob/f8cdbba3b8ac64c80f2c7701f35db4c568716f3f/packages/services/src/terminal/manager.ts#L217
using the TerminalManager.serverSettings, and works as expected.

However, the equivalent code to reopen a closed terminal widget is
https://github.com/jupyterlab/jupyterlab/blob/f8cdbba3b8ac64c80f2c7701f35db4c568716f3f/packages/terminal-extension/src/index.ts#L363
No setting are passed in so new one is created, and evidently this uses a real WebSocket in Lite rather than a mocked one.

To check this, if I change the last line to

const models = await TerminalAPI.listRunning(serviceManager.serverSettings);

then it works as expected using the mock socket which is routed to the terminal extension and handled by

app.router.get('/api/terminals', ...)

I don't know enough about the Jupyter Lab and Lite relationship to know if the workaround above is a reasonable thing to do, or if JupyterLite should be handling this in some other way such as by overriding the makeSettings in some way.

@jtpio
Copy link
Member Author

jtpio commented Nov 4, 2024

Thanks @ianthomas23 for investigating this!

It looks like this should indeed be fixed in JupyterLab directly with the change you proposed above. Would you like to open a PR?

Then we'll need to get this in a JupyterLab 4.3.x release, and make a release of JupyterLite updated to the latest @jupyterlab packages, after jupyterlite/jupyterlite#1514 is finished.

@ianthomas23
Copy link
Member

It looks like this should indeed be fixed in JupyterLab directly with the change you proposed above. Would you like to open a PR?

Yes, I'll do that tomorrow.

@ianthomas23
Copy link
Member

Corresponding JupyterLab issue jupyterlab/jupyterlab#16920 and PR jupyterlab/jupyterlab#16921.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants