Propose new api for configurable data provider factories

[frallax]

What it does

Proposal to extend the public API with a new class that can (should) be extended by all data provider factories that want to provide the configuration feature.

The main goal is to provide a "pattern" for configurable data provider factories, so that they follow a similar coding style (at least for the less advanced use cases). This hopefully can help to make configurable data provider factories easier to implement and to document how to implement them.

A practical example of how to use this is provided in a separate PR for the InAndOutDataProvider in the incubator project (I will add comments below).

How to test

Tests will be added if the proposal is accepted.
For the moment, checking out this branch together with the one named similarly for the incubator project, where the InAndOutDataProvider is used, it should enable seeing how the code changes while the InAndOutDataProvider has the same functions as before.

Follow-ups

If accepted, adding tests and documentation on how to create configurable data provider factories using this common pattern.

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the instructions in this template

[frallax]

NOTES:

• as a first draft, I am pushing to 2021-06 just because I have a environment configured on that env. It will eventually be pushed to master if discussions go in the right direction
• please ignore changes to non-java files for the draft PR
• this incubator PR shows how I foresee the usage of the new API

[bhufmann]

Thanks for the contribution. It's good to have code re-use. The incubator InAndOutAnalysis and other downstream plug-ins with similar configurations can use it. I added some comments this PR.

[bhufmann]

We can't remove it just yet. It's api and needs to be deprecated first. In incubator master branch there is already a new class that uses it.

[frallax]

done

[bhufmann]

We can't remove it just yet. It's api and needs to be deprecated first. In incubator master branch there is already a new class that uses it.

[frallax]

done

[bhufmann]

you need a @since 9.6 tag

[frallax]

done

[bhufmann]

If you put this public, then you can have the configurator as a separate class and the parent data provider factory doesn't need to extend that class (see IDataProviderFactory.getAdapter(), which needs to be overriden by the factory to return other objects that itself). I tried it with the InAndOutAnalysis and it worked.

[frallax]

I have not understood what you propose here. Could you please try to rephrase or provide some code snippets? Thanks!

[bhufmann]

So, the IDataProviderFactory provides a getAdapter(class) method (

org.eclipse.tracecompass/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/dataprovider/IDataProviderFactory.java

Line 97 in a081aac

default <T> T getAdapter(Class<T> adapter) {

) that the trace server is using to get the configurator instance. The factory implementation can decide to implement itself the ITmfDataProviderConfigurator interface or overrides the getAdapter(...) method as below and return a different class that implements the ITmfDataProviderConfigurator interface. So, the getConfigurationTable() method needs to be public instead of protected to allow that the factory class to call that method.

public class MyDataProviderFactory implements IDataProviderFactory {
// ....
   private MyConfigurator fConfigurator = new MyConfigurator();

   @Override
    public <T> @Nullable T getAdapter(@Nullable Class<T> adapter) {
        if (adapter != null && adapter.equals(ITmfDataProviderConfigurator.class)) {
            return (T) fConfigurator;
        }
        return null;
    }

// ...
}

where:

public class MyConfigurator implements ITmfDataProviderConfigurator {
// ...
}

So, if MyConfigurator would extend your new AbstractTmfDataProviderConfigurator the factory class might call fConfigurator.getConfigurationTable() to get configurations per trace. Here is the change that I had to do for the getDescriptor(trace) method of InAndOutDataProviderFactory of your example :

  @Override
    public Collection<IDataProviderDescriptor> getDescriptors(ITmfTrace trace) {
        List<IDataProviderDescriptor> list = new ArrayList<>();
        list.add(DESCRIPTOR);
        for (ITmfConfiguration config : configurator.getConfigurationTable().column(trace).values()) {
            list.add(fConfigurator.getDescriptorFromConfig(config));
        }
        return list;
    }

Having said all the above, you might be just able to change the method configurator.getDescriptorFromConfig(...) and pass the trace as parameter. Then you don't need to make the getConfigurationTable() public.

[frallax]

I made the method public as you requested

[bhufmann]

Copyright header

[frallax]

done

[bhufmann]

This this class handles tmf signals, we need to register to the signal manager and also derigster when it's disposed. You can register in the constructor.

TmfSignalManager.register(this);

Probably this class should have a dispose method to deregister from the signal manager. The factory dispose() method would call then this method.

[frallax]

done