Opened 6 years ago

Last modified 5 years ago

#15645 new patch

Report failed messages deliveries in conversation window

Reported by: hobarrera Owned by: EionRobb
Milestone: Patches Needing Improvement Component: unclassified
Version: 2.10.7 Keywords:
Cc: ichthuss

Description

When a message cannot be delivered for whatever reason, pidgin shows a popup notifying the user of this. There's a couple of issues with this:

  • It's rather intrusive.
  • The conversation log does not reflect this. When reviewing the log, there's no way to tell if the message was delivered or not.

I belive that it would be nice if pidgin could report the error in the conversation window - probaby with formatting similar to what the ones shown by VoiceVideo? or the "Buddy State Notification" plugin.

This would remove the rather intruvise error window, and help the logs actually reflect the way the conversation took place. It might also allow cleaning up some code (that particular dialog; since I haven't seen it used elsewhere).

Attachments (1)

pidgin-2.10.7-non_delivered_message.patch (3.7 KB) - added by ichthuss 6 years ago.
Patch: Notify user about non-delivered XMPP in chat box instead of pop-up window

Download all attachments as: .zip

Change History (12)

comment:1 Changed 6 years ago by datallah

  • Status changed from new to pending

Which protocol are you seeing this behavior with?

comment:2 Changed 6 years ago by hobarrera

  • Status changed from pending to new

XMPP. In particular, when the other contact is disconnected and the remote server does not support offline messages.

In case you're wanting to replicate this, if you have a gmail contact with server-side logging disabled, talking to him will result in a delivery failure.

comment:3 Changed 6 years ago by datallah

  • Milestone set to Patches Needing Improvement
  • Type changed from enhancement to patch

What does the attached patch do when the conversation window is closed before the error notification is received?

comment:4 follow-up: Changed 6 years ago by ichthuss

It silently ignores this error notification. I think this is the best decision, because showing popup window would still be too intrusive (in some cases error notification can be received long time after original message, and such popup could disturb you while you are typing other message). But if you think that popup notification is neccesary, I can change patch.

Also silent writing this message to log could be good idea, but I'm not sure if it can be done.

comment:5 in reply to: ↑ 4 Changed 6 years ago by hobarrera

I wouldn't mind silently writing to the logs, but if that's impossible, re-opening the conversation window is the best choice. A really important issue with this ticket is that logs should reflect how the conversation took place; and not logging delivery failures (while we're aware of them) makes the log reflect something different.

In short, if I look at a log for last week to make sure I said something to someone, I want to be sure I did, and don't want to have to manually recall if that messages bounced or not.

Changed 6 years ago by ichthuss

Patch: Notify user about non-delivered XMPP in chat box instead of pop-up window

comment:6 Changed 6 years ago by ichthuss

I've changed patch to append an error message to chat log. It appeared that logs returned by purple_log_get_logs() are not writable, so I tweaked log.c a bit to lazily open files for append. Should I document that loggers are expected to allow writes to list()'ed logs?

comment:7 Changed 6 years ago by datallah

The changes to the logging aren't acceptable as currently written - logs can't be assumed to be file based (there are already plugins that implement non-file-based logging.

comment:8 Changed 6 years ago by ichthuss

This patch doesn't assumes logs to be file based, it only assumes that logs returned by list() function are writable (at least the last one). I didn't find anything about it in documentation, but this assumption looks rather correct for me, because there still no notice that logs returned by purple_log_get_logs() are read only, so they are expected to implement any possible action.

comment:9 follow-up: Changed 6 years ago by datallah

The patch adds API to log.h that assumes logs are file based - after looking at the patch more, I realized that you don't need to add the API at all, your proposed change could just be done as part of the logging implementation. You are right that it changes an assumption that has implicitly been made about purple_log_get_logs() being readonly - I'm not sure how I feel about that, but it probably doesn't really matter since I don't think it's the right way to fix this (see below).

I still don't think this is the right approach. It seems pretty hacky to just write stuff to logs and even if that wasn't the case, you're not guaranteed to get the right log. It also doesn't provide any notification to the user that something went wrong (which I think is more important than the logging part).

I think that it probably would be best just to create a new conversation window and write the message to it if there isn't an existing one - this way you get the logging and the UI notification (and no popup unless you've configured new conversations to steal focus).

comment:10 in reply to: ↑ 9 Changed 6 years ago by hobarrera

Replying to datallah:

I think that it probably would be best just to create a new conversation window and write the message to it if there isn't an existing one - this way you get the logging and the UI notification (and no popup unless you've configured new conversations to steal focus).

I strongly agree with this, it covers all points mentioned in the bug report, and also what I believe users would be expecting (ie: it follows the Principle of least astonishment).

comment:11 Changed 5 years ago by rekkanoryo

  • Owner changed from rekkanoryo to EionRobb
Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!