Implemented external API

[ngondalia]

This commit provides an external API that can be used by other extensions to communicate with the base trace extension. The current API provides a way to retrieve the active experiment as well as fire events and register callbacks. This is the initial API and will be further expanded in future commits.

Fixes #140

Signed-off-by: Neel Gondalia [email protected]

[bhufmann]

Thanks for the contribution. I was able to use the external API in respect with getting the active trace.

I'm not clear on how to use the signal interface. @ngondalia, could you please walk me through what needs to implemented from an another extension using a simple example?

[ngondalia]

yeah definitely! Take a look at the snippet below @bhufmann :

const ext = vscode.extensions.getExtension("tracecompass-community.vscode-trace-extension");
const importedApi = ext.exports;

// The following gets the keys of event emitters that are registered to TraceExtensionEventManager
importedApi.getEventEmitterIds();

//The only registered event emitter right now is the "traceExplorer.openedTracesView.register_callback"

importedApi.fireEventEmitter("traceExplorer.openedTracesView.register_callback",
	{
		signal: "experimentSelected", 
		callback: (data: any) => {
       			vscode.window.showInformationMessage("Event Fired: exp selected");
		}
	}
});

//Now the provided callback every time the "experimentSelected" is propagated in the Opened traces view

[bhufmann]

I was able to test the signal call back interface. I had to modify your example a bit to make it work:

importedApi.fireEventEmitter("traceExplorer.openedTracesView.register_callback", 
	{
		signal: "experimentSelected", 
		callback: (data: any) => {
       			vscode.window.showInformationMessage("Event Fired: exp selected");
		}
	});

I tried to add a similar callback mechanism to available views webview provider (e.g. for output-added signal).

I noticed when the webviews are minimised the callback is not called anymore because the webviews are disposed. The callbacks are only called when the webview is visible. For the experimentSelected signal you can reproduce it when have at least 2 traces open, minimise the opened traces view and then switch traces panels. Not sure if this is an issue.

When the callback is called form when the message is received, it will forward the data structure. For experimentSelected it will contain the data about the Experiment. The actual Experiment class couldn't be used in the signal directly, because the start/end time are of type bitint which cannot be serialized to JSON by default. So, what the vscode-trace-extension does is to use JSONbigInt to serialize it to a string before sending and on the receiving side deserializes the string to the Experiment type re-creating start/end time as bigint. No, with this callback mechnism the receiving side needs to do the same thing. Right now there is no generic utility to be re-used. Maybe eclipse-cdt-cloud/tsp-typescript-client#56 could help, but the project hasn't decided to go with it.

Another comment is, that with this callback feature, the messages and their data structure become also API so that adopter extension can rely on.

We should add some documentation on the external API and how to use it. Your example would be good candidate to show how to use it.

I wrote quite a bit text as reply. Could you please look at the comments and let me know what you think and how we should proceed.

Thanks
Bernd

[ngondalia]

I noticed when the webviews are minimised the callback is not called anymore because the webviews are disposed. The callbacks are only called when the webview is visible. For the experimentSelected signal you can reproduce it when have at least 2 traces open, minimise the opened traces view and then switch traces panels. Not sure if this is an issue.

Yeah this is expected because the only place from which the "register_callback" event is registered with the traceExtensionEventManager, is TraceExplorerOpenedTracesViewProvider. But technically, you can do the same from all the webviews or for the extension itself (register a "register_callback" event with the traceExtensionManager).

[ngondalia]

When the callback is called form when the message is received, it will forward the data structure. For experimentSelected it will contain the data about the Experiment. The actual Experiment class couldn't be used in the signal directly, because the start/end time are of type bitint which cannot be serialized to JSON by default. So, what the vscode-trace-extension does is to use JSONbigInt to serialize it to a string before sending and on the receiving side deserializes the string to the Experiment type re-creating start/end time as bigint. No, with this callback mechnism the receiving side needs to do the same thing. Right now there is no generic utility to be re-used. Maybe eclipse-cdt-cloud/tsp-typescript-client#56 could help, but the project hasn't decided to go with it.

