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

Implementation of XEP-0308: Last Message Correction #73

Closed
wants to merge 1 commit into from

Conversation

ramabit
Copy link
Contributor

@ramabit ramabit commented Apr 7, 2016

Fixes SMACK-714

Documentation: https://xmpp.org/extensions/xep-0308.html

How to use it

Send:
message.addExtension(new MessageCorrectExtension(idInitialMessage));

Receive:
ProviderManager.addExtensionProvider(MessageCorrectExtension.ELEMENT_NAME, MessageCorrectExtension.NAMESPACE, new MessageCorrectProvider());

MessageCorrectExtension messageCorrectExtension = MessageCorrectExtension.from(message)

@ramabit ramabit closed this Apr 7, 2016
@ramabit ramabit reopened this Apr 7, 2016
@ramabit ramabit closed this Apr 7, 2016
@ramabit ramabit reopened this Apr 7, 2016
@ramabit
Copy link
Contributor Author

ramabit commented Apr 7, 2016

@Flowdalic TravisCI is killing the process of the 2nd check (java 1.7). I guess it is because of low memory.

@ramabit ramabit closed this Apr 7, 2016
@ramabit ramabit reopened this Apr 7, 2016
@@ -0,0 +1,120 @@
/**
*
* Copyright the original author or authors
Copy link
Member

Choose a reason for hiding this comment

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

That line should include a copyright year, i.e. 2016. And you state your name in the other copyright headers, why not here too?

@Flowdalic
Copy link
Member

Please squash merge all commits into a single commit and make sure that the issue's key 'SMACK-714' is mentioned somewhere in the commit message's body. Please don't put it into the title. Something like "Fixes SMACK-714" is sufficient.

Uhh, and I forget to say: Thanks for your contribution. :-)

*
* @author Fernando Ramirez, f.e.ramirez94@gmail.com
*/
public class MessageCorrectExtension implements ExtensionElement {
Copy link
Member

Choose a reason for hiding this comment

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

It's Smackomatic (a Smack Idiom) that ExtensionElements have a static from() method. Since XEP-308 is only defined for messages, in this case we should have

static MessageCorrectionExtension from(Message)

Please add this method.

@ramabit
Copy link
Contributor Author

ramabit commented Apr 11, 2016

@Flowdalic Thanks for the feedback !
The corrections are done and the pull request description is updated.
TravisCI is failing because a test that I didn't touch. Also, I can't reproduce that failure locally.

.append(getJidInitialMessage()).append('\'').append(" xmlns='").append(getNamespace()).append("'/>");
return stringBuilder.toString();
public XmlStringBuilder toXML() {
XmlStringBuilder xml = new XmlStringBuilder(this, getNamespace());
Copy link
Member

Choose a reason for hiding this comment

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

Just new XmlStringBuilder(this) will do the right thing, i.e. adding the opening tag and the xmlns attribute.

@Flowdalic
Copy link
Member

TravisCI is failing because a test that I didn't touch. Also, I can't reproduce that failure locally.

Some older Smack unit tests are not properly synchronized causing them to behave non deterministc especially on slow systems like the Travis CI infrastructure. I've restarted Travis (not sure if you are also able to hit the restart button, e.g. at https://travis-ci.org/igniterealtime/Smack/jobs/122258804 in the upper right corner).

Looks good so far 👍 I've added a few last remarks, please incorporate the changes, then squash merge all you commits into a single commit and then force push to the source branch of this PR.

@Flowdalic
Copy link
Member

By the way, this PR only implements the low-level API parts for XEP-308. While it's ok to start with them first, do you plan to submit an subsequent PR with the high-level API, i.e. the MessageCorrectionManager)?

@ramabit
Copy link
Contributor Author

ramabit commented Apr 11, 2016

By the way, this PR only implements the low-level API parts for XEP-308. While it's ok to start with them first, do you plan to submit an subsequent PR with the high-level API, i.e. the MessageCorrectionManager)?

It's not planned up to now, but I think we can discuss it.

@ramabit
Copy link
Contributor Author

ramabit commented Apr 11, 2016

@Flowdalic Changes done

@Flowdalic
Copy link
Member

Perfect, now you need to squash the commits into one.

@ramabit
Copy link
Contributor Author

ramabit commented Apr 11, 2016

@Flowdalic Github has a new option to do that when merge.

@ramabit
Copy link
Contributor Author

ramabit commented Apr 11, 2016

@Flowdalic I can close this PR and open another one squashing the commits

@Flowdalic
Copy link
Member

I can close this PR an open another one squashing the commits

Simply update this PR by updating its source branch with a forced push.

@ramabit ramabit force-pushed the master branch 2 times, most recently from d169632 to b3c3a43 Compare April 11, 2016 20:00
@ramabit
Copy link
Contributor Author

ramabit commented Apr 11, 2016

Simply update this PR by updating its source branch with a forced push.

@Flowdalic done

@Flowdalic
Copy link
Member

Thanks, but the message's body now full of meaningless half sentences, it also does not contain the issue key, which is required so that JIRA connects the issue with this commit (#73 (comment)). Please fix that.

@ramabit
Copy link
Contributor Author

ramabit commented Apr 11, 2016

Thanks, but the message's body now full of meaningless half sentences, it also does not contain the issue key, which is required so that JIRA connects the issue with this commit (#73 (comment)). Please fix that.

@Flowdalic done

@Flowdalic
Copy link
Member

Picked as ca58c65

Thanks again :)

@Flowdalic Flowdalic closed this Apr 14, 2016
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