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

get resume id from server #134

Closed
wants to merge 1 commit into from
Closed

Conversation

coderlxn
Copy link

No description provided.

@olesalscheider
Copy link
Contributor

LGTM.

@jlaine
Copy link
Contributor

jlaine commented Jun 23, 2018

IQ parsing / serialization is trivial to unit test, so do it :)

@jlaine jlaine force-pushed the master branch 4 times, most recently from 81b12d9 to cd81054 Compare June 24, 2018 08:26
@jlaine
Copy link
Contributor

jlaine commented Sep 30, 2018

@coderlxn can we please have a unit test for this so we can merge it?

@jlaine
Copy link
Contributor

jlaine commented Jan 21, 2019

@lnjX I'm not sure I understand what this does anymore, ideas?

@@ -132,6 +132,7 @@ void QXmppStreamManagementEnabled::parse(const QDomElement &element)
m_resume = resume == QString("true") || resume == QString("1");
m_max = element.attribute("max").toUInt();
m_location = element.attribute("location");
m_id = element.attribute("id");
Copy link
Member

@lnjX lnjX Jan 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only adds parsing, we also need to add this in ::toXml.

just in case someone actually wants to use the QXmppServer ;)

@lnjX
Copy link
Member

lnjX commented Jan 21, 2019

@jlaine The session id needs to be parsed to be able to resume the session (using previd in <resume/>) after a possible connection loss.


See https://xmpp.org/extensions/xep-0198.html#resumption

@coderlxn
Copy link
Author

@jlaine What kind of unit test should I add for this pull request? Sorry, I'm not familiar with the progress of pull request.

@lnjX
Copy link
Member

lnjX commented Jan 27, 2019

Unfortunately there's no test class for stream management, yet, but you could base on this test basically. It were also nice to have this new test class in a new commit.

@lnjX
Copy link
Member

lnjX commented Sep 9, 2024

Parsing of the id attribute has been added in the meantime, so this is obsolete.

@lnjX lnjX closed this Sep 9, 2024
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.

4 participants