Closed Bug 954986 Opened 10 years ago Closed 10 years ago

IRC should notify the user when messages couldn't be sent

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: aleth)

Details

Attachments

(2 files, 5 obsolete files)

*** Original post on bio 1554 by scrapmachines AT gmail.com at 2012-06-27 12:44:00 UTC ***

Sometimes, when I try to send a message on IRC chatroom, the message is posted in the conversation window, but instantly, IB displays a line that you are disconnectd, and then it tries to reconnect. After checking I come to know that message is not received by the server, while on the conversation window, it appears like it was sent.

So if a message is not sent in these situations, then the message should remain in the textbox, or come back to the textbox.
*** Original post on bio 1554 at 2012-06-27 12:50:20 UTC ***

It was mentioned on IRC that this happens when there's this error in the error console:
14:10:41 - Optimizer: Error: Connection reset.
14:10:41 - Optimizer: Source File: jar:file:///C:/Program%20Files%20(x86)/Instantbird/omni.ja!/components/irc.js
14:10:41 - Optimizer: Line: 502

I don't think the message should stay in the textbox, but if possible, we should add a system message indicating that the message couldn't be sent.
Summary: If a message is not sent to the IRC server, it should remain in the text box → IRC should notify the user when messages couldn't be sent
*** Original post on bio 1554 by scrapmachines AT gmail.com at 2012-06-27 12:51:43 UTC ***

Sometimes, its very difficult to type again, what you wanted to send. So it should be displayed somewhere so that one can copy paste it again
*** Original post on bio 1554 at 2012-06-27 12:54:24 UTC ***

(In reply to comment #2)
> Sometimes, its very difficult to type again, what you wanted to send. So it
> should be displayed somewhere so that one can copy paste it again

You just said it's displayed in the conversation, so it's easy to copy/paste.

And you may be interested in bug 954212 (bio 778) (and see the bug 954212 (bio 778) comment 1 :)).
*** Original post on bio 1554 at 2012-10-17 11:52:00 UTC ***

