bugfix: boxes: Fix quote text rendering for xfails. #942

zee-bit · 2021-03-05T21:57:21Z

No description provided.

neiljp · 2021-03-05T23:25:13Z

tests/ui/test_ui_tools.py

-                     marks=pytest.mark.xfail(reason="rendered_bug")),
+        ("""<blockquote>
+                <blockquote>
+                    <p>A<br/>B</p>


A quick test indicated the server actually generates a <br> (not xml) and after both A and B too?

There are also extra newlines in each blockquote associated with ::before and ::after - I've not looked at this in a while but are we relying on those to be present for spacing purposes?

There's a similar test just above this, that had <br/> as well, so I didn't cross-check with the server.

Yes, I presume they are there because the quoting expects some text after the quote, hence the newline?

neiljp

@zee-bit Thanks for tackling this - This looks like a great improvement already 👍 Our quoting is OK right now, but more complex quotes certainly need some work!

As you indicated, the next step is adding more test cases; these need not be xfails if you can fix them at the same time :)
Some areas that stand out:

blank lines after multi-quotes, eg. no response to a quote and quoted again -> extra blank line per quote level
text above the quoted text, which is much more common with the " said..." format we have now, for quote-level>=2
handling "split" or multiple quotes in the same message; this is particularly evident with the simple quotes enabled by starting lines with '>', though this isn't used in auto-quoting

Some of that may be part of #402. More work on this would be great, but if not then we should likely add more test cases with xfails, or at least a tracking issue which could point to #402 or perhaps replace it.

zulipterminal/ui_tools/boxes.py

tests/ui/test_ui_tools.py

zee-bit · 2021-03-07T20:59:53Z

@neiljp PR has been updated keeping in mind your review above.

I have managed to clear out most of the extra newlines for multilevel quotes.
The user said ... lines for multilevel quotes have also been fixed, though, for quote-level >= 3, the message formatting seems broken (probably some bug in soup2mark()?)
I tried fixing handling "splits", but it had some adverse side-effects in other simpler quotes, that were previously fixed.
Tests have also been added(and amended) with a few xfails.

zee-bit · 2021-03-07T21:01:39Z

@zulipbot add "PR needs review"
@zulipbot remove "PR awaiting update"

zulipbot · 2021-03-07T21:01:41Z

ERROR: Label "PR needs update" does not exist and was thus not removed from this pull request.

neiljp

@zee-bit The refactor definitely improves the readability, though it does expose the broken links!
This is definitely an improvement over the previous situation and works for higher levels of quoting including text above it, but we still end up with some strange behavior and style corruption at level 3-4+?

This appears an improvement over the previous iteration, though I'm not sure precisely what you've changed - and the newline changes are somewhat unclear.

This doesn't seem to address #402 directly (mixing of quote styles), though given the improvement already I'm open to merging a simplified version and iterating, if we can clarify the open points.

neiljp · 2021-03-07T22:10:40Z

tests/ui/test_ui_tools.py

+              "D"),
+             id="quoted level 3"),
+        case("""<blockquote>
+                    <p>A<br>B</p>


Does the server send this over two lines?

Ahh, yes. Sorry, forgot to change this!

neiljp · 2021-03-07T22:32:27Z

tests/ui/test_ui_tools.py

-             ("{} {} {} A\n\n"
-              "{} {} B\n\n"
-              "{} C\n\n"


Could you explain why the newlines are removed?

blank lines after multi-quotes, eg. no response to a quote and quoted again -> extra blank line per quote level

This is for addressing the first point of your review above. I thought you wanted those extra newlines to be removed. This is the code that that is responsible for this change:

if(markup[-1] == '\n' and isinstance(markup[-2], tuple) and markup[-2][0] == 'msg_quote' and isinstance(markup[-2][1], list) and markup[-2][1][-1] == '\n'): markup = markup[:-1]

I am segregating this change to a new commit, just In case you are not satisfied with this.

Are those extra newlines important?

neiljp · 2021-03-07T22:33:28Z

tests/ui/test_ui_tools.py

+             id="quoted level 2-1-2"),
+        case("""<p><a href='https://chat.zulip.org/1'</a>czo</p>
+                <blockquote>
+                    <p><a href='https://chat.zulip.org/2'</a>czo</p>


This applies to the original links too, but these are actually invalid links - how do these pass?

I don't understand what do you mean by "invalid links". Are you pointing out the incorrect syntax used for a tag or the invalid links in general?

neiljp · 2021-03-07T22:37:37Z

zulipterminal/ui_tools/boxes.py

+            # Remove extra newline from the end of blockquotes
+            if(markup[-1] == '\n'
+               and isinstance(markup[-2], tuple)
+               and markup[-2][0] == 'msg_quote'
+               and isinstance(markup[-2][1], list)
+               and markup[-2][1][-1] == '\n'):
+                markup = markup[:-1]


This looks rather messy. Can this be included in the quote handler? If so, would it be clearer there?

This piece of code checks for extra newlines after quotes and removes them. But, I agree, this looks messy and complex, mainly due to the following reason:

Our soup2mark function is recursive, hence it becomes difficult to check for unnecessary blank lines after quotes.
Currently, I am relying on markup to check for any appended \n at the end of each recursion-step, then comparing it with the previous element if it was blockquote and had a \n at the end already. We then remove this extra newline from the end of markup.

Let me see if I can simplify this!

zee-bit · 2021-03-08T19:43:18Z

@zulipbot add "PR needs review"
@zulipbot remove "PR awaiting update"

neiljp · 2021-03-10T07:28:10Z

@zee-bit I've just merged the first two commits, and replied on the stream about this.

This commit removes the extra newline that was getting rendered at the end of end of multiline quotes. Tests amended.

neiljp · 2021-04-21T01:06:42Z

As per my comment in the stream, let's take the remainder of this regarding improving the newline handling into a separate PR? I'm more convinced about the remaining commit now, but in any case there seem to be remaining issues, possibly related to the html returned by the server.

zee-bit · 2021-04-21T13:35:05Z

The later commit from this PR, related to removing extra newlines from the end of quoted messages, is branched off to #1003, as requested. Further improvements will be pushed to that PR. Closing this now.

zulipbot added the size: M [Automatic label added by zulipbot] label Mar 5, 2021

neiljp reviewed Mar 5, 2021

View reviewed changes

zee-bit force-pushed the fix-quote-rendering branch from 64ea1e1 to 7881c44 Compare March 6, 2021 12:38

zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Mar 6, 2021

neiljp reviewed Mar 6, 2021

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Mar 6, 2021

zee-bit force-pushed the fix-quote-rendering branch from 7881c44 to 6220988 Compare March 7, 2021 20:50

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Mar 7, 2021

zulipbot added the PR needs review PR requires feedback to proceed label Mar 7, 2021

neiljp reviewed Mar 7, 2021

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Mar 7, 2021

zee-bit force-pushed the fix-quote-rendering branch from 6220988 to bb0f5d7 Compare March 8, 2021 16:42

zulipbot added PR needs review PR requires feedback to proceed has conflicts and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Mar 8, 2021

boxes: Remove extra newline after multi-quotes.

7444e33

This commit removes the extra newline that was getting rendered at the end of end of multiline quotes. Tests amended.

zee-bit force-pushed the fix-quote-rendering branch from bb0f5d7 to 7444e33 Compare March 10, 2021 14:36

zulipbot added size: M [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] has conflicts labels Mar 10, 2021

neiljp removed the PR needs review PR requires feedback to proceed label Apr 21, 2021

zee-bit closed this Apr 21, 2021

neiljp added this to the Next Release milestone Feb 27, 2022

neiljp added the bug Something isn't working label Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: boxes: Fix quote text rendering for xfails. #942

bugfix: boxes: Fix quote text rendering for xfails. #942

zee-bit commented Mar 5, 2021

neiljp Mar 5, 2021

zee-bit Mar 6, 2021

neiljp left a comment

zee-bit commented Mar 7, 2021 •

edited

Loading

zee-bit commented Mar 7, 2021 •

edited

Loading

zulipbot commented Mar 7, 2021

neiljp left a comment

neiljp Mar 7, 2021

zee-bit Mar 8, 2021

neiljp Mar 7, 2021

zee-bit Mar 8, 2021 •

edited

Loading

neiljp Mar 7, 2021

zee-bit Mar 8, 2021 •

edited

Loading

neiljp Mar 7, 2021

zee-bit Mar 8, 2021

zee-bit commented Mar 8, 2021

neiljp commented Mar 10, 2021

neiljp commented Apr 21, 2021

zee-bit commented Apr 21, 2021

bugfix: boxes: Fix quote text rendering for xfails. #942

bugfix: boxes: Fix quote text rendering for xfails. #942

Conversation

zee-bit commented Mar 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

zee-bit commented Mar 7, 2021 • edited Loading

zee-bit commented Mar 7, 2021 • edited Loading

zulipbot commented Mar 7, 2021

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zee-bit Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zee-bit Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zee-bit commented Mar 8, 2021

neiljp commented Mar 10, 2021

neiljp commented Apr 21, 2021

zee-bit commented Apr 21, 2021

zee-bit commented Mar 7, 2021 •

edited

Loading

zee-bit commented Mar 7, 2021 •

edited

Loading

zee-bit Mar 8, 2021 •

edited

Loading

zee-bit Mar 8, 2021 •

edited

Loading