[jitsi-dev] MucRoomJabberImpl fix proposition


#1

Hi,

Here are my answers to your previous message

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

···

A : The message is sent in the both cases. It is just code improving.

Q : "How about just removing the Smack Message creating? We'll have less code, and no actual changes (which may or may not brake things)."
A : That would be a bad fix in my opinion. The point of uses the Message object from Smack Package is that you can add PacketExtension to it in order to support various XEP of the XMPP protocol.

Q : Would you like to submit a pull request (or send a patch we can apply) with this fix?
A : I first try to figure out some functionalities before I do the pull request.

May you can help me with this. I am actually searching for best practice to insert PacketExtension in XMPP messages from plugin.
The OperationSet BasicInstantMessaging does not provide any way to add any extra information with the message beside "Content", "Subject", "From" and "To".
Doesn't Jitsi provide anyway to do this ?
(May I should open a new thread for this issue)

Regards,
Pierric

-------------------------------------------------------------
Hi,

Thank you for reviewing the code and sharing your findings!

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

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

This message contains information that may be privileged or confidential and is the property of the Capgemini Group. It is intended only for the person to whom it is addressed. If you are not the intended recipient, you are not authorized to read, print, retain, copy, disseminate, distribute, or use this message or any part thereof. If you receive this message in error, please notify the sender immediately and delete all copies of this message.


#2

Hi Pierric,

I can't comment on all your remarks, but I'll comment on one
specifically. See below.

Hi,

[...]

May you can help me with this. I am actually searching for best
practice to insert PacketExtension in XMPP messages from plugin.

The OperationSet BasicInstantMessaging does not provide any way to add
any extra information with the message beside “Content”, “Subject”,
“From” and “To”.

Doesn’t Jitsi provide anyway to do this ?

No, not out of the box, as the operation set BasicInstantMessaging is
only composed for the functionality that is common to "all" protocols.

This is very specific to the XMPP protocol implementation for basic
instant messaging support. It makes sense, in this case, to check if the
implementing class is specifically the XMPP protocol implementation and
then cast it to the concrete type. At that point, you have the concrete
type and thus all the implementation details available that are offered.
Including those not available in the interface definition. (Of course
you should also gracefully handle cases where the type is different.)

Kind regards,
Danny

···

On 09-12-15 16:46, WILLEMET, Pierric wrote:

(May I should open a new thread for this issue)

Regards,

Pierric

-------------------------------------------------------------

Hi,

Thank you for reviewing the code and sharing your findings!

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

>/Hi there,/

>/ /

>/I recently started to analyse the Jitsi XMPP Implementation and I found/

>/this strange note in MucRoomJabberImpl class (Chat room support/

>/implementation) :confused:

>/ /

>/“/

>/ /

>//// 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 :confused:

>/ /

>/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 :confused:

>/ /

>/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

This message contains information that may be privileged or
confidential and is the property of the Capgemini Group. It is
intended only for the person to whom it is addressed. If you are not
the intended recipient, you are not authorized to read, print, retain,
copy, disseminate, distribute, or use this message or any part
thereof. If you receive this message in error, please notify the
sender immediately and delete all copies of this message.

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev


#3

Hi Danny, thanks for your answer.

I tried the method you described in the previous mail, but I ran into another issue.

From plugin, I used the UIService to place a JButton in the CHAT_STATUS_BAR (it appears below all chats).
On click, I am fetching the ProtocolProviderService then its OperationSetBasicInstantMessaging, but I can't cast it to OperationSetBasicInstantMessagingJabberImpl as the bundle containing this class does not expose anything. OSGI fails to import the package. I found the jar at "jitsi/sc-bundles/protocol-jabber.jar" . The only classes exposed are the interfaces contained in "jitsi/sc-bundles/protocol.jar".
Moreover, even if I could cast it to OperationSetBasicInstantMessagingJabberImpl, the method I would be interested in is :

"
private MessageDeliveredEvent sendMessage( Contact to,
                                    ContactResource toResource,
                                    Message message,
                                    PacketExtension[] extensions)
"

Which is private.
Currently, it seems that Jitsi cannot provide to add any way to add extra information to XMPP packets from plugins, or may I did something wrong ?

Regards,
Pierric

-----Message d'origine-----

···

De : dev [mailto:dev-bounces@jitsi.org] De la part de Danny van Heumen
Envoyé : mercredi 9 décembre 2015 19:30
À : Jitsi Developers
Objet : Re: [jitsi-dev] MucRoomJabberImpl fix proposition

