Properly quote multiline messages

[maximbaz]

I was quite confused today until I figured what was going on 😅 What do you think of this approach?

Before:           |  After:
---------------------------------------

✓  You            |   ✓  You
   one two        |      one two
   three four     |      three four
   five six!      |      five six!
✓  You            |   ✓  You
   | MaximBaz     |      | MaximBaz
   | one two      |      | one two
   three four     |      | three four
   five six!      |      | five six!
   seven eight    |      seven eight
   nine ten       |      nine ten

[maximbaz]

Don't review just yet, I realized that I broke the support for two-sided mode, and possibly markup....

Will update soon!

[maximbaz]

Still WIP, much better but I can't quite solve it for all cases, maybe you could give me a hint, or think of a different approach entirely?

The last example (with markup) is what is still broken:

[exquo]

Yep, currently quoting can be confusing if the message appears on multiple lines: only the first line is marked with |, making it unclear where the quoted message ends and the main message text begins.

The tricky part is handling the multi-line ("soft"-)wrapped text by urwid. This might require modifying urwid.TextLayout. But handling the explicit newlines, along the plan in this PR, should be an improvement on the current situation.

Additionally, as a workaround, we can add a line separating the quoted and the 'reply' texts. Something like

| OriginalSender
| one two
| three four
| five six!
| ---
seven eight
nine ten

Vertical space is valuable, but if it helps avoid confusion, inserting an extra line might be worth it.

I'll take a look at fixing the right alignment issue.

[exquo]

I like your approach in the quote() function! I wrote something similar: d9abdbcfdd40995c8b42758026d2718daf50fa24. Seems to work, but haven't tested it extensively.

The stuff I had before in if self.align == 'right': del ret[0] ... was most likely bolted on after I realized that my original version did not support --one-sided :). It looked in need of refactoring anyway.

Let me know what you think!

[maximbaz]

Wonderful! I think it works better than my implementation!

The only thing I could spot is quoting attachment:

But again, even with this it's already a significant improvement!

Additionally, as a workaround, we can add a line separating the quoted and the 'reply' texts. Something like Vertical space is valuable, but if it helps avoid confusion, inserting an extra line might be worth it.

With your fix I don't think it's already necessary!

[exquo]

The only thing I could spot is quoting attachment:

This looks like #48 (emoji breaking alignment).. I don't actually get this with attachemnts.

[maximbaz]

Interesting, not in this particular case (if I try in terminal with no custom config and bash shell with no custom config, #48 no longer reproduces but this does), but let this not stop us, it's truly minor, if I manage to find a reason I'll send a separate PR 😉

[maximbaz]

I found the answer, this is fixed with the patch below on top of your commit:

--- a/scli
+++ b/scli
@@ -327,7 +327,7 @@ def get_envelope_attachments(envelope):
 def get_attachment_name(attachment):
     if isinstance(attachment, dict):
         filename = attachment['filename']
-        return filename if filename is not None else attachment['contentType']
+        return filename if filename else attachment['contentType']
     else:
         return os.path.basename(attachment)

The reason is because this is how my quote object looks like (in particular, see that filename is '', not None):

DEBUG:root:quote: {'id': xxx, 'author': '+xxx', 'authorNumber': '+xxx', 'authorUuid': 'xxx', 'text': 'one two \n*three* ~four~', 'attachments': [{'contentType': 'image/png', 'filename': '', 'thumbnail': {'contentType
': 'image/png', 'filename': None, 'id': 'xxx', 'size': 1127}}]}

With the above, all my local tests seem to render really well, thanks for your help!

Just close this when you merge your commit 😉

[exquo]

Thanks for troubleshooting this! Indeed, this looks like a separate issue. Apparently, when an image is sent through an android client, it is received with filename: None, but after that message is quoted, its envelope has filename: ''. Your patch above should fix it!

[exquo]

Incorporated into develop. Thanks again for your help here!

[maximbaz]

Thanks, running develop branch now! To be honest can't quite justify the need for --- under the quote, but maybe it's just me and my thing for vertical space 😂

[exquo]

I'm myself not very fond of | --- or using up a whole line for it. But it's useful when the messages are soft-wrapped and there is no indication where the quoted text ends and the reply begins.

But that separator is only appended to the longer messages. I know, 50 characters is not that long, and modern terminals support (way) more than 80 columns. It would be preferable to determine at runtime which messages will be in fact soft-wrapped. But urwid does not make that easy, to my knowledge.

As an alternative, we could put some separator at the end of the quote, but on the same line. It likely won't look pretty either, though. We could also add a config switch for whether to add any separator, but an option just for that seems too petty :).

[maximbaz]

Aaaah it's for the soft-wrapped quotes, I see! just didn't happen to me yet 😅 OK, I see, I also dont think there is anything better we can quickly offer...