I think having a generic utility definitely should be the way going forward. Currently however, the calling extension has to handle this similarly to how the vscode-trace-extension does it.

[ngondalia]

Another comment is, that with this callback feature, the messages and their data structure become also API so that adopter extension can rely on.

We should add some documentation on the external API and how to use it. Your example would be good candidate to show how to use it.

Yeah I agree. I will add more helpers in the API to create these data structures. I will also add some detailed documentation in the following commit. Thanks!

[bhufmann]

Thanks for the interesting discussions to work out the external API. I'll needs some time to understand the recent comments and code. I'll follow-up with you soon.

[bhufmann]

I just wanted to let you know that I haven't been able to give my feedback and I'm still looking into this. Thank you for your patience in this matter.

[bhufmann]

Thanks again for this initiative, and also feedback of all participants. It's not an easy topic to come-up with an API that is somewhat future-proof and easy to maintain.

Ok, it took some time to think about this and to use the proposed API. I'm able to achieve to get a callback registered for a given signal (experimentSelectedSignal). I was also able to add additional hook inside the external API to trigger the start of the trace server. I was also able to register/deregister a listener using (onEventRegistered, onEventDisposed).

However, I have some difficulties in understanding the logic and wonder if it could be a bit simplified.

Firstly, a third-party extension has to call fireEvent() to register a callback for a given signal, that is called when the openTraces view receives a message. As an extender of the vscode-trace-extension I would expect to use the vscode EventEmitter classes to do the same, i.e. register a listener that is called when a message is received from vscode in a given webview. The registration API for listener could be per webview by passing the webview ID. The listener receives all the messages and needs to have it's own switch/case-statement to identify the message of interest.

Since the webviews can be disposed and recreated, there needs to be an API that notifies the extending extension about that event to be able to do some action on it.

To Colin's alternative suggestion to expose the webview or a class that can get the webview, is also an alternative to the proposal. Then extending extension could subscribe to the messages themselves and have listeners to the onDidDisposed(). This would also provide the possibility to extend the vscode-trace-extension in respect to custom message handlers.

Providing an API for the start/stop mechanism would be then separate I think and it won't be possible to use the custom message handlers as far as I can see. But still we can achieve that.

Secondly, I noticed when registering a listener to onEventRegistered(), the initial one is missed because the open traces webview do the first fireEvent() during its initialization and at that time the extending extension is not available.

WDYT?

[ngondalia]

Thanks for all of your feedback! It is tricky to design this particular API to be generic, yet simple and easy to maintain. I think for now, we can just expose the webviews like Colin suggested. Having webviews exposed gets rid of the necessity of having a generic signal handler. Also it greatly reduces the complexity of the API. For instances where we don't rely on webviews (i.e. start/stop server), we can just create specific events that the API provides a way to attach listeners to. We can address that in #159.

I will push a new patch soon.

[colin-grant-work]

I think the overall style of this new API makes it a lot easier to follow and makes it a lot clearer what the extension expects consumers to be interested in and what consumers can expect the extension to do. 👍

[colin-grant-work]

I'll leave final approval to @bhufmann, as he has more of a user / adopter's perspective in mind than I do.

[bhufmann]

Thanks for updating the proposed external API. It's much clearer to understand and to use. I have some comments though.

It would be good to have some automated test for the API.

[ngondalia]

Thanks for updating the proposed external API. It's much clearer to understand and to use. I have some comments though.

It would be good to have some automated test for the API.

I have updated the README in the latest patch with detailed instructions. The tests i can cover under a separate PR, as non UI tests will require additional configuration to setup.

[bhufmann]

It looks good to me now. I'll request a second committer review before merging it.

[marco-miller]

Left are only these minor comments for me; thanks!

[marco-miller]

LGTM too now, thanks again for this contrib.