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

[WIP] DCC Chat/Send #138

Closed
wants to merge 0 commits into from
Closed

Conversation

octylFractal
Copy link

@octylFractal octylFractal commented Sep 15, 2016

It's probably robust, but the code style needs work.

This closes #93.

Things to do:

  • Receive DCC requests
  • Fire DCCConnectionClosedEvent :P
  • Timeout if no response
  • Refactor the Netty handling
  • Implement DCC Send?

@codecov-io
Copy link

codecov-io commented Sep 15, 2016

Current coverage is 32.46% (diff: 0.00%)

Merging #138 into master will decrease coverage by 1.40%

@@             master       #138   diff @@
==========================================
  Files           139        146     +7   
  Lines          3460       3610   +150   
  Methods           0          0          
  Messages          0          0          
  Branches        498        502     +4   
==========================================
  Hits           1172       1172          
- Misses         2258       2408   +150   
  Partials         30         30          

Powered by Codecov. Last update 36075de...ad9896f

@Zarthus
Copy link
Member

Zarthus commented Sep 15, 2016

Is the class naming in line for what KICL generally uses?

I'd imagine the classes should be named 'IrcDcc' etc. Because the ISupport class follows the same pattern.

@octylFractal
Copy link
Author

IRC is convention:
IRC convention image

Not sure about DCC, could be changed to Dcc.


