Updates roster encoding for MIX channel. #87

tarun018 · 2017-08-04T08:33:15Z

Change-Id: Idb9683f45734ac58daa58736fe199dc014b9352a

tfar

Looks okay for the parts that are provided. Would be great to have unit and integration tests that check that an account supports MIX and checks that on join of a MIX it's added to the roster.

tfar · 2017-08-04T10:05:03Z

Swiften/Client/Client.cpp

@@ -58,6 +59,8 @@ Client::Client(const JID& jid, const SafeString& password, NetworkFactories* net
    directedPresenceSender = new DirectedPresenceSender(stanzaChannelPresenceSender);
    discoManager = new ClientDiscoManager(getIQRouter(), directedPresenceSender, networkFactories->getCryptoProvider());

+    mixRegistry = new MIXRegistry(roster);


We should probably only allocate this if the account supports MIX.

tfar · 2017-08-04T10:06:03Z

Swiften/Client/Client.h

@@ -191,6 +196,7 @@ namespace Swift {
            PresenceOracle* presenceOracle;
            DirectedPresenceSender* directedPresenceSender;
            StanzaChannelPresenceSender* stanzaChannelPresenceSender;
+            MIXRegistry* mixRegistry;


Initialise with nullptr or better, keep as unique_ptr.

tfar · 2017-08-04T10:06:49Z

Swiften/Elements/RosterItemPayload.h

@@ -41,12 +41,16 @@ namespace Swift {
                unknownContent_ += c;
            }

+            void setChannel() { isChannel_ = true; }


This should have a bool parameter.

tfar · 2017-08-04T10:08:21Z

Swiften/MIX/MIXRegistry.h

+    class SWIFTEN_API MIXRegistry {
+        public:
+            MIXRegistry(XMPPRoster* xmppRoster);
+            virtual ~MIXRegistry();


No virtual method otherwise, so doesn't need a virtual d-tor.

tfar · 2017-08-07T10:28:45Z

Swiften/Client/Client.cpp

@@ -74,6 +75,9 @@ Client::Client(const JID& jid, const SafeString& password, NetworkFactories* net
    jingleSessionManager = new JingleSessionManager(getIQRouter());
    blockListManager = new ClientBlockListManager(getIQRouter());

+    mixSupport = false;
+    requestFeatures();


This should called when the client went online, not in the constructor.

tfar · 2017-08-07T10:29:58Z

Swiften/Client/Client.cpp

@@ -74,6 +75,9 @@ Client::Client(const JID& jid, const SafeString& password, NetworkFactories* net
    jingleSessionManager = new JingleSessionManager(getIQRouter());
    blockListManager = new ClientBlockListManager(getIQRouter());

+    mixSupport = false;


This boolean is redundant, not? If MIX is supported, mixRegistry will be initialised. If not, it will not be initialised.

I just used this as a bool to pass to requestRoster. However it is still redundant.

tfar · 2017-08-07T10:32:31Z

Swiften/MIX/MIXRegistry.h

+        public:
+            boost::signals2::signal<void (const JID& /* joinedChannel */)> onChannelJoined;
+            boost::signals2::signal<void (ErrorPayload::ref /* joinResponse */)> onChannelJoinFailed;
+            boost::signals2::signal<void (const JID& /* leavedChannel */)> onChannelLeaved;


Should be onChannelLeft.

tfar · 2017-08-07T10:47:04Z

Swiften/MIX/MIXRegistry.cpp

+    }
+}
+
+std::unordered_set<std::string> MIXRegistry::getJoinedChannels() {


If I read https://xmpp.org/extensions/xep-0369.html#usecase-roster-management correctly the roster only contains joined channels. So getChannels sounds like a better name.

Shouldn't it return JIDs rather than strings?

It should return a list of all channels in the roster.

I'll update it to return JID rather than strings. As soon as a JID is added, the handler will check if it is a channel and add it to the map. So it will always return list of channels in the roster.

tfar · 2017-08-07T10:48:21Z

Swiften/MIX/MIXRegistry.cpp

+    return results;
+}
+
+MIX::ref MIXRegistry::getMIXInstance(const JID& channelJID) {


This method should return a MIXImpl instance for the channel JID, if and only if it is joined, so it is in the roster.

tfar · 2017-08-07T10:49:44Z

Swiften/MIX/MIXRegistry.h

+            IQRouter* iqRouter_;
+            XMPPRoster* xmppRoster_;
+            bool rosterPopulated_;
+            typedef std::map<JID, MIX::ref> MIXInstanceMap;


Why this typedef? If it's really needed, it should probably be a using instead.

tfar · 2017-08-07T10:55:50Z

Swiften/MIX/MIXRegistry.h

+            XMPPRoster* xmppRoster_;
+            bool rosterPopulated_;
+            typedef std::map<JID, MIX::ref> MIXInstanceMap;
+            MIXInstanceMap entries_;


This should probably map JID to weak_ptr. The idea I'd think is:

You create your Client instance and go online with it.

Ask the MIXRegistry for the channels.

Call MIXRegistry::getMIX(channelJID) to get a shared_ptr<MIX>. MIXRegistry holds a weak_ptr to it so it can hand out the same if another part of the app asks for the same channel.

If the user doesn't need the channel info anymore, it can get rid of its shared_ptr and the MIX instance is freed.

What do you think @intosi ?

I'm not sure that works, does it? I'd expect this to be a shared_ptr, or unique_ptr, personally. I'd consider the MIXRegistry to own all the channel objects.

If you have hundreds of MIX channels in your roster you probably don't want hundreds of MIX instances allocated all the time, right?

tfar · 2017-08-07T10:57:10Z

Swiften/MIX/MIXRegistry.cpp

+
+}
+
+void MIXRegistry::joinChannel(const JID& channelJID, const std::unordered_set<std::string>& nodes) {


This should probably check if a channel with channelJID is already on the roster and log a warning instead of issuing a join.

Kev · 2017-08-07T11:05:52Z

Swiften/Client/Client.cpp

@@ -148,13 +154,27 @@ void Client::handleConnected() {
    discoManager->handleConnected();
 }

+void Client::requestFeatures() {


I'm not sure this happening automatically is consistent with the design elsewhere, where network traffic isn't generally generated without being requested. It might be sensible, but it's not plugged in anywhere else, which means it's going to end up duplicated by the current disco logic in Swift isn't it?

Yeah. Currently it would be duplicated.

Kev · 2017-08-07T11:06:26Z

Swiften/Client/Client.cpp

+void Client::handleFeaturesReceived(DiscoInfo::ref payload, ErrorPayload::ref /*error*/) {
+    if (payload && payload->hasFeature(DiscoInfo::MIXFeature)) {
+        mixSupport = true;
+        mixRegistry = std::make_unique<MIXRegistry>(getJID(), getIQRouter(), getRoster());


Always having a mixRegistry, but it being empty when not supported, probably makes more sense, doesn't it?

Yeah. Already mentioned it in #87 (comment) .

Kev · 2017-08-07T11:07:05Z

Swiften/Client/Client.h

@@ -7,6 +7,8 @@
 #pragma once

 #include <Swiften/Base/API.h>
+#include <Swiften/Elements/DiscoInfo.h>


Check the include ordering please.

Kev · 2017-08-07T11:08:54Z

Swiften/Elements/RosterItemPayload.h

@@ -41,12 +41,16 @@ namespace Swift {
                unknownContent_ += c;
            }

+            void setChannel(bool isChannel) { isChannel_ = isChannel; }
+            bool getChannel() const { return isChannel_; }


We usually name bools like this isX, rather than getX. Also, does it make sense to explicitly call it a MIXChannel, rather than Channel, to avoid ambiguity with other channels?

Kev · 2017-08-07T11:13:31Z

Swiften/MIX/MIXRegistry.cpp

+
+void MIXRegistry::handleJIDAdded(const JID& jid) {
+    if (rosterPopulated_ && xmppRoster_->isChannel(jid)) {
+        std::map<JID, MIX::ref>::iterator i = entries_.find(jid);


Why not auto?

Kev · 2017-08-07T11:15:24Z

Swiften/MIX/MIXRegistry.cpp

+    }
+}
+
+std::unordered_set<std::string> MIXRegistry::getJoinedChannels() {


Wouldn't you expect this to return a list of the channel objects, rather than their JIDs?

Depends on whether you always want to have the objects in memory even when you don't need them.

I'd have expected that, if you request the mix bits of the roster, it'd then sort it all out for you populate them and you'd then use them. Is that not the likely use case?

If you are joined to 100 MIX channels but are currently not interested in the messages as you are on mobile or so, you probably don't want to allocate 100 MIXImpl instances. This way a MIXController could ask for the MIX shared_ptr it needs for its channel which is then created for it on demand. Allocating memory for stuff that isn't really used ultimately leads to bloated applications.

Unless MIXImpls are much bigger than I think they should be, this doesn't seem like a particularly big issue - 100 MIXs worth of messages you're not interested in seems likely to be a bigger issue (which needs sorting out with protocol).
OK, I'll go along with JIDs here for the moment as it's relatively easy to change later.

Kev · 2017-08-07T11:17:38Z

Swiften/MIX/MIXRegistry.h

+
+            void joinChannel(const JID& channelJID, const std::unordered_set<std::string>& nodes);
+
+            void leaveChannel(const JID& channelJID);


Is leave being on the registry or on the object itself more sensible?

I'd say if registry is handling the join and keeping track of joined channels, leave should also be here as successfully leaving a channel depends on (onJIDRemoved).

I'm not entirely convinced, but not a hill for me to die on.

Kev · 2017-08-07T11:19:45Z

Swiften/MIX/MIXRegistry.h

+            XMPPRoster* xmppRoster_;
+            bool rosterPopulated_;
+            typedef std::map<JID, MIX::ref> MIXInstanceMap;
+            MIXInstanceMap entries_;


I'm not sure that works, does it? I'd expect this to be a shared_ptr, or unique_ptr, personally. I'd consider the MIXRegistry to own all the channel objects.

Kev · 2017-08-07T11:20:37Z

Swiften/Parser/PayloadParsers/RosterParser.cpp

@@ -52,11 +52,15 @@ void RosterParser::handleStartElement(const std::string& element, const std::str
            if (attributes.getAttribute("ask") == "subscribe") {
                currentItem_.setSubscriptionRequested();
            }
+        } else if (element == "annotate" && ns == "urn:xmpp:mix:roster:0") {
+            getPayloadInternal()->setAnnotate(true);


If this is MIX-specific, shouldn't it be obvious from the method name? setSupportsMIX or setSupportsMIXAnnotations or something?

Kev · 2017-08-07T11:23:26Z

Swiften/MIX/MIXRegistry.cpp

+}
+
+void MIXRegistry::handleJIDRemoved(const JID& jid) {
+    if (rosterPopulated_ && xmppRoster_->isChannel(jid)) {


Won't isChannel always be false at this point, because it's been removed from the roster?

tfar · 2017-08-07T14:09:36Z

@Kev suggested adding bool includeMIX = false to Client::requestRoster and do away with automatic feature discovery. Client::getMIXRegistry() should always return a registry. Feature discovery on whether an account supports MIX or not is supposed to happen by the user of Client.

Update roster encoding for MIX channel Update roster parser and serializer License: This patch is BSD-licensed, see Documentation/Licenses/BSD-simplified.txt for details. Test-Information: Added tests for joining and leaving channels via MIXRegistry, which passes. Tested on Ubuntu 16.04 LTS. Change-Id: Idb9683f45734ac58daa58736fe199dc014b9352a

tarun018 · 2017-08-09T09:31:14Z

Updated based on feedback with unit tests.

tarun018 force-pushed the MIXRegistry branch from 04b7ae2 to b93cbf0 Compare August 4, 2017 08:39

tfar requested changes Aug 4, 2017

View reviewed changes

tarun018 force-pushed the MIXRegistry branch from b93cbf0 to a9c8353 Compare August 4, 2017 18:05

tfar requested changes Aug 7, 2017

View reviewed changes

Kev requested changes Aug 7, 2017

View reviewed changes

tarun018 force-pushed the MIXRegistry branch from a9c8353 to 295b3bf Compare August 8, 2017 21:32

tarun018 force-pushed the MIXRegistry branch from 295b3bf to 3d00927 Compare August 9, 2017 09:30


		}

		void MIXRegistry::joinChannel(const JID& channelJID, const std::unordered_set<std::string>& nodes) {


		void joinChannel(const JID& channelJID, const std::unordered_set<std::string>& nodes);

		void leaveChannel(const JID& channelJID);

Updates roster encoding for MIX channel. #87

Are you sure you want to change the base?

Updates roster encoding for MIX channel. #87

Conversation

tarun018 commented Aug 4, 2017

tfar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfar commented Aug 7, 2017

tarun018 commented Aug 9, 2017