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

Push Notifications (XEP-0357) implementation #83

Closed

Conversation

ramabit
Copy link
Contributor

@ramabit ramabit commented Jun 10, 2016

Push Notifications XEP-0357
Documentation: http://xmpp.org/extensions/xep-0357.html

  • Enable push notifications
  • Disable push notifications
  • Parse remote disabling of push notifications

@ramabit ramabit force-pushed the ramabit.push.notifications branch 2 times, most recently from 9dd9d11 to fc295a3 Compare June 10, 2016 20:46

@Override
public CharSequence toXML() {
XmlStringBuilder xml = new XmlStringBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

Should use new XmlStringbuilder(this) when possible. As it makes sure that the XML is well qualified with a namespace. This is not the case in the current code since only halfOpenElement() is used which does not add xmlns. As extra plus also reduces the LOC count by one since halfOpenElement() is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Flowdalic fixed

@Flowdalic
Copy link
Member

Needs conflict resolution

final XMPPConnection connection = connection();

IQ responseIQ = null;
PacketCollector responseIQCollector = connection.createPacketCollector(new IQReplyFilter(iq, connection));
Copy link
Member

Choose a reason for hiding this comment

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

For IQ request/response sequences simply use

connection.createPacketCollectorAndSend(iq).nextResultOrThrow();

this combination takes care of canceling the collector in any case. Reduces the LOC by ~10 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Flowdalic before or after connection.sendStanza(iq); ?

Copy link
Member

Choose a reason for hiding this comment

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

createPacketCollectorAndSend(IQ) does also send the stanza (look at the method name ;))

Thus the Smack Idiom to send a request IQ and get its response is

IQ request = ...;
IQ response = connection.createPacketCollectorAndSend(request).nextResultOrThrow();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Flowdalic awesome

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ramabit ramabit force-pushed the ramabit.push.notifications branch 2 times, most recently from 2f02e21 to 0bbc432 Compare July 27, 2016 18:52
@ramabit ramabit closed this Jul 27, 2016
@ramabit ramabit reopened this Jul 27, 2016
@ramabit ramabit force-pushed the ramabit.push.notifications branch from f46ab9a to fc9b1d6 Compare July 29, 2016 13:37
@ramabit
Copy link
Contributor Author

ramabit commented Jul 29, 2016

@Flowdalic Conflicts resolution done

@ramabit ramabit force-pushed the ramabit.push.notifications branch 2 times, most recently from 8cf2914 to f1f9758 Compare August 4, 2016 15:04
@Flowdalic
Copy link
Member

Flowdalic commented Oct 31, 2016

Reviewed, this appears to be ready to go after it has been squashed. Assigned SMACK-738 for it. Please mention the key somewhere in the commit message.

@ramabit ramabit force-pushed the ramabit.push.notifications branch 2 times, most recently from d525971 to 488e962 Compare October 31, 2016 16:52
@ramabit ramabit closed this Oct 31, 2016
@ramabit ramabit reopened this Oct 31, 2016
@ramabit
Copy link
Contributor Author

ramabit commented Oct 31, 2016

@Flowdalic Done
Note: The continuous integration is not working ok, is throwing errors for all branches in files that I didn't modify

@Flowdalic
Copy link
Member

Thanks, cherry picked ase266b1acd804c428cde26fa99fac265d9811612e

Note that putting the issue key in the commit title is discouraged.

@Flowdalic Flowdalic closed this Nov 1, 2016
@tenmaster tenmaster deleted the ramabit.push.notifications branch November 1, 2016 17:55
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.

2 participants