All of your MUCs are belong to Robut #18

Merged
merged 27 commits into from Nov 17, 2011

Projects

None yet

2 participants

@vrish88
vrish88 commented Oct 16, 2011

How does this look for a rough cut at multi-room support?

These are some initial changes to help support robut connecting to multiple MUC rooms. Both with private messages and in rooms work in Hipchat however I still need to update the tests.

It turned out to be a little more work than I was expecting. The Room class keeps the MUC associated with the plugins, so that when a plugin replies it knows which room to send it to. Then I moved private messages into its own class, to keep Connection focused on just connecting to Hipchat.

@justinweiss justinweiss commented on the diff Oct 27, 2011
lib/robut/plugin.rb
@@ -90,8 +93,8 @@ module Robut::Plugin
def fake_message(time, sender_nick, msg)
# TODO: ensure this connection is threadsafe
- plugins = Robut::Plugin.plugins.map { |p| p.new(connection, private_sender) }
- connection.handle_message(plugins, time, sender_nick, msg)
+ plugins = Robut::Plugin.plugins.map { |p| p.new(reply_to, private_sender) }
@justinweiss
justinweiss Oct 27, 2011 owner

Is there a reason you're not passing the connection object in here anymore?

@vrish88
vrish88 Oct 29, 2011

Yup, the connection should definitely be passed in here.

@justinweiss justinweiss commented on an outdated diff Oct 27, 2011
lib/robut/pm.rb
+class Robut::PM
+
+ attr_accessor :connection
+
+ def initialize(connection, rooms)
+ # Add the callback from direct messages. Turns out the
+ # on_private_message callback doesn't do what it sounds like, so I
+ # have to go a little deeper into xmpp4r to get this working.
+ self.connection = connection
+ connection.client.add_message_callback(200, self) do |message|
+ from_room = rooms.any? {|room| room.muc.from_room?(message.from)}
+ if !from_room && message.type == :chat && message.body
+ time = Time.now # TODO: get real timestamp? Doesn't seem like
+ # jabber gives it to us
+ sender_jid = message.from
+ plugins = Robut::Plugin.plugins.map { |p| p.new(self, connection, sender_jid) }
@justinweiss
justinweiss Oct 27, 2011 owner

Plugin's constructor still goes connection, reply_to, sender. Are you sure you're passing the parameters in the correct order here? It looks like replies will go to the root connection class and not the PM class.

@justinweiss justinweiss commented on an outdated diff Oct 27, 2011
lib/robut/room.rb
@@ -0,0 +1,53 @@
+# Handles connections and responses to different rooms.
+class Robut::Room
+ # The MUC that wraps the Jabber Chat protocol.
+ attr_accessor :muc
+
+ # The Robut::Connection that has all the connection info.
+ attr_accessor :connection
+
+ def initialize(connection, room)
+ self.muc = Jabber::MUC::SimpleMUCClient.new(connection.client)
+ self.connection = connection
+
+ # Add the callback from messages that occur inside the room
+ muc.on_message do |time, nick, message|
+ plugins = Robut::Plugin.plugins.map { |p| p.new(self, connection) }
@justinweiss
justinweiss Oct 27, 2011 owner

Same thing here as above.

@justinweiss justinweiss and 1 other commented on an outdated diff Oct 27, 2011
lib/robut/room.rb
+
+ muc.join(room + '/' + connection.config.nick)
+ end
+
+ # Send +message+ to the room we're currently connected to
+ def reply(message, to)
+ muc.send(Jabber::Message.new(muc.room, message))
+ end
+
+ # Sends the chat message +message+ through +plugins+.
+ def handle_message(plugins, time, nick, message)
+ # ignore all messages sent by robut. If you really want robut to
+ # reply to itself, you can use +fake_message+.
+ return if nick == connection.config.nick
+
+ plugins.each do |plugin|
@justinweiss
justinweiss Oct 27, 2011 owner

Can all this stuff be abstracted out somewhere? It seems like it's pretty identical between room/pm.

@vrish88
vrish88 Oct 29, 2011

I pulled this out into a parent class Presence and now PM and Room both inherit from it.

@justinweiss justinweiss and 1 other commented on an outdated diff Oct 27, 2011
lib/robut/connection.rb
self.store = self.config.store || Robut::Storage::HashStore # default to in-memory store only
+ self.config.rooms ||= [self.config.room] # legacy support?
@justinweiss
justinweiss Oct 27, 2011 owner

I usually prefer Array(...) to [...] if you're trying to coerce something into an array. It's a little more robust, and usually results in fewer nil checks down the line.

@vrish88
vrish88 Oct 29, 2011

That's awesome! I honestly didn't know there was a difference between Array.new(...) and [...]

@justinweiss
justinweiss Oct 29, 2011 owner

I've never really used Array.new, but the kernel method Array() is super useful.

Array(nil) => []
Array(my_object) => [my_object]
Array([my_object, my_other_object]) => [my_object, my_other_object]

Basically it converts whatever you throw at it into an array, so you can later #each, #map, etc. on it without having to do any nil checking or anything.

On Oct 29, 2011, at 11:16 AM, Michael Lavrisha wrote:

self.store = self.config.store || Robut::Storage::HashStore # default to in-memory store only
  • self.config.rooms ||= [self.config.room] # legacy support?

That's awesome! I honestly didn't know there was a difference between Array.new(...) and [...]

Reply to this email directly or view it on GitHub:
https://github.com/justinweiss/robut/pull/18/files#r198298

@vrish88
vrish88 Oct 29, 2011

Gah, yeah I'm just foolish. I meant to use Array(...).

@justinweiss
Owner

I really like the way you broke this apart, it's something I'd been meaning to do myself. I just left a few notes, and obviously some tests need to be written, but great start!

@vrish88
vrish88 commented Oct 28, 2011

Awesome, thanks for the review @justinweiss. I've got some time this weekend and I'll respond to your individual comments and refactor!

Look out Hubot, robut is on your tail! ;)

@vrish88
vrish88 commented Oct 29, 2011

All pre-existing tests are now passing. I've also made Room and PM inherit from Presence. This is where handle_message lives and the attr_accessor :connection.

@vrish88
vrish88 commented Oct 29, 2011

@justinweiss what kind of tests were you hoping to see related to multi room functionality?

@justinweiss
Owner

I would at least try different combinations of passing rooms and PMs to a fake plugin, and make sure all the replies go to the right place. The connection itself is kind of hard to test, but now that it's separate, it might also be nice to update the mock connection with any other methods you need and verify that the Room and PM classes are calling the methods you expect them to call in the correct order. Sound reasonable?

@vrish88
vrish88 commented on b050446 Nov 1, 2011

How does this look for a first pass at the room tests?

@justinweiss
Owner

Looks good to me. Sorry this took so long, I never got notified that there were changes waiting!

@justinweiss
Owner

We want to try this out, so I'm gonna merge it. If I run into any other issues, I'll let you know.

@justinweiss justinweiss merged commit 680d0e7 into justinweiss:master Nov 17, 2011
@vrish88
vrish88 commented Nov 17, 2011

Awesome! Yeah, we've been wanting this for a while too.
On Nov 17, 2011, at 1:45 PM, Justin Weiss wrote:

We want to try this out, so I'm gonna merge it. If I run into any other issues, I'll let you know.


Reply to this email directly or view it on GitHub:
#18 (comment)

@vrish88
vrish88 commented Nov 21, 2011

@justinweiss how is this looking?

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