PubSub messages management

[jjoannes]

According to the examples for PubSub you have to write the following code to process incoming PubSub messages:

xmppClient.addInboundMessageListener(e -> {
    Message message = e.getMessage();
    Event event = message.getExtension(Event.class);
    if (event != null) {
        for (Item item : event.getItems()) {
            // ...
        }
    }
});

If I'm not wrong that means that first you mix in band messages with PubSub notifications but also that you have to process in a single method all messages whatever the node which sends it. I'm afraid that, when dealing with many nodes, the code becomes an ugly if/then/else tangled code. Isn't it possible to add a listener directly at the PubSubNode level instead?

[Christian Schudt]

Good idea. I will think about it and probably design it similar as in the ChatRoom class.

[jjoannes]

Great!

[Christian Schudt]

There are 5 types of PubSub notifications:

• "Items" notification: When new items are published to the node (probably the most common one).
• Subscription changes: PubSub services notifies the subscriber about subscription changes (usually done by the node owner).
• Configuration change notifications: If the node owner changes the node configuration and pubsub#notify_config == true.
• Delete notifications: If the node owner deletes the node, subscribers are notified.
• Purge notifications: If the node owner purges the node, subscribers are notified.

We could either add one method and one event to the node, covering all these five cases:

void addNotificationListener(Consumer<PubSubEvent> listener);

where PubSubEvent simply holds the event and message, so you could do: e.getEvent().getItems() or e.getEvent().isDeleted() for example.

or we could do it for every type:

void addItemsNotificationListener(Consumer<ItemsNotifcationEvent> listener);
void addConfigurationChangeListener(Consumer<ConfigurationChangeEvent> listener);
void addDeletedListener(Consumer<DeleteEvent> listener);
void addPurgedListener(Consumer<PurgeEvent> listener);
void addSubscriptionChangedListener(Consumer<SubscriptionChangeEvent> listener);

Do you have any suggestions about the better API? I am leaning towards the first approach for simplicity.

[jjoannes]

Honestly given the use cases I have to deal with (just items notification) my preference is for the second approach. From my point of view it's also cleaner (no if) and more explicit. You want to process such type of notification? Just add the appropriate listener which will be called just for the notifications you're interested in, you don't have to bother checking the type first.

I noticed that Markus Karg has often good suggestions. Maybe you could ask him?

[Christian Schudt]

@mkarg Would you like to comment on this issue?

[Markus KARG]

Both solutions have their pros and cons. I share @sco0ter 's concerns that a lot of different registration methods clutter the API. On the other hand, I feel like @jjoannes that "if" is not nice from a user's view. A technical compromise could be <T extends PubSubListener> void addNotificationListener(Consumer<T> listener, Class<T> filter...) so at registration one could provide the class for which a listener is to be notified. If several classes are of interest, multiple registrations can happen (or just many classes are provided). This allows babbler to have the listeners registered in shared collections (just like the old-school Java property support does) which speeds up things as the selection (a.k.a the "if") happens upfront. I think it would serve both views, the API provider's and the user's.

Another idea would be have a fluent reactive API, i. e. each generic event produces a CompletableStage, so a filter function at the next stage would be able define the "if" upfront at the registration of the reactive action instead of "inside" the action's code. This could look roughly like node.whenEvent().is(Class<T>... cls).then(Consumer<T> action) (in pseudo code).

HTH -Markus

[jjoannes]

Very interesting... Whatever the solution which is selected, could it be also applied to

XmppSession.addXXXListener

to reduce the number of methods at this level?

By the way I do not really like to find such methods as "addInboundPresenceListener" in XmppSession. From my (uneducated) point of view this method belongs to PresenceManager not to the core, which should be independent of the extensions. Just my 2 cents...