Discussion about UI XMPP Abstraction

[stefaus]

trafficstars

We have several reasons to remove XMPP / Smack references from UI packages, such as reusing functionality, making it easier to update Smack, and possibly one day supporting different transport protocols. This issue should be used to discuss how this can be achieved.

---

Reading the code I stumbled across the XMPPUtils class. For example looking at the method: https://github.com/saros-project/saros/blob/b55aad218d78217a01801150e2c21598e0fc5428/core/src/saros/net/util/XMPPUtils.java#L151

It requires a Smack Connection object that is received in both calls via https://github.com/saros-project/saros/blob/b55aad218d78217a01801150e2c21598e0fc5428/core/src/saros/net/xmpp/XMPPConnectionService.java#L320 My first thought was to replace all XMPPUtils method calls using Smack Connection objects with the Saros XMPPConnectionService object like XMPPUtils.getNickname() already does. (two of the class methods could just be set private) Which I think is pretty straight forward.

What seems unnecessary to me is that XMPPUtils even needs a Smack / Saros XMPP connection object for calls, because XMPPConnectionService is a singleton handled by PicoContainer and is set as a default service when calling getNickname (at container init) without a specified XMPPConnectionService.

https://github.com/saros-project/saros/blob/b55aad218d78217a01801150e2c21598e0fc5428/core/src/saros/net/util/XMPPUtils.java#L62-L65

There is only one occasion were the (singleton) XMPPConnectionService is additional supplied. → Saros is already just using one connection

So what do you think about the topic?

1. Is everyone Ok with handling one connection only and removing connection from the argument list (here and in other occurrences)?
2. Making XMPPUtils a non-static class and accessible via XMPPConnectionService?
3. XMPPUtils.getNickname is nearly always called with arguments (null, jid, jid.getBase()). I would suggest to extend the JID class with a getDisplayname() method providing easy access for this, which is in my opinion a more Object oriented programming approach. Looking at this following code comment, this seems controversial.
   https://github.com/saros-project/saros/blob/b55aad218d78217a01801150e2c21598e0fc5428/core/src/saros/net/xmpp/JID.java#L85-L86 The same for User, looking at UserFormatUtils this is a Utility class just for calling XMPPUtils.getNickname.

For now I would focus on some first abstractions to move XMPP stuff into the core, maybe to a point where a Smack Update is easier and later shifting the focus on a more generic network abstraction and moving to access the network layer via ConnectionHandler.

[srossbach]

A JID does not have a display / nickname. It is just an address. A contact (identified with a JID) in your roster may has a nickname.

Of course you can add more methods to the XMPPUtils class so that it does not need a connection object (which is null anyway in most cases as you already figured out).

The reason this class takes Connections and not the singleton XMPPService is basically a design choice. It offers abstraction around Smack and not some Saros components. The whole network (net package) stuff is build around like this, i.e you can just copy it to another project and start using it as there is no Saros stuff in it.

The reason getNickname does take the connection service (although it could also be the connection) is just (bad) convenience. For UI components it is not so easy to get the connection instance. To do that you have to track it or just use the XMPPConnectionService.

So does it really matter to call getNickname(xmppConnectionService, ...) or getNickname(xmppConnectionService.getConnection, ...) ?

Of course it is not straight forward and maybe it is fine to over getNickname(JID jid) and getNickname(Connection connection, JID jid) instead.

As for the Smack upgrade (I think you mean the libraries). It is just an immense time waste. You will spend so MUCH time for almost nothing. It would only be worth if they would include https://xmpp.org/extensions/attic/jep-0174-0.3.html . I doubt that will ever be happen.

[stefaus]

A JID does not have a display / nickname. It is just an address. A contact (identified with a JID) in your roster may has a nickname.

Technically yes. So how about creating a XMPPContact class extending JID and adding such methods to it? Maybe already implementing a rudimentarily generic Contact Interface?

The whole network (net package) stuff is build around like this, i.e you can just copy it to another project and start using it as there is no Saros stuff in it.

Ok, but I don't get why you wouldn't use saros.net.xmpp.XMPPConnectionService from the net package instead of org.jivesoftware.smack.Connection. This wouldn't introduce dependencies to other packages.

So does it really matter to call getNickname(xmppConnectionService, ...) or getNickname(xmppConnectionService.getConnection, ...) ?

Then you got me wrong, I think it matters to just call jid.getDisplayname() / xmppContact.getDisplayname() instead of doing the same thing again and again https://github.com/saros-project/saros/blob/b55aad218d78217a01801150e2c21598e0fc5428/eclipse/src/saros/ui/eventhandler/NegotiationHandler.java#L312-L314 https://github.com/saros-project/saros/blob/b55aad218d78217a01801150e2c21598e0fc5428/eclipse/src/saros/ui/wizards/JoinSessionWizard.java#L260-L262

As for the Smack upgrade (I think you mean the libraries). It is just an immense time waste. You will spend so MUCH time for almost nothing.

Looking at the Changelog I think we will have to face it,

• [SMACK-856] - Smack fails under JDK 11 because com.sun.jndi.dns.DnsContextFactory is not inaccessible
• [SMACK-739] - Smack starts SASL step without TLS in case STARTTLS is stripped even if SecurityMode.Required is used
• [SMACK-410] - Any valid SSL server certificate can be used to perform a man-in-the-middle attack

Security problems are in my Opinion worse enough, incompatibility with newer JDKs become a show stopper. Even if this mentioned problem could belong to newer smack versions, the same can happen to the current.

[srossbach]

I thought they got rid of accessing internal Oracle packages as they are not available in certain JDKs ...

The time waste was a reference to features that we do not even use.

As for the getNickname stuff. Are you fine with introducing the 2 methods getNickname(JID jid) and getNickname(Connection connection, JID jid) and so replace the existing one ?

