MUC addMessageListener() and addParticipantListener() conflict

Smack Smack Dev
1

Hello,

I looked for an answer to this problem I encountered but couldn’t find one. If you can help me with his here or direct me to a solution in some other discussion I would appreciate it.

I wanted to add MUC support to my android application using the asmack library. However, when I tried to add my custom class, implementing PacketListener, as both the MessageListener and ParticipantLisener in the MultiUserChat class this didn’t work as only the last listener worked.

I looked into the MultiUserChat.java code and found where the problem was:

messageFilter =

new AndFilter(

new FromMatchesFilter(room),

new MessageTypeFilter(Message.Type.groupchat));

messageFilter = new AndFilter(messageFilter, new PacketFilter() {

public boolean accept(Packet packet) {

Message msg = (Message) packet;

return msg.getBody() != null;

}

});

public void addMessageListener(PacketListener listener) {

connection.addPacketListener(listener, messageFilter);

connectionListeners.add(listener);

}

presenceFilter =

new AndFilter(new FromMatchesFilter(room), new PacketTypeFilter(Presence.class));

public void addParticipantListener(PacketListener listener) {

connection.addPacketListener(listener, presenceFilter);

connectionListeners.add(listener);

}

First of all, the messageFilter object is created once so the first creation is useless. Then, the listener problem. The listener is added both to the local list connectionListeners and to the main connection PacketListener list. However, the former is never used in the MUC class so I guess that the packets are only forwarded thanks to the main connection PacketListener list to my custom class. The problem is that the packetFilter for this listener is overwritten if I’m using only one class in both cases. A solution is to use two different classes - one for MessageListener and another one for ParticipantListener but I would like to avoid it and do it all in a single class. The current implementation doesn’t allow me to do that.

2

No, the first creation is not useless. It get’s immediately used for the new AndFilter that overwrites the seconds declaration of messageFilter in line 2115. Not sure why it’s done this way, especially since the AndFilter accepts infinite PacketFilters as arguments. The current code is just confusing.

The MUC local connectionListeners were introduced because of SMACK-48 in this changeset. They** get used** in the finalize() method of MultiUserChat.

PacketListener list to my custom class. The problem is that the packetFilter for this listener is overwritten if I’m using only one class in both cases. A solution is to use two different classes - one for MessageListener and another one for ParticipantListener but I would like to avoid it and do it all in a single class. The current implementation doesn’t allow me to do that.

Not sure if I get your problem. Did you try to use an OrFilter? But anyway, I think that using two different classes is the superior design and way more cleaner. How are the actions that both listeners do related?

I don’t think that this is a design bug of smack.

3

No, the first creation is not useless. It get’s immediately used for the new AndFilter that overwrites the seconds declaration of messageFilter in line 2115. Not sure why it’s done this way, especially since the AndFilter accepts infinite PacketFilters as arguments. The current code is just confusing.
Sure, I missed that, sorry.

They** get used** in the finalize() method of MultiUserChat. The MUC local connectionListeners were introduced because of SMACK-48 in this
The only way they get used in the finalize() method is removing them. What I meant is that they do not fire any events.

Not sure if I get your problem. Did you try to use an OrFilter?
It’s not my who’s applying the packet filter. This is done in the addXXXListener() in the MUC class. the addMessageListener() adds the messageFilter and the addParticipantListener() adds the presenceFilter. I’ll describe what I’m doing step by step - I understand this can be confusing In my OpenfireChat.java class I have the following statements:

mMUCCurrent = new MultiUserChat(mConnection, bld.toString());

mMUCCurrent.addParticipantListener(this); // Line A

mMUCCurrent.addMessageListener(this); // Line B

The class implements the PacketListener interface and overrides the processPacket() method. What I wanted to do is the following:

@Override

public void processPacket(Packet packet) {

if (packet instanceof Message) {

// do something

} else if (packet instanceof Presence) {

// do something else

}

}

I wanted to do this in one class, without creating any additional ones. What I guess happens now is that in Line A in the snippet above my class is added to the listeners list with the packet filter responsible for Presence packets. Then, in Line B, the same object is given as the parameter. However, this time it goes together with the Message packet filter (more specifically, the ‘groupchat’ filter). This means that my processPacket() method will never get any Presence packets. If I swap lines A and B I only get Presence packets and no Message packets.

But anyway, I think that using two different classes is the superior design and way more cleaner. How are the actions that both listeners do related?
I think using one class the way I described above shouldn’t be discouraged. I can live with two separate classes that are within my main class since my custom class is just a helper class with its own set of listeners and whenever I get a group Message or a Presence packet I fire the listeners’ methods but to me that’s making the code more complicated.

I hope I made myself more clear this time

4

Thanks, now I know what you are trying to do.

The problem is in Connection.java:594

The recvListeners Map uses the PacketListener as Key. For now your only choice is to use two different objects for the packet listeners, which will be the case if you use inner classes as you already mentioned.

But in fact, this seems like a little design flaw in smack. I can’t find a reason why they used a Map that maps from PacketListener to ListenerWrapper. I would suggest another data structure:

Map<PacketFilter, List>

**
**

This way, smack could reuse the same PacketFilter for different PacketListeners (only if they are the same object) and your problem would pop up.

5

Although I don’t have the code in front of me, I agree that it looks like a small design flaw.

Logged as SMACK-380. Looks like your suggestion might make it an easy fix.