Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extensive changes #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

agimenezwally
Copy link

Current changes:

1-Added a new constructor to IM.Message which allows to create a message from an XmlElement
2-Left the Xmpp.Xml namespace public to allow to use the functions to create/append Xml elements
3-Implemented a new event named BodylessMessage which receives messages with no body
4-BodylessMessage uses MessageEventArgs, for that I created a new constructor which does not require a Jid as these messages come without an ID (they come from the server itself, not from an user)
6-Created a property on the connection to disable roster retrieval when connection is created.
7-Created a public function to send a default Presence stanza as the usual with no roster is to not send the presence, but in case some service uses it without roster the function is available.
8-Changed Tls from bool to enum, this allows to use StartTLS extension or Ssl sockets.
9-Updated ArSoft.Tools.Net reference

@pgstath
Copy link
Owner

pgstath commented Feb 2, 2016

I have created the dev-GCM-pull14 branch in order to test the changes. I will make some minor changes, in order to preserve backward compatibility. Then I will commit in order to test it, and then I can merge it to master.

@pgstath
Copy link
Owner

pgstath commented Feb 19, 2016

Hi @agimenezwally, I have just realized that I did not mentioned you in the previous comment. Did you or @gusmanb had a chance to test 24445c7 for GCM? I would like to merge it with the main branch, but before that I would like you to confirm that it is working for your use cases.
Thanks again.

@gusmanb
Copy link

gusmanb commented Feb 19, 2016

HI @pgstath .

Sorry for the delay, I'm on a business travel so I can't test it until next week...
If you want, merge it and if there is any problem (i doubt because the changes you did seem to be fully compatible with my use case, GCM) I can create another pull request.

Cheers.

@pgstath pgstath mentioned this pull request Jul 10, 2016
@gusmanb
Copy link

gusmanb commented Jul 11, 2016

Hi @pgstath sorry for the delay I've been really busy these months.

Now I have some spare time, if you guide me a bit on how can I retrieve all the changes and test it I can check this week if everything works with GCM (I'm not very good with branches, pull requests, versions and so on on GIT, sorry :( )

@olibos
Copy link

olibos commented Jul 18, 2016

Hi @pgstath ,
I have tested your branch with FCM (Firebase Cloud Message, the new name of GCM) and I can connect, send & receive messages.

Thank you!

If @gusmanb agrees, I think you can merge the changes to the main branch.

Cheers.

@gusmanb
Copy link

gusmanb commented Jul 18, 2016

HI @olibos

Of course, if it's working I would be more than glad to see this merged on main.

Also @pgstath if you want to remove the bodyless message event and mix it with the message event, it would be great, just need to remove the event and were was the rising change it to this code:

if (message.Data["body"] != null) Message.Raise(this, new MessageEventArgs(message.From, message)); else Message.Raise(this, new MessageEventArgs(message));

Sorry for the formatting, I have no clue on how to add multiline code...

@olibos
Copy link

olibos commented Jul 25, 2016

Hello,

I have made the change from @gusmanb in my forked repo: https://github.com/olibos/Sharp.Xmpp/tree/dev-GCM-pull14

Just a quick question, is there any reason why XmppExtension, IInputFilter and IOutputFilter are internals?

If everything related to Extensions are switched to public it may allow to extend XMPP with some proprietary handling.
In the case of FCM, it may handle:

If it's public we can avoid adding proprietary code inside the core library and add those inside another library which use Sharp.Xmpp and then avoid to reinvent the wheel in each FCM client application.

What do you think? @pgstath, @gusmanb

@gusmanb
Copy link

gusmanb commented Jul 26, 2016

Hi @olibos and @pgstath

I can see two things here:

1-Control messages must be managed by the app or library, I can't see any benefit of using an extension, per example in my implementation when an ACK is received I remove the messages from a retry queue, so an extension is not viable for it, and for the draining, when I receive it I create a new connection and move the draining connection to a separate management queue to destroy it when the draining is finished.

2-Despite point 1, it may be useful in other cases to have the functionality of extensions available, maybe transformation of an incomming/outgoing message to simplify main code, or to support different data codification per example.

So if @pgstath agrees I belive it's a good and viable enhancement.

@olibos
Copy link

olibos commented Jul 26, 2016

Hi @gusmanb ,

I agree, but the control message could be managed in an event inside the extension like UserAvatar.AvatarChanged event.
The app or library can connect to the Control event.
Then the application manage only messages inside Message event and Control inside the Control event.

I will probably make a proof of concept if @pgstath makes extensions public ;-)

@gusmanb
Copy link

gusmanb commented Jul 26, 2016

Hmmm, good point @olibos and nice thought, this can really simplify the code and make it more maintainable.

Cheers.

@olibos olibos mentioned this pull request Jul 26, 2016
@pgstath
Copy link
Owner

pgstath commented Aug 4, 2016

Hi guys, apologies for the delay, some quite heavy workload during this period. I will try to follow up with the discussion and test the pull request.

@gusmanb
Copy link

gusmanb commented Sep 4, 2016

Hi guys.

I'm just here to inform I have ported the library to .net Core, I had to replace the dns resolution system, make some things async and blablabla :D

If you want to take a look at it it's here: https://github.com/gusmanb/Sharp.Xmpp.NetCore

It have mixed the most interesting changes I have seen (olivo's BodylessMessage unification, UseRooster changed to RetrieveRooster and all my previous changes) but if I missed something don't be hesitate to do a pull request.

This is still under test, but the base to make it work is here, I'm going to work on this for the next weeks until I got all running and stable as I want to change all the API to be async.

Cheers.

stevenlivz added a commit to ParamountVentures/XMPPEngineer that referenced this pull request May 9, 2017
@sk6868
Copy link

sk6868 commented Jun 11, 2020

It would be great if this issue could be resolved and merged into mainline. Trying to connect to FCM via Sharp.Xmpp at the moment is kind of a pain since the NuGet version does not work. Is anyone following this up?

jpenny1993 pushed a commit to jpenny1993/Sharp.Xmpp that referenced this pull request Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants