[jitsi-dev] MucRoomJabberImpl fix proposition


#1

Hi there,

I recently started to analyse the Jitsi XMPP Implementation and I found this strange note in MucRoomJabberImpl class (Chat room support implementation) :

"

// We send only the content because it doesn't work if we send the
// Message object.
"
In the sendMessage(Message message) method, line 945, Jitsi 2.8.0.
I didn't find any entry about this issue in the mailing list.

The content of this method is incoherent :

1. It constructs a Smack message called msg with no parameter

2. It applies message content into msg

3. Then it calls the sendMessage(String msg) of the Smack MultiUserChat Object with the content of message as parameter
The Smack Message instance msg is never used.

Here is my proposition of a fix which seems to work.

BEFORE
public void sendMessage(Message message)
        throws OperationFailedException
    {
         try
         {
             assertConnected();

             org.jivesoftware.smack.packet.Message msg =
                new org.jivesoftware.smack.packet.Message();

             msg.setBody(message.getContent());
             //msg.addExtension(new Version());

             MessageEventManager.
                 addNotificationsRequests(msg, true, false, false, true);

             // We send only the content because it doesn't work if we send the
             // Message object.
             multiUserChat.sendMessage(message.getContent());
         }
         catch (XMPPException ex)
         {
             logger.error("Failed to send message " + message, ex);
             throw new OperationFailedException(
                 "Failed to send message " + message
                 , OperationFailedException.GENERAL_ERROR
                 , ex);
         }
    }

AFTER
public void sendMessage(Message message)
        throws OperationFailedException
    {
         try
         {
             assertConnected();

             // Construct a ChatRoom designed Smack message
org.jivesoftware.smack.packet.Message msg = new org.jivesoftware.smack.packet.Message(multiUserChat.getRoom(), org.jivesoftware.smack.packet.Message.Type.groupchat);

             msg.setBody(message.getContent());
             //msg.addExtension(new Version());

             MessageEventManager.
                 addNotificationsRequests(msg, true, false, false, true);

             multiUserChat.sendMessage(msg);
         }
         catch (XMPPException ex)
         {
             logger.error("Failed to send message " + message, ex);
             throw new OperationFailedException(
                 "Failed to send message " + message
                 , OperationFailedException.GENERAL_ERROR
                 , ex);
         }
    }

What it does now :

1. Construct a new Smack Message msg with parameter for a MucRoom message

2. applies message content into msg

3. uses that Smack MultiUserChat Object sendMessage(Message method) with msg as parameter

Hope it helps. Sry if that was already known.

···

_______________________________________________________________________
[Email_CBE.gif]Pierric Willemet
Ingénieur Logiciel | Division Aérospatiale et Défense

Capgemini France | Rennes
Tel.: +33 2 99 28 06 32 - Mob.: + 33 6 30 18 12 43
www.capgemini.com<http://www.capgemini.com/>

People matter, results count.
_______________________________________________________________________
Connect with Capgemini:
[cid:image002.gif@01CF9484.23D158F0]<http://www.capgemini.com/insights-and-resources/blogs>[cid:image003.gif@01CF9484.23D158F0]<https://twitter.com/#!/capgeminifrance>[cid:image004.gif@01CF9484.23D158F0]<http://www.facebook.com/#!/capgeminifrance>[cid:image005.gif@01CF9484.23D158F0]<http://www.linkedin.com/groups/Capgemini-France-3849095>[cid:image006.gif@01CF9484.23D158F0]<http://www.slideshare.net/capgemini>[cid:image007.gif@01CF9484.23D158F0]<http://www.youtube.com/capgeminimedia>

Capgemini is a trading name used by the Capgemini Group of companies which includes Capgemini Technology Services SAS,
a company registered in France (RCS : 479 766 842 - APE 6202 A NANTERRE)
whose registered office is at 5/7, rue Frédéric Clavel - 92287 Suresnes cedex


#2

Hi,

Thank you for reviewing the code and sharing your findings!

Hi there,

I recently started to analyse the Jitsi XMPP Implementation and I found
this strange note in MucRoomJabberImpl class (Chat room support
implementation) :

/// We send only the content because it doesn't work if we send the
// Message object./

In the sendMessage(Message message) method, line 945, Jitsi 2.8.0.

I didn’t find any entry about this issue in the mailing list.

The content of this method is incoherent :

1.It constructs a Smack message called msg with no parameter

2.It applies message content into msg

3.Then it calls the sendMessage(String msg) of the Smack MultiUserChat
Object with the content of message as parameter

The Smack Message instance msg is never used.

Here is my proposition of a fix which seems to work.

Just to clarify: does this fix a problem that you observed, or is it just about improving the code?

[snip]

What it does now :

1.Construct a new Smack Message msg with parameter for a MucRoom message

2.applies message content into msg

3.uses that Smack MultiUserChat Object sendMessage(Message method) with
msg as parameter

How about just removing the Smack Message creating? We'll have less code, and no actual changes (which may or may not brake things).

Would you like to submit a pull request (or send a patch we can apply) with this fix?

Regards,
Boris

···

On 04/12/15 03:55, WILLEMET, Pierric wrote: