jupyterlite / terminal

A terminal for JupyterLite.
https://jupyterlite-terminal.vercel.app/
BSD 3-Clause "New" or "Revised" License
9 stars 2 forks source link

Open and shut down terminals #12

Open jtpio opened 5 months ago

jtpio commented 5 months ago

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 https://github.com/jupyterlite/terminal/pull/11.

ianthomas23 commented 5 months ago

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 commented 1 week ago

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 commented 1 week ago

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 https://github.com/jupyterlite/jupyterlite/pull/1514 is finished.

ianthomas23 commented 1 week ago

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 commented 1 week ago

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