Hi Pierric,

I can't comment on all your remarks, but I'll comment on one specifically. See below.

On 09-12-15 16:46, WILLEMET, Pierric wrote:

Hi,

[...]

May you can help me with this. I am actually searching for best
practice to insert PacketExtension in XMPP messages from plugin.

The OperationSet BasicInstantMessaging does not provide any way to add
any extra information with the message beside “Content”, “Subject”,
“From” and “To”.

Doesn’t Jitsi provide anyway to do this ?

No, not out of the box, as the operation set BasicInstantMessaging is only composed for the functionality that is common to "all" protocols.

This is very specific to the XMPP protocol implementation for basic instant messaging support. It makes sense, in this case, to check if the implementing class is specifically the XMPP protocol implementation and then cast it to the concrete type. At that point, you have the concrete type and thus all the implementation details available that are offered.
Including those not available in the interface definition. (Of course you should also gracefully handle cases where the type is different.)

Kind regards,
Danny

(May I should open a new thread for this issue)

Regards,

Pierric

-------------------------------------------------------------

Hi,

Thank you for reviewing the code and sharing your findings!

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

>/Hi there,/

>/ /

>/I recently started to analyse the Jitsi XMPP Implementation and I
>found/

>/this strange note in MucRoomJabberImpl class (Chat room support/

>/implementation) :confused:

>/ /

>/“/

>/ /

>//// 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 :confused:

>/ /

>/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 :confused:

>/ /

>/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

This message contains information that may be privileged or
confidential and is the property of the Capgemini Group. It is
intended only for the person to whom it is addressed. If you are not
the intended recipient, you are not authorized to read, print, retain,
copy, disseminate, distribute, or use this message or any part
thereof. If you receive this message in error, please notify the
sender immediately and delete all copies of this message.

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev

This message contains information that may be privileged or confidential and is the property of the Capgemini Group. It is intended only for the person to whom it is addressed. If you are not the intended recipient, you are not authorized to read, print, retain, copy, disseminate, distribute, or use this message or any part thereof. If you receive this message in error, please notify the sender immediately and delete all copies of this message.


#4

Hi Pierric,

Hi Danny, thanks for your answer.

I tried the method you described in the previous mail, but I ran into another issue.

From plugin, I used the UIService to place a JButton in the CHAT_STATUS_BAR (it appears below all chats).
On click, I am fetching the ProtocolProviderService then its OperationSetBasicInstantMessaging, but I can't cast it to OperationSetBasicInstantMessagingJabberImpl as the bundle containing this class does not expose anything. OSGI fails to import the package. I found the jar at "jitsi/sc-bundles/protocol-jabber.jar" . The only classes exposed are the interfaces contained in "jitsi/sc-bundles/protocol.jar".

That makes sense. In that case, i.e. if the classes aren't exported,
then you do indeed have a tricky situation.

The "nicest" way would be to define a new interface for "extensions" in
general, of course. But this is very tricky, as you would need to make
this a generic solution that fits with various protocols. I don't think
this is the right way to go at this moment, as we only have 1 (or rather
1 potential) implementation of this right now.

The other way would be to see if we can expose the required classes.
Maybe you can try this out in your local working copy to see if this
works as expected. It does not make sense to add (Jabber-specific)
PacketExtensions to the generic interfaces, so we would again need to cast.
It is of course trivial to add a public method which accepts
PacketExtensions and forward this to the private method.

Moreover, even if I could cast it to OperationSetBasicInstantMessagingJabberImpl, the method I would be interested in is :

"
private MessageDeliveredEvent sendMessage( Contact to,
                                    ContactResource toResource,
                                    Message message,
                                    PacketExtension[] extensions)
"

Which is private.
Currently, it seems that Jitsi cannot provide to add any way to add extra information to XMPP packets from plugins, or may I did something wrong ?

I haven't checked the code right now, so I can't say for sure. But IIRC
I did not encounter such methods while implementing IRC support. I'd
need to check the code though, to be sure.

Danny

···

On 10-12-15 10:07, WILLEMET, Pierric wrote:

Regards,
Pierric

-----Message d'origine-----
De : dev [mailto:dev-bounces@jitsi.org] De la part de Danny van Heumen
Envoyé : mercredi 9 décembre 2015 19:30
À : Jitsi Developers
Objet : Re: [jitsi-dev] MucRoomJabberImpl fix proposition

