Connections pane integration with the variables pane

[dfalbel]

Intent

This PR integrates the variables pane with the connections pane. When variables are assigned and they are known to the connections pane we display a small icon, that triggers the registration of that connection with the connections pane.
Addresses #2503

Approach

Describe the approach taken and the tradeoffs involved if non-obvious; add an overview of the solution if it's complicated.

1. Added a new inspector that catches supported connections and enables the view buttom for them:

positron/extensions/positron-python/pythonFiles/positron/positron_ipykernel/inspectors.py

Lines 902 to 909 in f233f3e

	class ConnectionInspector(ObjectInspector):
	CLASS_QNAME = ["sqlite3.Connection", "sqlalchemy.engine.base.Engine"]
	def has_viewer(self) -> bool:
	return True
	def get_kind(self) -> str:
	return "connection"

2. Made the variables pane preview handling depend on the type of object, and for supported values we will
   enable register the object with the connections service:

positron/extensions/positron-python/pythonFiles/positron/positron_ipykernel/variables.py

Lines 519 to 537 in f233f3e

	def _perform_view_action(self, path: List[str]) -> None:
	"""
	Performs the view action depending of the variable type.
	"""
	if path is None:
	return
	is_known, value = self._find_var(path)
	if not is_known:
	return self._send_error(
	JsonRpcErrorCode.INVALID_PARAMS,
	f"Cannot find variable at '{path}' to view",
	)
	if self.kernel.connections_service.object_is_supported(value):
	self._open_connections_pane(path, value)
	else:
	self._open_data_explorer(path, value)

3. Add listeners to variable updates/deletions in the variables service allowing the connections service to handle those events, if required.

positron/extensions/positron-python/pythonFiles/positron/positron_ipykernel/variables.py

Lines 157 to 169 in f233f3e

	for name in removed:
	if exp_service.variable_has_active_explorers(name):
	exp_service.handle_variable_deleted(name)
	if con_service.variable_has_active_connection(name):
	con_service.handle_variable_deleted(name)
	for name, value in assigned.items():
	if exp_service.variable_has_active_explorers(name):
	exp_service.handle_variable_updated(name, value)
	if con_service.variable_has_active_connection(name):
	con_service.handle_variable_updated(name, value)

QA Notes

Add additional information for QA on how to validate the change, paying special attention to the level of risk, adjacent areas that could be affected by the change, and any important contextual information not present in the linked issues.

A small video below showing different scenarios that we expect this PR to work with:

Screen.Recording.2024-04-04.at.10.59.43.mov

[isabelizimm]

Some random teeny nits, but overall, this works really nicely for Python 3.10+! It looks like sqlite3 Connections are not registering on Python 3.9 and lower. I took a look at the different versions (this is the diff of 3.9 and 3.10), but wasn't quite sure where to look. There's not much actual Python code there, so it's probably somewhere on the C side, under Modules/_sqlite 😅

One more random thing: when users click on the DB icon in the Variables pane, should it bring focus to the Connections pane ?

[isabelizimm]

I might be missing something, but is essentially the same as lines 232-238?

[dfalbel]

This is very similar, indeed. The only reason why it's here, is to make sure, the variables pane is in sync with what's supported here. Otherwise, it's easy to break the variables pane, but keep unit tests passing.

[dfalbel]

One more random thing: when users click on the DB icon in the Variables pane, should it bring focus to the Connections pane ?

Yes, I think this makes sense! I'll do that!
Edit: added in ae36eab

[isabelizimm]

This looks good for the supported versions of Python! It also feels much faster than before ☄️

One small thing-- if you have a Connection open and then shut down the runtime (without disconnecting), you'll get this in the logs:

[Python]   File "/Users/isabelzimmerman/code/positron/extensions/positron-python/python_files/positron/positron_ipykernel/positron_ipkernel.py", line 374, in do_shutdown
[Python]     self.connections_service.shutdown()
[Python]   File "/Users/isabelzimmerman/code/positron/extensions/positron-python/python_files/positron/positron_ipykernel/connections.py", line 346, in shutdown
[Python]     for comm_id in self.comms.keys():
[Python] RuntimeError: dictionary changed size during iteration

I think if you change that line to list(self.comms.keys()) it should be good!

[jmcphers]

Nice!

One thing that isn't clear to me is why we need the createEventEmitter bits. It looks like they are duplicating the codegen'ed event onDidFocus. If you want to expose that event in ConnectionsComm, you can do so just by creating a public-facing Event and then assigning it to the wrapped onDidFocus from the generated class.

[jmcphers]

Should this be commented out? If it causes trouble for us to clean up the emitter when disposed, then who takes ownership of the emitter for cleanup?

[jmcphers]

Nit: Should document return value if specifying @returns

[dfalbel]

My goal was to make ConnectionsComms.ts almost an exact copy of the codegenerated file that lives in src/vs/workbench/services/languageRuntime/common/positronConnectionsComm.ts so it's easier to update when we modify the RPC contract.

positron/src/vs/workbench/services/languageRuntime/common/positronBaseComm.ts

Lines 156 to 168 in cd0b655

	/**
	* Create a new event emitter.
	* @param name The name of the event, as a JSON-RPC method name.
	* @param properties The names of the properties in the event payload; used
	* to convert positional parameters to named parameters.
	* @returns
	*/
	protected createEventEmitter<T>(name: string, properties: string[]): Event<T> {
	const emitter = new PositronCommEmitter<T>(name, properties);
	this._emitters.set(name, emitter);
	this._register(emitter);
	return emitter.event;
	}

AFAICT from extensions, the only way to listen to backend events is to listen to this event:

positron/extensions/positron-connections/src/comms/BaseComm.ts

Line 215 in cd0b655

const event = this.clientInstance.onDidSendEvent((event: any) => {

So I think I could instead of using super().createEventEmmiter() I could make it a simpler wrapper around clientInstance.onDidSendEvent but I think the code complexity would become similar if we wanted to make disposable events, etc.

Does it make sense?

[jmcphers]

My goal was to make ConnectionsComms.ts almost an exact copy of the codegenerated file that lives in src/vs/workbench/services/languageRuntime/common/positronConnectionsComm.ts

Oh, thanks, that makes it clearer. Should make it easier to reabsorb into core at some point.