Does this (comment #1) still happen? Do we have STR?
*** Original post on bio 1554 at 2012-10-17 11:59:49 UTC ***

(In reply to comment #4)
> Does this (comment #1) still happen? Do we have STR?

Yes this is still an issue.

The solution (in my mind) is to ping the server every x minutes (2? 3?), the server should send a pong within a certain number of seconds after that (10? 20?). It's also, we can then reduce the socket timeout since we know data will be being sent every x minutes.
*** Original post on bio 1554 by scrapmachines AT gmail.com at 2012-10-17 12:01:25 UTC ***

(In reply to comment #4)
> Does this (comment #1) still happen? Do we have STR?

Yes, it still happens.

Whenever my internet is disconnected, and I am in the middle of typing a message, so unknowingly I press enter, and after a second IB tells be that your account is disconnected, but it does not tells me that the last message was sent or not.
*** Original post on bio 1554 at 2012-10-17 12:13:58 UTC ***

Well then I'm confirming it! ;)
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
*** Original post on bio 1554 at 2013-02-10 15:43:18 UTC ***

(In reply to comment #5)
> The solution (in my mind) is to ping the server every x minutes (2? 3?), the
> server should send a pong within a certain number of seconds after that (10?
> 20?). It's also, we can then reduce the socket timeout since we know data will
> be being sent every x minutes.

Since something like this has now been implemented (bug 955245 (bio 1812)), is this still an issue?
*** Original post on bio 1554 at 2013-02-10 19:48:28 UTC ***

04:52:25 PM - Optimizer: I think the bug is still valid, as even though I get a message saying that "Your account is not connected"
04:52:56 PM - Optimizer: I have no clue whether the message sent just before the "Your account is not connected" was received by the server
04:53:51 PM - Optimizer: although most of the time I take it that the message was not sent and that is the case only
Attached patch WIP for testing (obsolete) — Splinter Review
*** Original post on bio 1554 as attmnt 2226 at 2013-02-10 19:52:00 UTC ***

Optimizer, could you please test this patch (it's possible to apply the changes inside omni.jar, no need to recompile IB) and see if it helps with the remaining problem, as I can't reproduce.
Attachment #8353989 - Flags: feedback?
Comment on attachment 8353989 [details] [diff] [review]
WIP for testing

*** Original change on bio 1554 attmnt 2226 at 2013-02-10 20:17:50 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353989 - Attachment description: Patch → WIP for testing
Comment on attachment 8353989 [details] [diff] [review]
WIP for testing

*** Original change on bio 1554 attmnt 2226 at 2013-02-11 02:49:13 UTC ***

I don't particularly like this patch. I think it's a band-aid over the real issue. Unfortunately, I can't really think of a better way to do this...
Attachment #8353989 - Flags: feedback? → feedback+
Comment on attachment 8353989 [details] [diff] [review]
WIP for testing

*** Original change on bio 1554 attmnt 2226 at 2013-05-30 09:58:58 UTC ***

I agree it's a bandaid, but I ran into this issue yesterday and it's annoying when it happens (I only saw from the log that a couple of messages were never sent). We can always improve the UX in a followup.
Attachment #8353989 - Flags: review?(clokep)
Comment on attachment 8353989 [details] [diff] [review]
WIP for testing

*** Original change on bio 1554 attmnt 2226 at 2013-05-30 10:44:06 UTC ***

To summarize our IRC conversation...

There's on way this will work, aMessage in the caller is just a string, but aMessage in conversationErrorMessage is an incoming message object.
Attachment #8353989 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1554 as attmnt 2464 at 2013-05-30 13:52:00 UTC ***

This only adds an error message when the sent message was a PRIVMSG, as in general we don't know which conversation to display the error message in (the usual problem).
Attachment #8354231 - Flags: review?(clokep)
Comment on attachment 8353989 [details] [diff] [review]
WIP for testing

*** Original change on bio 1554 attmnt 2226 at 2013-05-30 13:52:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353989 - Attachment is obsolete: true
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8354231 [details] [diff] [review]
Patch

*** Original change on bio 1554 attmnt 2464 at 2013-06-14 10:36:06 UTC ***

I don't love having sendRawMessage return a boolean, but I can't think of a better way to do it. Please add a comment saying that it returns whether it was successful.

Would it make more sense to also bubble this boolean out of sendMessage? This would allow sendMsg (in the conversation) to check the boolean and print directly in the conversation without having to check if it's a PRIVMSG and refind the conversation from aParams[0]. What do you think?
Attachment #8354231 - Flags: review?(clokep) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1554 as attmnt 2484 at 2013-06-14 11:29:00 UTC ***

Yes, that's a better way of doing it.
Attachment #8354251 - Flags: review?(clokep)
Comment on attachment 8354231 [details] [diff] [review]
Patch

*** Original change on bio 1554 attmnt 2464 at 2013-06-14 11:29:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354231 - Attachment is obsolete: true
Comment on attachment 8354251 [details] [diff] [review]
Patch

*** Original change on bio 1554 attmnt 2484 at 2013-06-14 11:57:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354251 - Flags: review?(clokep) → review+
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1554 as attmnt 2485 at 2013-06-14 12:15:00 UTC ***

Moves the !target.length check to the various commands.
Attachment #8354252 - Flags: review?(clokep)
Comment on attachment 8354251 [details] [diff] [review]
Patch

*** Original change on bio 1554 attmnt 2484 at 2013-06-14 12:15:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354251 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1554 as attmnt 2486 at 2013-06-14 12:23:00 UTC ***

Added comment.
Attachment #8354253 - Flags: review?(clokep)
Comment on attachment 8354252 [details] [diff] [review]
Patch

*** Original change on bio 1554 attmnt 2485 at 2013-06-14 12:23:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354252 - Attachment is obsolete: true
Attachment #8354252 - Flags: review?(clokep)
Attached patch PatchSplinter Review
*** Original post on bio 1554 as attmnt 2487 at 2013-06-14 12:28:00 UTC ***

Adds hyphen.
Attachment #8354254 - Flags: review?(clokep)
Comment on attachment 8354253 [details] [diff] [review]
Patch

*** Original change on bio 1554 attmnt 2486 at 2013-06-14 12:28:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354253 - Attachment is obsolete: true
Attachment #8354253 - Flags: review?(clokep)
Comment on attachment 8354254 [details] [diff] [review]
Patch

*** Original change on bio 1554 attmnt 2487 at 2013-06-14 12:30:31 UTC ***

Looks good, sorry for the long review for this. Let's give this a try in the nightlies.
Attachment #8354254 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 1554 at 2013-06-15 04:11:05 UTC ***

Thanks for fixing this! http://hg.instantbird.org/instantbird/rev/f12655ab11a9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
*** Original post on bio 1554 at 2013-06-17 10:22:16 UTC ***

This is causing xpcshell test failures, any chance you can fix that?

http://buildbot.instantbird.org/builders/win32-nightly-default/builds/977/steps/shell_3/logs/stdio
Attached patch Fix test bustageSplinter Review
*** Original post on bio 1554 as attmnt 2492 at 2013-06-17 10:34:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354259 - Flags: review?(aleth)
*** Original post on bio 1554 at 2013-06-18 10:17:51 UTC ***

(In reply to comment #23)
> Created attachment 8354259 [details] [diff] [review] (bio-attmnt 2492) [details]
> Fix test bustage

http://hg.instantbird.org/instantbird/rev/83ddae231f8f
Comment on attachment 8354259 [details] [diff] [review]
Fix test bustage

*** Original change on bio 1554 attmnt 2492 at 2013-06-19 08:05:31 UTC ***

Thanks for fixing this! Sorry I missed it.
Attachment #8354259 - Flags: review?(aleth) → review+
You need to log in before you can comment on or make changes to this bug.