[stefaus]

So does it really matter to call getNickname(xmppConnectionService, ...) or getNickname(xmppConnectionService.getConnection, ...) ?

I would like to add a strong yes here, because getConnection leads to an escaping internal (Connection object from Smack). And I would propose to make this at least package private.

As for the getNickname stuff. Are you fine with introducing the 2 methods getNickname(JID jid) and getNickname(Connection connection, JID jid) and so replace the existing one ?

So like said, I think Connection shouldn't escape. Too illustrate my aversion against utility classes a bit: It's difficult to follow the Open / Closed Principle If we get Nicknames (outside the net package) via XMPPUtils.getNickname(...) and would like to add a new transport like matrix. We would add a new Util class with MatrixUtils.getNickname(...) having different args, making it difficult to have a general Interface to get a nickname for a contact. This would probably again lead to leaking information about the transport impl and how to work with it.

Avoiding utility classes we could have a xmppContact.getNickname() (noargs) and a matrixContact.getNickname() hidden behind a Interface Contact.getNickname().

So maybe we are focusing on different things, but even if we stick with Utility classes for now, I think they shouldn't need Smack objects when called outside the package.

[srossbach]

It does matter if you leak a Connection object or the XMPPService. They both offer a "great amount" of methods to **** up everything aka "with great power comes great care ...)

As for the nickname stuff. I added functionality to choose a nickname during a session. It got removed because it did not match the XMPP world. Taken your example lets have a simple TCP4/6Utils.getNickname(...). What should it return ? I guess null.

So before any further discussion: Should the Aliases in a Session represent their XYZ connection nicknames ?

To be more specific. Lets assume a Saros Server from a far future hosts a Session. The first user is connected using a plain IP address. The second uses XMPP, the third Skype, the next one Discord and the last one comes from the past and uses ICQ. There are two options, either let the user chose their nickname or let the server retrieve the nicknames and let it broadcast. There is no way client A with protocol X can retrieve the nickname of user B using protocol Z.

[stefaus]

It does matter if you leak a Connection object or the XMPPService.

Could you clearify this? I'm not sure what your message is.

I added functionality to choose a nickname during a session.

A different nickname than the xmpp nickname?

Taken your example lets have a simple TCP4/6Utils.getNickname(...). What should it return ? I guess null.

Probably the hostname or if available username@hostname. If a nickname is unavailable getNickname() would return a null or Optional. And alternatively getDisplayname() could directly provide a alternative like the technical identifier.

So before any further discussion: Should the Aliases in a Session represent their XYZ connection nicknames ?

I think the nickname should be the one from the transport, like when a person uses an other chat client they should see the same names for their contacts.

let the server retrieve the nicknames and let it broadcast

Yes something like this would probably be needed. I think supporting multiple transports is one thing and cross transport connections is a completely other one we don't have to focus now.

[srossbach]

I am talking about this commit.

https://github.com/saros-project/saros/commit/fda6497260319c7dedbf9973d03c993578764771

Problem was that the session nickname was not always the same as the XMPP nickname which is no surprise because the nickname is set on two sides.

[stefaus]

Ok, but this patch removed a session nickname and that's not really the topic.

I think the User object should have a getDisplayname() calling the getDisplayname() of a jid / XMPPContact object and not store it. So it stays automatically in sync if the Contact gets renamed. Jid / XMPPContact .getDisplayname() should show a nickname or as fallback the technical name.
This can lead to Alice seeing Carl with self chosen Carl (from ACME Corp.) and Bob seeing Carl with his jid or just Carl? Do you see a problem in this?

Looking at this commit f58baf4b77409264fd1a12d2d24f3771625de4f5, I think it's not a good solution. Even if we argue that all Dialog messages created in the core should be created in the UI part (creating work I don't think is worth it) then this again violates the Open Closed Principle. UserFormatUtils needs a change for every transport.

[srossbach]

The User object should be a simple POJO. We already must have a XStream converter for that class. However good luck writing Unit tests when a user needs a Connection object (and long time ago a Session object). But I have problems if we offer a getDisplayName method that really goes through network stuff to retrieve a simple stupid string.

I do not get the discussion at all. We have 2 places where we represent the nickname. In some tooltips from some annotations (which never get updated anyway) and in the Contact List which is XMPP specific. (and some 1 "timers" in the OSN/ISN, OPN/IPN)

And now we have the session overview. What should we display as nickname ? If user A uses Skype and User B uses XMPP you cannot retrieve the nicknames.

[stefaus]

The User object should be a simple POJO. We already must have a XStream converter for that class.

Ok, so we disagree on this. What is your idea to eliminate the need for an XStream converter?

when a user needs a Connection object

Why do you think we need a Connection object here?

But I have problems if we offer a getDisplayName method that really goes through network stuff to retrieve a simple stupid string.

I think hiding the implementation how to get a Displayname makes a lot easier. How would you extend the current code to be capable handling multiple transports?

And now we have the session overview. What should we display as nickname ? If user A uses Skype and User B uses XMPP you cannot retrieve the nicknames.

I think cross transport support should not be in the focus now, but as you already asking so maybe we would create a contact class for contacts provided by the server like ServerContact which would implement a getDisplayname() returning the infos provided by the server. Having a User object (referencing a contact) you could get the nickname via getDisplayname() not knowing how it was provided.

[srossbach]

You simply cannot eliminate the converter because it is necessary in this setup (we do not want to introduce some data holder objects again for each activity). Feel free to implement your getDisplayName method but just as one last thought on this topic. Maybe it is not the worst idea to implement this as an event driven concept, i.e add a component that listenes to XMPP roster changes and fire a session event. Maybe this works fine for XMPP but I do not know how this will payout once (in a far future) we have more than one transport protocol.