feat(trace-viewer): refactor trace viewer

[ruifigueira]

Refactoring before introducing embedded mode.

• TraceViewer renamed to SpawnTraceViewer and moved into testModel (see feature(trace-viewer): embedded mode support PoC #483 (comment))
• TestModel is now disposable, to ensure all resources are properly released when extension is disposed

[pavelfeldman]

Love the test coverage!

[pavelfeldman]

This is still quite confusing, sorry. We should be able to handle this use case without additional complexity. All the objects we operate have nested lifetimes:

• TestModelCollection outlives models that it contains
• TestModels contain respective trace viewers
• There is a way to clear the collection that destroys all the models due to containment
• There is a way to reset the model while maintaining it alive.

I'm not sure why we can't keep things straightforward, but maybe it is the view aspect that makes us keep just one trace viewer open that stands in the way?

[ruifigueira]

Current TestModelCollection in main branch is not included in the extensions disposables, and I needed to add it to ensure it was properly released in case the extension is deactivated. But TestModelCollection dispose in main branch doesn't reset its models, only disposes the model.onUpdate event handlers (it uses the DisposableBase implementation).

So, to make this step clear, I creaed another PR just trying to fix that: #511.

But then there's the difficulty of adding another disposables that should not be disposed on reset / clear , like event handlers. If I just add them to this._disposables, those handlers will be disposed on TestModelCollection.clear and therefore won't receive the correponding events afterwards.

An option for TestModelCollection is to add event handler disposables to ExtensionContext.subscption, since its lifetime is the same as the extension, but that doesn't seem right to me, because it should be TestModelCollection's responsability to actually dispose all its internal state.

[ruifigueira]

Sometimes it's better to take one step back: if we move trace viewer instances back into the Extension class, we can avoid registering event handlers in TestModelCollection and TestModel, so basically I'm rolling back changes in those classes.

So:

• TraceViewer implementations become shallow objects once again, with the same lifetime as the extension (right now, it's only SpawnTraceViewer, but the same logic will be applied to EmbeddedTraceViewer in the next phase)
• They keep a reference to TestModelCollection so that they can get the current configuration / server URL
• All event handling that can impact trace viewers (models.onUpdated / showTrace.onChange, embeddedTraceViewer.onChange) are handled in extensions

Let me know if this approach is better

[pavelfeldman]

It is hard for me to give a good advice without doing the actual work. My naive thought process is as follows:

• User selects test item in the tree
• Extension (controller) handles this change and makes a decision that trace for the test result should be shown
• Extension creates (or reuses) trace viewer for this test via the test model.
• Extension takes owership of the trace viewer and listens for the model to be disabled and/or removed, destroys dangling trace viewer when needed.
• Extension opens trace in the created trace viewer

[ruifigueira]

Ok my latest commit no longer makes sense with the changes you introduced. I'll drop it.

[pavelfeldman]

I don't think I changed anything fundamentally, but you would know better what applies and what does not. Please let me know which patch I should be reviewing!

[ruifigueira]

you can review the current one, I already rolled back the "step back" solution.