Closed Bug 954800 Opened 10 years ago Closed 10 years ago

Inform the user when attempting to send a message to an offline nick

Categories

(Chat Core :: IRC, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: clokep)

References

Details

(Whiteboard: [1.2-blocking][regression])

Attachments

(1 file, 4 obsolete files)

*** Original post on bio 1366 at 2012-04-06 20:59:00 UTC ***

This is an important subset of the 401 error message. 

To fix this, bug 954049 (bio 613) would be helpful (so the conversation knows the conversation partner is offline) but not sufficient.
*** Original post on bio 1366 at 2012-04-06 21:13:43 UTC ***

Failing to send a message and not notifying the user about it even though the server returned an error is a serious issue -> blocking 1.2 as it's a regression compared to the libpurple behavior.
Severity: normal → major
Whiteboard: [1.2-blocking][regression]
Attached patch Minimal patch (obsolete) — Splinter Review
*** Original post on bio 1366 as attmnt 1330 at 2012-04-11 16:31:00 UTC ***

This minimalistic patch fixes the regression for /msg (and when sending messages in a DM). As there is already always an error message in the server tab, one could take something like this as a stopgap if noone submits a more complete solution before 1.2...
*** Original post on bio 1366 at 2012-04-13 00:15:47 UTC ***

This is a regression from bug 953944 (bio 507).
Blocks: 953944
*** Original post on bio 1366 as attmnt 1401 at 2012-04-25 01:46:00 UTC ***

This is a patch to handle all possible responses from sending a private message, it probably needs a bit of work...but it's a start. :)
Attachment #8353154 - Flags: review?(bugzilla)
Comment on attachment 8353083 [details] [diff] [review]
Minimal patch

*** Original change on bio 1366 attmnt 1330 at 2012-04-25 01:46:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353083 - Attachment is obsolete: true
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8353154 [details] [diff] [review]
Patch to handle all responses from PRIVMSG

*** Original change on bio 1366 attmnt 1401 at 2012-04-25 09:04:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353154 - Attachment is patch: true
Attachment #8353154 - Attachment mime type: application/octet-stream → text/plain
*** Original post on bio 1366 at 2012-04-25 13:06:30 UTC ***

