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

Fixing JOIN parse #1049

Closed
wants to merge 7 commits into from
Closed

Conversation

iwalkalone69
Copy link

The parse of JOIN was wrong. Channel name its after :#, not before.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 31.321% when pulling 4a99fca on iwalkalone69:master into ca9ac01 on processone:master.

@badlop
Copy link
Member

badlop commented Apr 6, 2016

Ah, right, the old code was something like:

> lists:subtract(":#cove", ":#").
"cove"

But the new code messed it when was converted to binary:

1>  [Chan1 | _] = binary:split(<<":#cove">>, <<":#">>), Chan1.
<<>>

However, your solution isn't yet fully equivalent:

2> [_ | Chan2] = binary:split(<<":#cove">>, <<":#">>), Chan2.
[<<"cove">>]

This would be better, right? Can you confirm this third solution is better?

3> [_, Chan3] = binary:split(<<":#cove">>, <<":#">>), Chan3.
<<"cove">>

@badlop badlop self-assigned this Apr 6, 2016
@badlop badlop added this to the ejabberd 16.04 milestone Apr 6, 2016
@iwalkalone69
Copy link
Author

I am so sorry but I'm not good with erlang and I don't understand the difference with 2 and 3 in your examples. Now the code i submitted is:

[FromUser | FromIdent] = str:tokens(From, <<"!">>),
[_ | Chan] = binary:split(Channel, <<":#">>),
ejabberd_router:route(jid:make(iolist_to_binary(

What would you change here?

Thanks.

@badlop
Copy link
Member

badlop commented Apr 6, 2016

The difference between 2 and 3 is the character between _ and Chan: you put | and I put , because that way the result is more correct, apparently.

@iwalkalone69
Copy link
Author

You're right, what you say fixes JOIN and NICK messages.
Thanks a lot! I update the pull request.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 31.158% when pulling 90688db on iwalkalone69:master into ca9ac01 on processone:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 31.269% when pulling 06e67a6 on iwalkalone69:master into ca9ac01 on processone:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 31.272% when pulling 8829e90 on iwalkalone69:master into ca9ac01 on processone:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 31.273% when pulling faad65c on iwalkalone69:master into ca9ac01 on processone:master.

@mremond
Copy link
Member

mremond commented Apr 13, 2016

Hi, why did you close it ?

@iwalkalone69
Copy link
Author

Because I noticed it is failing and I want to submit it with more patches. In general I see that a lot of things have to be improved more closed to the IRC protocol. For example, KICK messages are not sending the information in a proper way for a web client.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 31.273% when pulling fe63561 on iwalkalone69:master into ca9ac01 on processone:master.

@mremond
Copy link
Member

mremond commented Apr 13, 2016

ok, fine then.

Please, keep us posted :)

@iwalkalone69
Copy link
Author

Maybe you can help me with some doubts. Why it is sending two stanza for some single IRC line? It could be done with only one. This happens in multiple places, for example process_kick().

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 31.265% when pulling fe63561 on iwalkalone69:master into ca9ac01 on processone:master.

@mremond
Copy link
Member

mremond commented Apr 13, 2016

@badlop any idea about this question ?

@iwalkalone69
Copy link
Author

For now, I'm fixing some messages and adding some parameters to isolate parts of the IRC protocol messages. For example, in KICK, I've added the param moderator to be able to get it in the client as the one who kicks the user and message to get the kick message. What was happening also is that if kick message had :, this was truncated to the string since the last :.

I've also added message parameter to isolate the part and quit messages.

For now I'm planning to send the 366 numeric message (end of /names) to be able to, when client receives it, render all previous users & channel user count all at once in the web client jQuery app I'm coding to improve performance.

When I finish, I will post it here with all modifications and the reasons why I did it.

@lock
Copy link

lock bot commented Jun 10, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants