search

meebey / SmartIrc4net

IRC C# Library
http://www.meebey.net/projects/smartirc4net/
Other
126 stars 52 forks source link

Initial Twitch support #34

Closed djkaty closed 8 years ago

djkaty commented 9 years ago

This is a roll-up of the previous ROOMSTATE and USERSTATE support and the other Twitch tweaks that I hadn't committed yet, re-formulated as a derived class.

So a few matters arising from the implementation depending how you want to do it.

  1. A ROOMSTATE or USERSTATE generates a ReceiveType of ReceiveType.Custom and a new custom type string which is determined in TwitchIrcClient and included in IrcMessageData (I did it this way because you can't inherit an Enum), but this involves overriding _GetMessageType which seems like a duplication of effort over _GetCustomMessageType. Should we just ditch the first one and keep it as ReceiveType.Unknown?
  2. Not sure whether you want these methods to be overridden or new ones created and the existing ones renamed to eliminate the use of base._* in derived classes.
  3. Do you want all the Event* and _EventRPL* style messages standardized to be protected virtual (or a new set of stubs made)?

Katy.

meebey commented 9 years ago

To be honest, the MessageType thing was a mistake in the library and should go away (or be ignored for now) :-D It is a leftover from the Net_SmartIRC code that I ported from PHP to C#

It was only there because you couldn't easily tell why you received a callback in the PHP code... In C# you know which event was raised as your callback is specific to the event.

meebey commented 9 years ago

For the other 2 things, I will have to think about it.

meebey commented 9 years ago

What you did in TwitchIrcClient is basically what I had in mind, that looks good, but I try to get rid of the "legacy" stuff

djkaty commented 9 years ago

Okay, I can remove the Message stuff from the Twitch code easily and you can presumably remove it altogether from the main branch. I'll wait for you to let me know on the other items before I make any more changes or merge any of the other new features.

djkaty commented 9 years ago

(I couldn't find anywhere appropriate to post this on GitHub, so...)

While you are thinking, regarding WebSockets: currently the Connect() takes an extra optional bool specifying whether to negotiate a WebSocket connection or not. The code is lightly peppered in "if (_IsWebSocket)" type stuff (mainly in Connect(), ReadLine() and WriteLine()). Before I make a branch for this, do you prefer:

  1. using optional argument in IrcConnection as above (current implementation),
  2. DI using an interface (eg. something like IChatFrameLayer, with implementations like IRCFrameLayer and WebSocketFrameLayer which implement Connect, ReadLine and WriteLine and are passed to the IrcConnection constructor), or:
  3. Deriving IrcConnection and eg. a new WebSocketConnection from an abstract class that implements any non-frame specific stuff.

I slightly favour option 2 but I'll glue it together however you like :)

meebey commented 9 years ago

To discuss changes best is to create a new issue on GitHub, else we will mix the Twitch review with unrelated changes. We had already issues with alternative socket handling because there is currently no abstraction for the socket handling. Another issue is that someone else already worked on trying to streamline the socket handling code since SmartIrc4net has currently some code duplication there, but it wasn't finished/merge yet see #31 but it would be a good idea to finish that one first, else your WebSocket will conflict with that.

meebey commented 9 years ago

The WebSockets issue can be found in #35 now

djkaty commented 9 years ago

I've reviewed the ReceiveType and ReplyCode types/parameters and although they can be removed, i think for clarity of the source code and the user there is no harm leaving them in. I propose therefore that derived classes of IrcClient just ignore them and the base class can set them to Unknown by default. Do you agree?

meebey commented 9 years ago

Yes, the ReceiveType can stay with Unknown by default. This field should be marked as [Deprecated] so that people check for ReplyCode instead, which is the official IRC way to know what the reply is about if there is no high-level event available in IrcClient for that.