Hi Pierric,

I can't comment on all your remarks, but I'll comment on one specifically. See below.

On 09-12-15 16:46, WILLEMET, Pierric wrote:

Hi,

[...]

May you can help me with this. I am actually searching for best
practice to insert PacketExtension in XMPP messages from plugin.

The OperationSet BasicInstantMessaging does not provide any way to add
any extra information with the message beside “Content”, “Subject”,
“From” and “To”.

Doesn’t Jitsi provide anyway to do this ?

No, not out of the box, as the operation set BasicInstantMessaging is only composed for the functionality that is common to "all" protocols.

This is very specific to the XMPP protocol implementation for basic instant messaging support. It makes sense, in this case, to check if the implementing class is specifically the XMPP protocol implementation and then cast it to the concrete type. At that point, you have the concrete type and thus all the implementation details available that are offered.
Including those not available in the interface definition. (Of course you should also gracefully handle cases where the type is different.)

Kind regards,
Danny

(May I should open a new thread for this issue)

Regards,

Pierric

-------------------------------------------------------------

Hi,

Thank you for reviewing the code and sharing your findings!

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

/Hi there,/
/ /
/I recently started to analyse the Jitsi XMPP Implementation and I
found/
/this strange note in MucRoomJabberImpl class (Chat room support/
/implementation) :confused:
/ /
/“/
/ /
//// 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 :confused:
/ /
/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 :confused:
/ /
/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

This message contains information that may be privileged or
confidential and is the property of the Capgemini Group. It is
intended only for the person to whom it is addressed. If you are not
the intended recipient, you are not authorized to read, print, retain,
copy, disseminate, distribute, or use this message or any part
thereof. If you receive this message in error, please notify the
sender immediately and delete all copies of this message.

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev

This message contains information that may be privileged or confidential and is the property of the Capgemini Group. It is intended only for the person to whom it is addressed. If you are not the intended recipient, you are not authorized to read, print, retain, copy, disseminate, distribute, or use this message or any part thereof. If you receive this message in error, please notify the sender immediately and delete all copies of this message.
_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev


#5

Hi,

I followed your previous recommendations and I report you my results.

"
The "nicest" way would be to define a new interface for "extensions" in general, of course. But this is very tricky, as you would need to make this a generic solution that fits with various protocols. I don't think this is the right way to go at this moment, as we only have 1 (or rather
1 potential) implementation of this right now.

The other way would be to see if we can expose the required classes.
Maybe you can try this out in your local working copy to see if this works as expected. It does not make sense to add (Jabber-specific) PacketExtensions to the generic interfaces, so we would again need to cast.
It is of course trivial to add a public method which accepts PacketExtensions and forward this to the private method.
"

In the manifest of the Jabber Impl, I exposed two packages :
- net.java.sip.communicator.impl.protocol.jabber,
- net.java.sip.communicator.impl.protocol.jabber.extensions
By doing so, I was able to cast to the OperationSetBasicInstantMessaging to OperationSetBasicInstantMessagingJabberImpl as expected and to create a class extending the AbstractPacketExtension in order to use it as parameter of the method :

" private MessageDeliveredEvent sendMessage( Contact to,
                                    ContactResource toResource,
                                    Message message,
                                    PacketExtension[] extensions) "

I modified a bit the OperationSetBasicInstantMessagingJabberImpl class by adding a public method which takes the same parameters as above, calls the private method, then fire a MessageDeliveredEvent.

But this was not enough. In order to use this new method instead of the already-existing when the user press "ENTER" to send his message, I had to fetch the ChatWritePanel and the ChatPanel to redirect the "send" action to a custom action in my plugin which perform the different tasks before send the message. I also had to expose the ChatPanel and the ChatWritePanel in the concerned Manifest file.
Currently, my plugin works as expected (message are send with extra-information in XMPP packets), but I think Jitsi should offer an easier way to achieve this. It would, for instance, greatly facilitate the development of XEP support.

Let me know if you want me to expose my code, propose a pull request, or add something in the bug tracker.

Regards,

Pierric.

-----Message d'origine-----

···

De : dev [mailto:dev-bounces@jitsi.org] De la part de Danny van Heumen
Envoyé : samedi 12 décembre 2015 01:17
À : Jitsi Developers
Objet : Re: [jitsi-dev] MucRoomJabberImpl fix proposition