I am a bit confused by this (maybe because I don't understand enough about IRC).

_pendingMessage is never deleted unless an error message is received. So is its only purpose not to respond with two error messages to the same original message? Why is it only tested in the 401 handler, but not in the other handlers? (Is it clear in which order these error messages will arrive?)

Sending the terse error.* error strings to normal conversation/channel tabs is not ideal. The user should get something a little more verbose and helpful.
e.g. I have no idea what "is not a unique target" means or what do do about it without googling.
*** Original post on bio 1366 at 2012-04-25 13:46:43 UTC ***

(In reply to comment #5)
> _pendingMessage is never deleted unless an error message is received. So is its
> only purpose not to respond with two error messages to the same original
> message?
Yes, we don't receive any "success" message back so we can't delete it. It allows us to not show error messages when requesting WHOIS information, etc.

> Why is it only tested in the 401 handler, but not in the other
> handlers? (Is it clear in which order these error messages will arrive?)
The other (i.e. not 401) handlers can ONLY occur from sending a PRIVMSG, so we know where the error is from and can just act on the error. 401 needs to test whether we received it from this case or from sending a WHOIS, etc.

You should only ever receive one of these error messages, not multiple.

The _pendingMessage flag is cleared in the 404 one and should be cleared in the 407 one. The 411 - 415 ones...we don't know what channel/nick actually caused the error (and in some cases there isn't even a particular one: like no target given).

> Sending the terse error.* error strings to normal conversation/channel tabs is
> not ideal. The user should get something a little more verbose and helpful.
OK...I don't see how much more detail we can give though: the errors returned from the server kind of suck.

> e.g. I have no idea what "is not a unique target" means or what do do about it
> without googling.
I agree with your example, but a longer message would be "%S is not a unique user@host or shortname or you tried to join too many channels at once."
Comment on attachment 8353154 [details] [diff] [review]
Patch to handle all responses from PRIVMSG

*** Original change on bio 1366 attmnt 1401 at 2012-04-25 15:14:44 UTC ***

> The _pendingMessage flag is cleared in the 404 one and should be cleared in the
> 407 one. 

OK, so that is missing from 407.

> The 411 - 415 ones...we don't know what channel/nick actually caused
> the error (and in some cases there isn't even a particular one: like no target
> given).

I took 411-415 errors to be those which really should not occur when using JS-IRC normally unless there is some bug or the user explicitly does something strange with IRC commands. Is that right?
 
> > Sending the terse error.* error strings to normal conversation/channel tabs is
> > not ideal. The user should get something a little more verbose and helpful.
> OK...I don't see how much more detail we can give though: the errors returned
> from the server kind of suck.

Yes :(
 
> > e.g. I have no idea what "is not a unique target" means or what do do about it
> > without googling.
> I agree with your example, but a longer message would be "%S is not a unique
> user@host or shortname or you tried to join too many channels at once."

Well, that's already much more helpful :)

Other than that it seems to work well.

UI issue: If you are going to send error-styled messages (rather than system messages) to normal conversations, you should file a bug to add styling for these to the default message styles. They are currently really ugly in Bubbles at any rate.
Attachment #8353154 - Flags: review?(bugzilla) → review-
*** Original post on bio 1366 at 2012-04-26 00:56:23 UTC ***

(In reply to comment #7)
> Comment on attachment 8353154 [details] [diff] [review] (bio-attmnt 1401) [details]
> Patch to handle all responses from PRIVMSG
> UI issue: If you are going to send error-styled messages (rather than system
> messages) to normal conversations, you should file a bug to add styling for
> these to the default message styles. They are currently really ugly in Bubbles
> at any rate.

Flo, what's our real thought on this? We /have/ an error flag, but it's unstyled. We generally seem to use system messages instead of error messages though. Thoughts?
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1366 as attmnt 1405 at 2012-04-26 00:58:00 UTC ***

Meets the review comments, whether to use an error or system message is still up in the air.
Attachment #8353158 - Flags: review?(bugzilla)
Comment on attachment 8353154 [details] [diff] [review]
Patch to handle all responses from PRIVMSG

*** Original change on bio 1366 attmnt 1401 at 2012-04-26 00:58:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353154 - Attachment is obsolete: true
*** Original post on bio 1366 at 2012-04-26 10:17:46 UTC ***

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 8353154 [details] [diff] [review] (bio-attmnt 1401) [details]
> > Patch to handle all responses from PRIVMSG
> > UI issue: If you are going to send error-styled messages (rather than system
> > messages) to normal conversations, you should file a bug to add styling for
> > these to the default message styles. They are currently really ugly in Bubbles
> > at any rate.
> 
> Flo, what's our real thought on this? We /have/ an error flag, but it's
> unstyled. We generally seem to use system messages instead of error messages
> though. Thoughts?

I think the last time we discussed this I suggested using both the "system" and the "error" flags for these messages. We could also change this check: http://lxr.instantbird.org/instantbird/source/chat/modules/imThemes.jsm#398

Adding some CSS for it in some default message themes also seems like a good idea, but completely unrelated to this bug.
Comment on attachment 8353158 [details] [diff] [review]
Patch v2

*** Original change on bio 1366 attmnt 1405 at 2012-04-26 11:03:40 UTC ***

There is a typo in line 1038, ERR_CANNONTSENDTOCHAN. Sorry I didn't spot this earlier.

Wouldn't the comment explaining _pendingMessage be clearer/more specific if it was something along the lines of "This is set to true after a message is sent to stop 401 ERR_NOSUCHNICK writing an error message to the conversation when there was already another error message"? Or are you going to check for _pendingMessage in other error messages as well in later bugs?

> I think the last time we discussed this I suggested using both the "system" and
> the "error" flags for these messages. We could also change this check:
> http://lxr.instantbird.org/instantbird/source/chat/modules/imThemes.jsm#398

I think this is a good idea - it would ensure the error messages get styled as system messages by message styles which don't support the error flag. Then you could add CSS for the error flag to the default styles if you want the error messages to look different. It might be better to do this in separate bugs though; if you agree, please file them ;)

Otherwise r+ :)
Attachment #8353158 - Flags: review?(bugzilla) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
*** Original post on bio 1366 as attmnt 1408 at 2012-04-27 00:43:00 UTC ***

Meets the review requirements. I changed the comment from exactly what you wrote, but it's similar.
Attachment #8353161 - Flags: review?(bugzilla)
Comment on attachment 8353158 [details] [diff] [review]
Patch v2

*** Original change on bio 1366 attmnt 1405 at 2012-04-27 00:43:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353158 - Attachment is obsolete: true
Comment on attachment 8353161 [details] [diff] [review]
Patch v3

*** Original change on bio 1366 attmnt 1408 at 2012-04-27 08:22:42 UTC ***

> I changed the comment from exactly what you wrote, but it's similar.
s/similar/better :)

Thanks for tackling this!
Attachment #8353161 - Flags: review?(bugzilla) → review+
Hardware: x86 → All
Whiteboard: [1.2-blocking][regression] → [1.2-blocking][regression][checkin-needed]
Comment on attachment 8353161 [details] [diff] [review]
Patch v3

*** Original change on bio 1366 attmnt 1408 at 2012-04-29 22:52:09 UTC ***

>diff --git a/chat/locales/en-US/irc.properties b/chat/locales/en-US/irc.properties
>--- a/chat/locales/en-US/irc.properties
>+++ b/chat/locales/en-US/irc.properties
>@@ -127,16 +127,19 @@ error.noChannel=There is no channel: %S.
> error.tooManyChannels=Cannot join %S; you've joined too many channels.
> #    %1$S is your new nick, %2$S is the kill message from the server.
> error.nickCollision=Nick already in use, changing nick to %1$S [%2$S].
> error.banned=You are banned from this server.
> error.bannedSoon=You will soon be banned from this server.
> error.mode.wrongUser=You cannot change modes for other users.
> error.noSuchNick=There is no nickname or channel: %S
> error.wasNoSuchNick=There was no nickname: %S

Why isn't there a localization note before these 2 messages explaining what %S is?

>+#    %S is the channel name.
>+error.cannotSendToChannel=You cannot send messages to %S.
>+error.nonUniqueTarget=%S is not a unique user@host or shortname or you have tried to join too many channels at once.

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm

>       // Error messages, Implement Section 5.2 of RFC 2812
>     "401": function(aMessage) { // ERR_NOSUCHNICK
>       // <nickname> :No such nick/channel
>       // Can arise in response to /mode, /invite, /kill, /msg, /whois.
>-      // TODO Only handled in the conversation for /whois so far.
>+      // TODO Handled in the conversation for /whois and /mgs so far.
>+      if (this.hasConversation(aMessage.params[1])) {
>+        // If the conversation exists and we just sent a message from it, then
>+        // notify that the user is offline.
>+        let conv = this.getConversation(aMessage.params[1]);
>+        if (conv._pendingMessage) {
>+          conv.writeMessage(aMessage.servername,
>+                            _("error.noSuchNick", aMessage.params[1]),
>+                            {error: true, system: true});

The message in error.noSuchNick seems right for the server tab, but quite confusing in the case of it being displayed in a private conversation. Especially if the user is sure the nick is right; because incoming messages have been received in that conversation already.


>+      let conv = this.getConversation(aMessage.params[1]);
>+      conv.writeMessage(aMessage.servername,
>+                        _("error.cannotSendToChannel", aMessage.params[1]),
>+                        {error: true, system: true});
>+      delete conv._pendingMessage;
>+      return true;

This pattern seems duplicated several times with only the name of the localized string that changes. Is there any easy way to factor our this error handling code?

Thanks for working on this bug! :)
Attachment #8353161 - Flags: review-
Attached patch Patch v4Splinter Review
*** Original post on bio 1366 as attmnt 1422 at 2012-05-01 01:34:00 UTC ***

This now has different strings for a channel vs. a nickname. It also abstracts out some of the error codes. Let me know what you think. :)
Attachment #8353174 - Flags: review?(florian)
Comment on attachment 8353161 [details] [diff] [review]
Patch v3

*** Original change on bio 1366 attmnt 1408 at 2012-05-01 01:34:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353161 - Attachment is obsolete: true
Comment on attachment 8353174 [details] [diff] [review]
Patch v4

*** Original change on bio 1366 attmnt 1422 at 2012-05-01 09:01:36 UTC ***

Seems good, thanks (note: I haven't tried the patch, just read it).
Attachment #8353174 - Flags: review?(florian) → review+
*** Original post on bio 1366 at 2012-05-01 11:01:18 UTC ***

https://hg.instantbird.org/instantbird/rev/31f19195a359
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [1.2-blocking][regression][checkin-needed] → [1.2-blocking][regression]
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.