discovery: Use constructor injection to simplify lifecycle

[cweitkamp]

• Use constructor injection to simplify lifecycle

Fixes #834

Signed-off-by: Christoph Weitkamp [email protected]

[kaikreuzer]

Hm, I do not understand from this comment, why these two lines couldn't be moved to the activate() method, which would seem to be the much better place to me.

[cweitkamp]

Yes, we can do that. But maybe I understood it wrong: Isn't the constructor part of the activation in this case?

[maggu2810]

As you added the @Activate annotation to the constructor, the constructor "is" the activate method.

[cweitkamp]

Thus I conclude that the @Activate annotation is not mandatory for constructor injection? If so @kaikreuzer proposal sounds right.

[kaikreuzer]

the constructor "is" the activate method

I learned something new - thanks 🤗

[maggu2810]

Sorry, you understand my comment not correctly.
My comment has been about the code that is used by @cweitkamp (so, the reviewed one).

Here my understanding of the specification:
If constructor injection is used, the constructor must be annotated with @Activate.
You can add another activation method then you need to add another method that is annotated by @Activate.

There is an example here:
https://osgi.org/specification/osgi.cmpn/7.0.0/service.component.html#service.component-reference.element

So, the preferred solution here and for the other (IIRC it has been persistence inbox) would be to use the constructor injection (activate annotated constructor) and another activate method (activate annotated method).

    @Activate
    public AutomaticInboxProcessor(final @Reference ThingTypeRegistry thingTypeRegistry,
            final @Reference ThingRegistry thingRegistry, final @Reference Inbox inbox) {
        super(ThingStatusInfoChangedEvent.TYPE);
        this.thingTypeRegistry = thingTypeRegistry;
        this.thingRegistry = thingRegistry;
        this.inbox = inbox;
    }

    @Activate
    public void activate() {
        this.thingRegistry.addRegistryChangeListener(this);
        this.inbox.addInboxListener(this);
    }

[kaikreuzer]

Hm, that is pretty confusing that the constructor requires the annotation as well, although it does not serve as the activation method... So then it would indeed be cleaner to add a dedicated activate() method for the listeners.