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

XEP-0172 Improvement #276

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

miguelhincapie
Copy link
Contributor

Description

Current XEP-0172 in Smack library only has a minimal implementation Nick class.
With this PR some classes are added in order to make this extension easily to use.

Changes

  • Added NickManager class
  • Added NickListener
  • Added NickFilter

Copy link
Member

@Flowdalic Flowdalic left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, please find my remarks inline.

asyncButOrdered.performAsyncButOrdered(message, new Runnable() {
@Override
public void run() {
for (NickListener listener : nickListeners) {
Copy link
Member

Choose a reason for hiding this comment

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

nickListeners is not synchronized although it should be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean something like:

synchronized (nickListeners) {
     for (NickListener listener : nickListeners) {
         listener.newNickMessage(message);
     }
 }

am I right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, or some other sort of means to ensure access to the datastructure is properly synchronized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, added a synchronized, tell me if that is enough or you like another kind of synchronized.

SmackException.NotLoggedInException {
final Message message = (Message) packet;

asyncButOrdered.performAsyncButOrdered(message, new Runnable() {
Copy link
Member

Choose a reason for hiding this comment

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

The AsyncButOrdered API is not correctly used. Right now, it assures the runnable is ordered with respect to the message, when it should be ordered with respect to the originating entity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain me a little bit more about what could be the originating entiy?
Because there is no much doc about that AsyncButOrdered I have been using it as I saw in others parts of Smack, so it's not much clear to me how should I use it or when or the options I have instead it.

Copy link
Member

Choose a reason for hiding this comment

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

The originating entity is the entity that send the message stanza. So you need to replace message with message.getFrom(). But make sure to add a null check.

Copy link
Contributor Author

@miguelhincapie miguelhincapie Mar 30, 2019

Choose a reason for hiding this comment

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

and what if it is null? I mean, should I throw an exception?

@miguelhincapie
Copy link
Contributor Author

I hope someday my 2 PRs can be merged ... u_u

@Flowdalic
Copy link
Member

I am so sorry. Multiple factors (other important things, illness, holiday season) delay that PR, but I have not forgotten about it. I hope to find some time for this before the upcoming XSF Summit.

SmackException.NotLoggedInException {
final Message message = (Message) packet;

asyncButOrdered.performAsyncButOrdered(message, new Runnable() {
Copy link
Member

Choose a reason for hiding this comment

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

The originating entity is the entity that send the message stanza. So you need to replace message with message.getFrom(). But make sure to add a null check.

asyncButOrdered.performAsyncButOrdered(message, new Runnable() {
@Override
public void run() {
for (NickListener listener : nickListeners) {
Copy link
Member

Choose a reason for hiding this comment

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

Yes, or some other sort of means to ensure access to the datastructure is properly synchronized.

import static org.junit.Assert.assertNotNull;


public class NickManagerTest extends InitExtensions {
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 I was searching in current tests and I couldn't find anything as a guide or to follow, the most close to it was DeliveryReceiptTest, so please be patient with this part :).

Can I use Mockito?
i.e. for getInstanceFor I wish to:

  • test if INSTANCES is really working.
  • test if addSyncStanzaListener is being called inside constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm feeling lonely with this u_u'

Copy link
Member

Choose a reason for hiding this comment

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

Can I use Mockito?

Yes, but it is usually a good idea to start with tests that do not require mocking.

}

@Test
public void addNickMessageListener() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in order to be able to do this test I will need to add Set<NickListener> nickListeners to parameters constructor because I need to mock it first. What do you think @Flowdalic?

Copy link
Member

Choose a reason for hiding this comment

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

"to parameters constructor"? Depends on what you want to test within the test.

}

@Test
public void getInstanceFor() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Test methods should be prefixed (or postfixed) with "test".

@Test
public void getInstanceFor() throws Exception {
DummyConnection dummyConnection = new DummyConnection();
dummyConnection.connect();
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason for calling connect()?

import static org.junit.Assert.assertNotNull;


public class NickManagerTest extends InitExtensions {
Copy link
Member

Choose a reason for hiding this comment

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

Can I use Mockito?

Yes, but it is usually a good idea to start with tests that do not require mocking.




public class NickManagerTest extends InitExtensions {
Copy link
Member

Choose a reason for hiding this comment

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

Please migrate this to JUnit 5.

return nickManager;
}

public synchronized boolean addNickMessageListener(NickListener listener) {
Copy link
Member

Choose a reason for hiding this comment

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

That synchronization is broken: You are protecting nickListeners with the Manager's object monitor and above you use the nickListeners's monitor.

final Message message = (Message) packet;

Jid from = message.getFrom();
if (from != null) {
Copy link
Member

Choose a reason for hiding this comment

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

In order to avoid an extra level of indentation use if (from == null) return; here.

}

@Test
public void addNickMessageListener() {
Copy link
Member

Choose a reason for hiding this comment

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

"to parameters constructor"? Depends on what you want to test within the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants