profanity-im / profanity

Ncurses based XMPP client
https://profanity-im.github.io/
Other
1.33k stars 187 forks source link

Improve string convertion error handling #1933

Closed H3rnand3zzz closed 1 year ago

H3rnand3zzz commented 1 year ago

A bit hard to reproduce. You need to add a plugin with the following code (or any other plugin with prof_pre_chat_message_display present, such as emoticons.py):

def prof_pre_chat_message_display(barejid, resource, message):
    return message

and then receive/send message with incorrectly encoded character, it can be any invalid UTF8 symbol. You'll see error in console, errors don't represent actual errors in plugins, but rather Profanity's shortcoming, though it make appear so that the problem is in plugin.

Actual proper handling would likely be using y instead of s format (see reference)

s (str or None) [const char *] Convert a null-terminated C string to a Python str object using 'utf-8' encoding. If the C string pointer is NULL, None is used.

to

y (bytes) [const char *] This converts a C string to a Python bytes object. If the C string pointer is NULL, None is returned.

since there is a problem with s:

s (str) [const char *] Convert a Unicode object to a C pointer to a character string. A pointer to an existing string is stored in the character pointer variable whose address you pass. The C string is NUL-terminated. The Python string must not contain embedded null code points; if it does, a ValueError exception is raised. Unicode objects are converted to C strings using 'utf-8' encoding. If this conversion fails, a UnicodeError is raised.

In python such problem can be handled using errors='ignore' in bytes.decode or in a more sophisticated manner, depending on needs and realization.

H3rnand3zzz commented 1 year ago

Thanks for the good description. Actually I like it so much that I think maybe it would be good to add the description of the PR also at the end of the already existing commit description? I think combined they give a good picture of whats going on.

It looks a bit too long for a commit description and formatting is probably going to be broken, so it would be better to just reference this PR link in the commit description + fix typos there. In a few hours I am going to do just that.

sjaeckel commented 1 year ago

It looks a bit too long for a commit description and formatting is probably going to be broken, so it would be better to just reference this PR link in the commit description + fix typos there

IMO there's no "too long" commit message if it's reasonable.

GH is not guaranteed to stay as long as the commit history and I'd also prefer to have it in the commit message.

H3rnand3zzz commented 1 year ago

New revision

jubalh commented 1 year ago

Thanks!