(In reply to comment #0)
> why is polari not working with /msg NickServ on irc.freenode.net?

Because /msg hasn't been implemented yet :-)


> try this:
> /msg NickServ help commands 

For now, you can do
 /query NickServ
 help commands

Does not work for Quakenet and the Q bot which needs to execute :
/msg Q@CServe.quakenet.org AUTH username password
also tried with :
/query Q@CServe.quakenet.org AUTH username password

(tried on irssi and it worked)

Today, I didn't found any solution to auth on Quakenet....

Created attachment 305130 [details] [review]
ircParser: Support for /msg command in entry area

Attached patch adds basic support for /msg. What do you think of the approach?

A thing which might be nice to also support could be to have multiple recipients by fx entering "/msg john,paula,nick this is text".

(In reply to Bastian Ilsø from comment #3)
> Attached patch adds basic support for /msg. What do you think of the
> approach?

Not a huge fan. Traditionally, "/query user" is used to open a conversation with user, and "/msg user foo bar baz" to send a one-off message to user - I don't think dumping the private message into the active channel as I've seen in some UIs works very well either, but I'd expect "/msg" to at least not focus the new conversation.
(I'm also not too convinced about adding "random" parameters to requestChannel() - a more generic approach could be to take an optional callback parameter that's called with the newly created channel or null in case of an error)


> A thing which might be nice to also support could be to have multiple
> recipients by fx entering "/msg john,paula,nick this is text".

How useful is this though? I can think of use cases, but it's not really something I've ever missed. Also, I'm not aware of any IRC client (either graphical or text) that does this, so it'd probably end up as Easter egg anyway ...

Recently I spent a day trying to figure out how to register a nick to Freenode (as I had to join a channel which requires registered nicknames) because neither Empathy nor Polari had a /msg command, command suggested by freenode to send password and email for an account. /privmsg didn't work either.

Eventually someone recommended to use irssi and I managed to do it.

I'm surprised that this basic feature is not supported by these two linux clients. I've surely missed something, right?

Thanks for this nice app

Unless I'm mistaken, empathy used to have a /msg command.  It seems to have swapped to polari as a client recently.

(In reply to Berend De Schouwer from comment #6)
> Unless I'm mistaken, empathy used to have a /msg command.  It seems to have
> swapped to polari as a client recently.

It still does, I used it today. It might have been implemented within the last couple of years.

Florian, Bastian, what do you think would be the best approach to handle this? \msg foo hi send foo hi and that's it?

The command should be used like this:
/msg <username> <message>

As florian wrote in comment 4 it should send a one-off message to user. by "one-off" my understanding is that the message is sent but without opening and focusing any private messaging window.

(In reply to Bastian Ilsø from comment #9)
> As florian wrote in comment 4 it should send a one-off message to user. by
> "one-off" my understanding is that the message is sent but without opening
> and focusing any private messaging window.

Not opening the conversation window is kinda hard. I think not focusing it is good enough.

Created attachment 319725 [details] [review]
chatroomManager: Don't set active room in handlechannels function

We already set active room when joining a room, don't set
active room again in handlechannels

Created attachment 319726 [details] [review]
app: Add optional callback parameter to request channel

Add optional callback paramter with channel id
to request channel function to get the newly created
channel. Also use ensure_and_observe channel to get
the channel

Created attachment 319727 [details] [review]
app: Change message user signal to query user signal

Change name of message user signal for query
command to query user signal in functions.
We will be using message user signal for message
command soon.

Created attachment 319728 [details] [review]
ircParser: Support for /msg command in entry area

Adds support for /msg command which sends "one-off"
message to user without focussing on the coversation
window

Review of attachment 319725 [details] [review]:

No. When telepathy tells us to present a channel, we should do that. Setting the active channel on 'join' for channels we request ourselves is just a shortcut to make the interface more responsive, it's not an excuse for ignoring external channels (for example the user clicking on a chat notification in gnome-shell to present it in polari)

Review of attachment 319726 [details] [review]:

::: src/application.js
@@ +277,3 @@
         this._pendingRequests[roomId] = requestData;
 
+        this._ensureChannel(requestData, callback);

You should just save callback in requestData instead of modifying every function involved

@@ +311,3 @@
+            let channel = req.ensure_and_observe_channel_finish(res);
+            if (callback)
+                callback(channel);

I don't like how the error handling meant for channel creation now applies to callback() as well - I'd prefer:

 let account = req.account;
 let channel = null;

 try {
    channel = req.ensure_and_observe_channel_finish(res);
 } catch (...)
 }

 if (requestData.callback)
     requestData.callback(channel);

(this means not calling the callback when we get disconnected due to a network failure, but that's not less random than ignoring the callback on cancel - considering that the latter patch doesn't actually handle the error case, I'd consider changing the condition to "requestData.callback && channel")

@@ +350,3 @@
         let [accountPath, channelName, time] = parameter.deep_unpack();
         this._requestChannel(accountPath, Tp.HandleType.ROOM,
+                             channelName, time, null);

Just don't set callback, and it'll be undefined

@@ +360,3 @@
         let [accountPath, contactName, time] = parameter.deep_unpack();
         this._requestChannel(accountPath, Tp.HandleType.CONTACT,
+                             contactName, time, null);

Dto.

Review of attachment 319727 [details] [review]:

You are changing an action name and a method - no signal involved anywhere ;-)

Also note that 'message-user' is named after the menu item in the (+) menu, not the /command. It's probably less invasive to just add 'ms' to the existing signature (if that works with GApplication, never tried it - otherwise could take the empty string '' to mean "no message") instead of copying action and methods

Review of attachment 319728 [details] [review]:

Nit: "coversation" in commit message

::: src/application.js
@@ +368,3 @@
+    _sendMessage: function(channel, message) {
+        let TpMessage = Tp.ClientMessage.new_text( Tp.ChannelTextMessageType.NORMAL,
+	                                               message);

Style nits: no space after '(', messed up indentation

::: src/chatroomManager.js
@@ +201,3 @@
+
+        let room = this._ensureRoom(account, channelName, Tp.HandleType.CONTACT);
+        let [present, ] = Tp.user_action_time_should_present(time);

Unused

::: src/ircParser.js
@@ +189,3 @@
+                                                   nick,
+                                                   message,
+                                                   Utils.getTpEventTime() ]));

You should use Tp.USER_ACTION_TIME_NOT_USER_ACTION to indicate that the channel should not be presented

Created attachment 320000 [details] [review]
app: Add optional callback parameter to request channel

Add optional callback paramter to request channel function
to get the newly created channel. Also use ensure_and_observe_channel
function to get the channel as return value.

Created attachment 320001 [details] [review]
ircParser: Support for /msg command in entry area

Adds support for /msg command which sends "one-off"
message to user without focussing on the conversation
window.

Created attachment 320002 [details] [review]
app: Add optional callback parameter to request channel

Add optional callback paramter to request channel function
to get the newly created channel. Also use ensure_and_observe_channel
function to get the channel as return value.

(In reply to Florian Müllner from comment #16)
> Review of attachment 319726 [details] [review] [review]:
>  try {
>     channel = req.ensure_and_observe_channel_finish(res);
>  } catch (...)
>  }
> 
>  if (requestData.callback)
>      requestData.callback(channel);
> 
> (this means not calling the callback when we get disconnected due to a
> network failure, but that's not less random than ignoring the callback on
> cancel - considering that the latter patch doesn't actually handle the error
> case, I'd consider changing the condition to "requestData.callback &&
> channel")
> 

Checking for channel!=null in latter patch. Perhaps in future we might want to handle the error cases and propagate them to user?

Review of attachment 320002 [details] [review]:

LGTM

Review of attachment 320001 [details] [review]:

Yup

Review of attachment 320001 [details] [review]:

Pushed with commit 687614e5

Review of attachment 320002 [details] [review]:

Pushed with commit 7a2bfb3e