final class NettyManager {

Copy link
Member

Choose a reason for hiding this comment

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

Looks like your IDE is conflicting with Mbaxter's. I don't really care, but it might be useful to ensure you've got the same code style configuration as him later.

@@ -977,6 +977,12 @@ public void ctcp(ClientReceiveCommandEvent event) {
case "FINGER":
reply = "FINGER om nom nom tasty finger";
break;
case "DCC":
Copy link
Member

Choose a reason for hiding this comment

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

Missing break?

Copy link
Author

@octylFractal octylFractal Sep 15, 2016

Choose a reason for hiding this comment

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

It falls through out (it's the last one), plus there's the entire comment below it :)

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like bad coding practice to me. I thought the primary use of that was to make more than one case statement apply.

Copy link
Author

Choose a reason for hiding this comment

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

There's no break needed on the last statement...what would that accomplish?

Copy link
Author

Choose a reason for hiding this comment

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

It's got a break. Let me have a break.

Copy link
Member

@kashike kashike left a comment

Choose a reason for hiding this comment

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

Some things to fix.

@@ -23,6 +23,17 @@
*/
package org.kitteh.irc.client.library;

import java.io.File;
Copy link
Member

Choose a reason for hiding this comment

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

You rearranged the imports.

import javax.annotation.Nonnull;

public interface DCCChat extends DCCExchange {

Copy link
Member

@kashike kashike Sep 15, 2016

Choose a reason for hiding this comment

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

No empty line at the start of the class.

*
* @return the socket address of the local end
*/
SocketAddress getLocalSocket();
Copy link
Member

Choose a reason for hiding this comment

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

Should be getLocalSocketAddress.

*
* @return the socket address of the remote end
*/
SocketAddress getRemoteSocket();
Copy link
Member

@kashike kashike Sep 15, 2016

Choose a reason for hiding this comment

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

Should be getRemoteSocketAddress.

* the message to send
*/
void sendMessage(@Nonnull String message);

Copy link
Member

Choose a reason for hiding this comment

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

No empty line at the end of the class.

}

class IRCDCCChatSnapshot extends IRCDCCExchangeSnapshot implements DCCChat {

Copy link
Member

Choose a reason for hiding this comment

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

No empty line at the start of the class.


@Override
public void sendMessage(String message) {
this.nettyChannel.writeAndFlush(message.trim());
Copy link
Member

Choose a reason for hiding this comment

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

message should be validated.

public void sendMessage(String message) {
this.nettyChannel.writeAndFlush(message.trim());
}

Copy link
Member

Choose a reason for hiding this comment

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

No empty line at the end of the class.

@@ -23,7 +23,40 @@
*/
package org.kitteh.irc.client.library.implementation;

import java.io.File;
Copy link
Member

Choose a reason for hiding this comment

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

You rearranged the imports.

@@ -256,7 +283,11 @@ private void schedule(boolean force) {
}
long delay = 0;
if (this.scheduledSending != null) {
delay = this.scheduledSending.getDelay(TimeUnit.MILLISECONDS); // Negligible added delay processing this
delay = this.scheduledSending.getDelay(TimeUnit.MILLISECONDS); // Negligible
Copy link
Member

Choose a reason for hiding this comment

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

Why.

@@ -829,4 +829,7 @@ default void sendMultiLineNotice(@Nonnull MessageReceiver target, @Nonnull Strin
* @param reason quit message to send
*/
void shutdown(@Nonnull String reason);

@Nonnull
void beginDCCChat(@Nonnull User target);
Copy link
Member

Choose a reason for hiding this comment

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

Missing javaducks.

@@ -100,4 +102,15 @@
* @return true if away
*/
boolean isAway();

/**
* Begins a DCC chat with this user. When the chat is connected,
Copy link
Member

Choose a reason for hiding this comment

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

    /**
     * Begins a DCC chat with this user.
     *
     * <p>When the chat is connected, a {@link DCCConnectedEvent} will be fired. If the connection fails,
     * a {@link DCCFailedEvent} will be fired.</p>
     */

IRCDCCChatSnapshot snapshot() {
return new IRCDCCChatSnapshot(this);
}

Copy link
Member

Choose a reason for hiding this comment

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

No empty line at the end of the class.

abstract class IRCDCCExchangeSnapshot extends IRCActorSnapshot implements DCCExchange {

protected final io.netty.channel.Channel nettyChannel;
private final SocketAddress local;
Copy link
Member

Choose a reason for hiding this comment

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

localAddress


protected final io.netty.channel.Channel nettyChannel;
private final SocketAddress local;
private final SocketAddress remote;
Copy link
Member

Choose a reason for hiding this comment

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

remoteAddress

@Override
@Nonnull
public String toString() {
return new ToStringer(this).add("client", this.getClient()).add("name", this.getName()).add("localSocket", this.local).add("remoteSocket", this.remote).add("connected", this.connected).toString();
Copy link
Member

Choose a reason for hiding this comment

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

localSocket -> localSocketAddress
remoteSocket -> ``remoteSocketAddress`

private final String type;
private final String name;
private io.netty.channel.Channel nettyChannel;
private SocketAddress local;
Copy link
Member

Choose a reason for hiding this comment

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

localAddress

private final String name;
private io.netty.channel.Channel nettyChannel;
private SocketAddress local;
private SocketAddress remote;
Copy link
Member

Choose a reason for hiding this comment

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

remoteAddress

@@ -829,4 +829,6 @@ default void sendMultiLineNotice(@Nonnull MessageReceiver target, @Nonnull Strin
* @param reason quit message to send
*/
void shutdown(@Nonnull String reason);

void beginDCCChat(@Nonnull User target);
Copy link
Member

Choose a reason for hiding this comment

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

Missing javadocs.

public interface DCCExchange extends Actor {
/**
* Gets the socket address of the local end.
* May return null if not connected.
Copy link
Member

Choose a reason for hiding this comment

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

Optional<SocketAddress>


/**
* Gets the socket address of the remote end.
* May return null if not connected.
Copy link
Member

Choose a reason for hiding this comment

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

Optional<SocketAddress>


public DCCFailedEvent(Client client, String reason, Throwable cause) {
super(client);
this.reason = reason;
Copy link
Member

Choose a reason for hiding this comment

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

Should be Sanity checked, or marked as Optional.

public DCCFailedEvent(Client client, String reason, Throwable cause) {
super(client);
this.reason = reason;
this.cause = cause;
Copy link
Member

Choose a reason for hiding this comment

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

Should be Sanity checked, or marked as Optional.

abstract void onMessage(String msg);

final void onSocketBound() {
sendCTCP();
Copy link
Member

Choose a reason for hiding this comment

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

this.


@Override
public void sendMessage(@Nonnull String message) {
Sanity.nullCheck(message, "message cannot be null");
Copy link
Member

Choose a reason for hiding this comment

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

Should most likely ensure the contents of the message are valid? Not sure if DCC allows different content and does not need validation.

Copy link
Author

Choose a reason for hiding this comment

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

DCC only limitation is that messages are CRLF terminated. I think that accepting CRLF in the message is fine.

import java.util.Queue;
import java.util.Set;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.TimeUnit;

final class NettyManager {

Copy link
Member

Choose a reason for hiding this comment

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

No new line at start of class.

@@ -108,6 +109,7 @@
*
* <p>When the chat is connected, a {@link DCCConnectedEvent} will be fired. If the connection fails,
* a {@link DCCFailedEvent} will be fired.</p>
* @see Client#beginDCCChat(User)
Copy link
Member

Choose a reason for hiding this comment

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

New line before this.

@@ -829,4 +831,12 @@ default void sendMultiLineNotice(@Nonnull MessageReceiver target, @Nonnull Strin
* @param reason quit message to send
*/
void shutdown(@Nonnull String reason);

/**
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment is really well written.

Copy link
Author

Choose a reason for hiding this comment

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

thanks


import javax.annotation.Nonnull;

public interface DCCChat extends DCCExchange {
Copy link
Member

Choose a reason for hiding this comment

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

How does one "shutdown"/"terminate" a DCC chat?

Copy link
Author

Choose a reason for hiding this comment

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

you don't

jk it's coming.

Copy link
Author

Choose a reason for hiding this comment

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

see close in DCCExchange

@dequis
Copy link
Member

dequis commented Sep 15, 2016

dequis was assigned by mbax

Hold on am i a contributor here or did github extend assignments to non-contributors too? Either way whoa.

@kashike
Copy link
Member

kashike commented Sep 15, 2016

You're a mean one collaborator, Mr. @dequis.

@lol768
Copy link
Member

lol768 commented Sep 15, 2016

Any plans for increasing the (💥 🌠 ☑️ 🎉 ) coverage once the work is stable? Seems it would currently be decreased by pulling.

Lemme know if you want a hand with testing

*
* @see Client#beginDCCChat(User)
*/
default void beginDCCChat() {
Copy link
Member

@Zarthus Zarthus Sep 15, 2016

Choose a reason for hiding this comment

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

I'd consider renaming this to requestDccChat over begin, because the latter sort-of implies that the target-user does not have to confirm it. In this case you're requesting a (connection|chat), not beginning it. Might be useful to have some more opinions on this. Afaik DCC only sends your IP if you accept, right?

(Unless they don't, and I'm just wrong.)

Copy link
Author

Choose a reason for hiding this comment

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

You're right, it is a request.

The protocol goes like this:

A = Initiator
B = Target

A: "/ctcp DCC CHAT chat <bound-ip> <bound-port>"
B: Accept/rejects, on accept connects to <bound-ip>:<bound-port>
A/B: Send CRLF terminated messages.

@octylFractal
Copy link
Author

@lol768 test will be coming as soon as this lands out of WIP 📝

case "DCC":
// Handle targeted DCC chat
// this.fire(new DCCRequestEvent());
// TODO how can the parameters be extracted @mbaxter???
Copy link
Author

Choose a reason for hiding this comment

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

@mbax halp

Copy link
Member

Choose a reason for hiding this comment

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

event.getParameters().get(2) is going to be the CHAT
3 will be ... uh... chat again? wut
4 will be ip
5 will be ... addr? What is your documentation saying? :P

Copy link
Author

Choose a reason for hiding this comment

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

ok thx <3

/**
* Sends a DCC CHAT request to the target.
*
* <p>When the chat is connected, a {@link DCCConnectedEvent} will be fired. If the connection fails,
Copy link
Member

Choose a reason for hiding this comment

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

I wrap javadocs at a max of column 78.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I don't use html tags for this. Seems to work out fine. :3

Copy link
Author

Choose a reason for hiding this comment

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

it won't actually work out fine, at least in Eclipse

Copy link
Member

Choose a reason for hiding this comment

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

Only thing that I care about for 'fine' is the resulting javadocs page.

Copy link
Member

@mbax mbax left a comment

Choose a reason for hiding this comment

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

Fun stuff. Some initial comments made.

/**
* Sends the user a message over DCC chat.
*
* <p>This method won't return until the chat is connected and the message has
Copy link
Member

Choose a reason for hiding this comment

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

Why is this blocking, though? What if you want to send multiple messages over one DCC session?

If we decide to stick with blocking, I'd advocate for the word 'block' or 'blocking' in the javadoc.

Copy link
Author

Choose a reason for hiding this comment

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

It's actually non-blocking currently, I need to update docs.

But I don't understand how blocking affects sending multiple messages?

Copy link
Member

Choose a reason for hiding this comment

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

It affects it only in terms of causing some very sloooooow code. :3

I'm trying to keep KICL non-blocking everywhere I can in the API.

public interface DCCExchange extends Actor {
/**
* Gets the socket address of the local end.
* May return {@link Optional#empty()} if not connected.
Copy link
Member

@mbax mbax Sep 16, 2016

Choose a reason for hiding this comment

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

I'd just leave it as @return the socket address of the local end if connected

Same for the one below

/**
* Gets the connection status.
*
* @return {@code true} if the exchange is connected, otherwise false
Copy link
Member

Choose a reason for hiding this comment

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

I should probably update my stuff to be {@code true} but why do you not do it for false?

Copy link
Author

Choose a reason for hiding this comment

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

idk fixed

default void requestDCCChat() {
this.getClient().requestDCCChat(this);
}

Copy link
Member

Choose a reason for hiding this comment

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

No line here thx

Copy link
Author

Choose a reason for hiding this comment

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

k

case "DCC":
// Handle targeted DCC chat
// this.fire(new DCCRequestEvent());
// TODO how can the parameters be extracted @mbaxter???
Copy link
Member

Choose a reason for hiding this comment

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

event.getParameters().get(2) is going to be the CHAT
3 will be ... uh... chat again? wut
4 will be ip
5 will be ... addr? What is your documentation saying? :P

private final IRCDCCExchange exchange;
private final InternalClient client;
// Only allow one connection per DCC. Weird, I know.
private boolean oneConnection;
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps rename to connected? Or something similar. if(oneConnection) reads oddly to me.

Copy link
Author

Choose a reason for hiding this comment

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

it's happened

}
this.oneConnection = true;
this.exchange.setNettyChannel(channel);
dccConnections.computeIfAbsent(this.client, c -> new ArrayList<>()).add(channel);
Copy link
Member

Choose a reason for hiding this comment

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

Reference this a bit more... clearly. NettyManager.this.dccConnections

Copy link
Author

@octylFractal octylFractal Sep 16, 2016

Choose a reason for hiding this comment

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

It's actually NettyManager.dccConnections. Do you still want fully qualified for outer static fields?

Copy link
Member

Choose a reason for hiding this comment

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

Yes pls

Copy link
Author

Choose a reason for hiding this comment

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

done and done


private NettyManager() {

}

private static synchronized void removeClientConnection(@Nonnull ClientConnection connection, boolean reconnecting) {
connections.remove(connection);
List<Channel> dcc = dccConnections.get(connection.client);
Copy link
Member

Choose a reason for hiding this comment

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

this.dccConnections

Copy link
Author

Choose a reason for hiding this comment

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

NettyManager.dccConnections?

Copy link
Member

@mbax mbax Sep 16, 2016

Choose a reason for hiding this comment

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

Uhhhhh yeah. Missed it being static. This is fine.

@@ -333,6 +446,28 @@ public void initChannel(SocketChannel channel) throws Exception {
return clientConnection;
}

static Runnable connectDCC(InternalClient client, IRCDCCExchange exchange) {
Sanity.nullCheck(eventLoopGroup, "A DCC connection cannot be made without a client");
ServerBootstrap dccBootstrap = new ServerBootstrap();
Copy link
Member

Choose a reason for hiding this comment

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

You can chain this all:

        ChannelFuture future = new ServerBootstrap()
            .channel(NioServerSocketChannel.class);
            .childHandler(new DCCConnection(exchange, client));
            .childOption(ChannelOption.TCP_NODELAY, true)
            .childOption(ChannelOption.SO_KEEPALIVE, true)
            .group(eventLoopGroup)
            .bind(0);

Copy link
Author

Choose a reason for hiding this comment

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

diddly dang done

dccConnections.computeIfAbsent(this.client, c -> new ArrayList<>()).add(channel);

// Outbound - Processed in pipeline back to front.
channel.pipeline().addFirst("[OUTPUT] Output listener", new MessageToMessageEncoder<String>() {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like lots of duplication from above - it should be possible to share most, if not all, of it.

Copy link
Author

Choose a reason for hiding this comment

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

done it

* a {@link DCCConnectedEvent} will be fired. If the connection fails, a
* {@link DCCFailedEvent} will be fired.</p>
*/
void requestDCCChat(@Nonnull User target);
Copy link
Member

Choose a reason for hiding this comment

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

So there's a way to request a new DCC chat, but I see no way to get existing ones.

Copy link
Author

Choose a reason for hiding this comment

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

I don't know if that should be needed? All DCC chats will come through as events so you can keep tabs on them if you want.

However, it currently isn't possible to tell if you have multiple DCC exchanges open to a client or distinguish between them, so maybe we can figure out a way to do that and add a way to get a specific event?

/**
* Represents an exchange using DCC.
*/
public interface DCCExchange extends Actor {
Copy link
Member

Choose a reason for hiding this comment

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

extends Actor, Closeable?

Copy link
Author

Choose a reason for hiding this comment

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

checked and done

@@ -977,6 +978,9 @@ public void ctcp(ClientReceiveCommandEvent event) {
case "FINGER":
reply = "FINGER om nom nom tasty finger";
break;
case "DCC":
handleDccEvent(user, event.getOriginalMessages(), event.getParameters());
Copy link
Member

Choose a reason for hiding this comment

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

this.

} catch (NumberFormatException invalidPort) {
return;
}
fire(new DCCRequestEvent(this.client, originalMessages, dccType, ip, portInt, user));
Copy link
Member

Choose a reason for hiding this comment

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

this.

* @return the readable reason for the failure
*/
public Optional<String> getReason() {
return Optional.ofNullable(this.reason);
Copy link
Member

Choose a reason for hiding this comment

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

Store the Optional in the field instead.

* @return the exception that caused the failure
*/
public Optional<Throwable> getCause() {
return Optional.ofNullable(this.cause);
Copy link
Member

Choose a reason for hiding this comment

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

Store the Optional in the field instead.

this.port = port;
}

public String getType() {
Copy link
Member

Choose a reason for hiding this comment

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

Javadocs?

ActorProvider.this.client.sendCTCPMessage(this.getName(), this.getCTCP());
}

String getType() {
Copy link
Member

Choose a reason for hiding this comment

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

@Nonnull

ActorProvider.this.client.getEventManager().callEvent(new DCCMessageEvent(ActorProvider.this.client, Collections.emptyList(), snapshot(), msg));
}

@Override
Copy link
Member

Choose a reason for hiding this comment

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

@Nonnull

}

@Override
IRCDCCChatSnapshot snapshot() {
Copy link
Member

Choose a reason for hiding this comment

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

@Nonnull

this.connected = actor.connected;
}

@Override
Copy link
Member

Choose a reason for hiding this comment

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

@Nonnull

return Optional.ofNullable(this.localAddress);
}

@Override
Copy link
Member

Choose a reason for hiding this comment

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

@Nonnull

@mbax
Copy link
Member

mbax commented Jan 3, 2017

k then

@octylFractal
Copy link
Author

💀 RIP DCC....

attempt 1.

DCC 2: Electric Boogaloo

Coming soon to a repository near you.

@octylFractal octylFractal mentioned this pull request Jan 4, 2017
7 tasks
@mbax mbax modified the milestone: Future! Apr 27, 2017
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.

DCC Support
7 participants