Hi Pierric,

On 10-12-15 10:07, WILLEMET, Pierric wrote:

Hi Danny, thanks for your answer.

I tried the method you described in the previous mail, but I ran into another issue.

From plugin, I used the UIService to place a JButton in the CHAT_STATUS_BAR (it appears below all chats).
On click, I am fetching the ProtocolProviderService then its OperationSetBasicInstantMessaging, but I can't cast it to OperationSetBasicInstantMessagingJabberImpl as the bundle containing this class does not expose anything. OSGI fails to import the package. I found the jar at "jitsi/sc-bundles/protocol-jabber.jar" . The only classes exposed are the interfaces contained in "jitsi/sc-bundles/protocol.jar".

That makes sense. In that case, i.e. if the classes aren't exported, then you do indeed have a tricky situation.

The "nicest" way would be to define a new interface for "extensions" in general, of course. But this is very tricky, as you would need to make this a generic solution that fits with various protocols. I don't think this is the right way to go at this moment, as we only have 1 (or rather
1 potential) implementation of this right now.

The other way would be to see if we can expose the required classes.
Maybe you can try this out in your local working copy to see if this works as expected. It does not make sense to add (Jabber-specific) PacketExtensions to the generic interfaces, so we would again need to cast.
It is of course trivial to add a public method which accepts PacketExtensions and forward this to the private method.

Moreover, even if I could cast it to OperationSetBasicInstantMessagingJabberImpl, the method I would be interested in is :

"
private MessageDeliveredEvent sendMessage( Contact to,
                                    ContactResource toResource,
                                    Message message,
                                    PacketExtension[] extensions) "

Which is private.
Currently, it seems that Jitsi cannot provide to add any way to add extra information to XMPP packets from plugins, or may I did something wrong ?

I haven't checked the code right now, so I can't say for sure. But IIRC I did not encounter such methods while implementing IRC support. I'd need to check the code though, to be sure.

Danny

Regards,
Pierric

-----Message d'origine-----
De : dev [mailto:dev-bounces@jitsi.org] De la part de Danny van Heumen
Envoyé : mercredi 9 décembre 2015 19:30 À : Jitsi Developers Objet :
Re: [jitsi-dev] MucRoomJabberImpl fix proposition

Hi Pierric,

I can't comment on all your remarks, but I'll comment on one specifically. See below.

On 09-12-15 16:46, WILLEMET, Pierric wrote:

Hi,

[...]

May you can help me with this. I am actually searching for best
practice to insert PacketExtension in XMPP messages from plugin.

The OperationSet BasicInstantMessaging does not provide any way to
add any extra information with the message beside “Content”,
“Subject”, “From” and “To”.

Doesn’t Jitsi provide anyway to do this ?

No, not out of the box, as the operation set BasicInstantMessaging is only composed for the functionality that is common to "all" protocols.

This is very specific to the XMPP protocol implementation for basic instant messaging support. It makes sense, in this case, to check if the implementing class is specifically the XMPP protocol implementation and then cast it to the concrete type. At that point, you have the concrete type and thus all the implementation details available that are offered.
Including those not available in the interface definition. (Of course
you should also gracefully handle cases where the type is different.)

Kind regards,
Danny

(May I should open a new thread for this issue)

Regards,

Pierric

-------------------------------------------------------------

Hi,

Thank you for reviewing the code and sharing your findings!

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

/Hi there,/
/ /
/I recently started to analyse the Jitsi XMPP Implementation and I
found/ /this strange note in MucRoomJabberImpl class (Chat room
support/
/implementation) :confused:
/ /
/“/
/ /
//// 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 :confused: / / /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 :confused:
/ /
/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

This message contains information that may be privileged or
confidential and is the property of the Capgemini Group. It is
intended only for the person to whom it is addressed. If you are not
the intended recipient, you are not authorized to read, print,
retain, copy, disseminate, distribute, or use this message or any
part thereof. If you receive this message in error, please notify the
sender immediately and delete all copies of this message.

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev

This message contains information that may be privileged or confidential and is the property of the Capgemini Group. It is intended only for the person to whom it is addressed. If you are not the intended recipient, you are not authorized to read, print, retain, copy, disseminate, distribute, or use this message or any part thereof. If you receive this message in error, please notify the sender immediately and delete all copies of this message.
_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev