Markup markdown text in stream description #749

preetmishra · 2020-07-29T14:57:35Z

While the PR's ultimate intent is to markup the stream description, the first commit has changes to decouple our markup methods.

Commits

The first commit makes our primary markup methods, transform_content, soup2markup and indent_quoted_content, independent of their class object.
The second commit uses transform_content in StreamInfoView for markup.
The third commit makes footlinks_view static and more configurable.

Fixes #743.

sumanthvrao

@preetmishra Left some thoughts on the first commit here.

zulipterminal/ui_tools/boxes.py

preetmishra · 2020-08-29T17:22:05Z

@sumanthvrao Thanks for the review. I have amended the tests and made the suggested amendments in the latest update.

preetmishra · 2020-08-30T06:39:34Z

Updated to resolve conflicts.

neiljp

@preetmishra This appears to work pretty well 👍

I'm not sure how busy you are at the moment to do any final work on this; my inline points should be fairly minor though we did just migrate the popup tests which has led to the conflict in test_ui_tools.py.

Is the first commit just a refactor? (if large - I'm not sure how easy it'd be to split it?)

Maybe originally I thought this was just an exploratory PR and put off reviewing it, and perhaps wondered if you planned to move the adjusted methods into a separate file for processing rendered content? It is effective as it stands, and we can always make the latter migration later - it should be much easier with the decoupling changes you'll have made here 👍

zulipterminal/ui_tools/boxes.py

tests/ui/test_ui_tools.py

preetmishra · 2020-11-16T11:30:46Z

@neiljp Thanks for the review. I have resolved the conflict and made the suggested amendments.

Yes, the first commit is a refactor (a bit tricky to separate further).

While our ultimate intent was to move the rendering methods to a separate file, we decided to decouple the methods first and do the migrations later if needed.

preetmishra · 2020-11-16T11:42:59Z

(Travis is having some trouble building the script for OS X. I will try restarting the job again in a while.)

preetmishra · 2020-11-16T16:32:37Z

(Fixed.)

neiljp

@preetmishra Thanks for keeping this updated 👍

I continue to like the move of the htmlsoup->urwidmarkup machinery into a state where we could extract it, though looking at this again I'm a little concerned that we've gone very verbose in the recursive calls with the extra parameters. Did we discuss other ways of achieving this? Can we use metadata? Along similar lines, perhaps we could have a "constant" dict (eg. metadata) and keep dynamic data in the state dict?

Most of my feedback otherwise is minor, I'd just like to keep the recursive code as clean as possible, if we can :)

neiljp · 2020-11-22T23:23:34Z

zulipterminal/ui_tools/boxes.py

@@ -695,12 +695,19 @@ def footlinks_view(
                             align='left', left=8, width=('relative', 100),
                             min_width=10, right=2)

-    def soup2markup(self, soup: Any, **state: Any) -> List[Any]:
+    @classmethod
+    def soup2markup(cls, soup: Any, server_url: str, metadata: Dict[str, Any],


metadata is actually mutable? This got accidentally changed in the commit title?

Yes, metadata is mutable but it was being used to track immutable objects between recursive calls. I changed the commit description on purpose.

Update: It is now being used to track mutable and immutable objects together to decrease the verbosity.

zulipterminal/ui_tools/boxes.py

neiljp · 2020-11-22T23:47:07Z

zulipterminal/ui_tools/views.py

+        self.markup_desc, *_ = MessageBox.transform_content(
+            rendered_desc,
+            self.controller.model.server_url,
+        )
+        desc = urwid.Text(self.markup_desc)


Is it necessary to have this here? It feels like it belongs above where the desc was previously set.

Thanks for pointing it out. Fixed!

zulipterminal/ui_tools/boxes.py

neiljp · 2020-11-23T02:38:59Z

zulipterminal/ui_tools/boxes.py

+        text_widget = urwid.Text(footlinks, wrap=wrap)
+        if padded:
+            return urwid.Padding(text_widget, align='left', left=8,
+                                 width=('relative', 100), min_width=10,
+                                 right=2), footlinks_width
+        else:
+            return text_widget, footlinks_width


This is the only part that depends on wrap or padded? That makes it feel separate to the function, ie. the conditional belongs outside of the calling points. The padding is clearly just a wrapper, that is used in one place, and the use of the wrap parameter is less clear - it depends if we're set on returning a widget vs footlinks.

This does feel out of place. We could return only the markup from footlinks_view (+ rename it to markup_footlinks) and then add wrappers as needed in the caller functions. Thoughts?

zulipterminal/ui_tools/boxes.py

preetmishra · 2021-01-14T18:22:25Z

@neiljp Thanks for the review and pointers. I wasn't inclined towards verbose recursive calls as well. I have moved the data that I want to track amidst the recursive calls into metadata to reduce the verbosity. I have also made other amendments as per your in-line comments.

This decouples MessageBox's markup methods, soup2markup(), transform_content() and indent_quoted_content(), for reusability. Additionally, this introduces `metadata` argument for soup2markup() to track objects amidst the recursive calls. Tests amended.

Footlinks support is added in a subsequent commit. Test amended. Fixes zulip#743.

Tests amended.

This modifies footlinks_view() to accept a range of configurable parameters and return footlinks_width. Tests amended.

preetmishra · 2021-01-17T07:35:26Z

Updated to resolve conflicts.

zulipbot · 2021-01-30T20:34:24Z

Heads up @preetmishra, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

neiljp · 2021-02-11T19:35:53Z

@preetmishra I just rebased this to take into account recent changes including the footlinks threshold work and manually pushed this to main ending at 1be52c5 🎉

All seems to work fine - thanks for your work on this and the rebasing!

zulipbot added the size: XL [Automatic label added by zulipbot] label Jul 29, 2020

preetmishra added the feedback wanted label Jul 29, 2020

preetmishra force-pushed the feat-markup-stream-desc branch 2 times, most recently from 01ac899 to 5d88ccd Compare August 28, 2020 19:13

sumanthvrao reviewed Aug 29, 2020

View reviewed changes

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved

preetmishra force-pushed the feat-markup-stream-desc branch from 5d88ccd to f1b3de2 Compare August 29, 2020 17:17

preetmishra force-pushed the feat-markup-stream-desc branch from f1b3de2 to 0deaca8 Compare August 29, 2020 17:26

preetmishra marked this pull request as ready for review August 29, 2020 17:39

preetmishra added PR blocks other PR PR needs review PR requires feedback to proceed and removed feedback wanted labels Aug 29, 2020

preetmishra force-pushed the feat-markup-stream-desc branch from 0deaca8 to 6eacfa6 Compare August 30, 2020 06:38

neiljp reviewed Oct 20, 2020

View reviewed changes

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved

tests/ui/test_ui_tools.py Outdated Show resolved Hide resolved

neiljp removed the PR needs review PR requires feedback to proceed label Oct 20, 2020

preetmishra force-pushed the feat-markup-stream-desc branch 2 times, most recently from 90c68a2 to 0671ead Compare November 16, 2020 11:24

preetmishra added the PR needs review PR requires feedback to proceed label Nov 16, 2020

neiljp reviewed Nov 23, 2020

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Nov 23, 2020

preetmishra changed the title ~~Markup markdown text in stream description~~ [WIP] Markup markdown text in stream description Nov 23, 2020

preetmishra force-pushed the feat-markup-stream-desc branch 2 times, most recently from 8238744 to f6aef8b Compare January 14, 2021 17:57

preetmishra changed the title ~~[WIP] Markup markdown text in stream description~~ Markup markdown text in stream description Jan 14, 2021

preetmishra added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jan 14, 2021

preetmishra force-pushed the feat-markup-stream-desc branch from f6aef8b to 175e6ff Compare January 17, 2021 07:25

preetmishra added 4 commits January 17, 2021 12:57

views: Markup stream description using MessageBox.transform_content().

1c85091

Footlinks support is added in a subsequent commit. Test amended. Fixes zulip#743.

refactor: boxes: Make footlinks_view() static to facilitate reusability.

123408f

Tests amended.

boxes/views: Generalize footlinks_view() for StreamInfoView.

b03659c

This modifies footlinks_view() to accept a range of configurable parameters and return footlinks_width. Tests amended.

preetmishra force-pushed the feat-markup-stream-desc branch from 175e6ff to b03659c Compare January 17, 2021 07:35

neiljp added this to the Release after next milestone Jan 28, 2021

Base automatically changed from master to main January 30, 2021 20:30

zulipbot added the has conflicts label Jan 30, 2021

neiljp closed this Feb 11, 2021

preetmishra deleted the feat-markup-stream-desc branch February 12, 2021 02:26

neiljp added area: UI General user interface update missing feature: user A missing feature for all users, present in another Zulip client and removed PR needs review PR requires feedback to proceed labels Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Markup markdown text in stream description #749

Markup markdown text in stream description #749

preetmishra commented Jul 29, 2020 •

edited

Loading

sumanthvrao left a comment

preetmishra commented Aug 29, 2020

preetmishra commented Aug 30, 2020

neiljp left a comment

preetmishra commented Nov 16, 2020

preetmishra commented Nov 16, 2020

preetmishra commented Nov 16, 2020

neiljp left a comment

neiljp Nov 22, 2020

preetmishra Jan 14, 2021 •

edited

Loading

neiljp Nov 22, 2020

preetmishra Jan 14, 2021

neiljp Nov 23, 2020

preetmishra Jan 14, 2021

preetmishra commented Jan 14, 2021

preetmishra commented Jan 17, 2021

zulipbot commented Jan 30, 2021

neiljp commented Feb 11, 2021

Markup markdown text in stream description #749

Markup markdown text in stream description #749

Conversation

preetmishra commented Jul 29, 2020 • edited Loading

Commits

sumanthvrao left a comment

Choose a reason for hiding this comment

preetmishra commented Aug 29, 2020

preetmishra commented Aug 30, 2020

neiljp left a comment

Choose a reason for hiding this comment

preetmishra commented Nov 16, 2020

preetmishra commented Nov 16, 2020

preetmishra commented Nov 16, 2020

neiljp left a comment

Choose a reason for hiding this comment

neiljp Nov 22, 2020

Choose a reason for hiding this comment

preetmishra Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

neiljp Nov 22, 2020

Choose a reason for hiding this comment

preetmishra Jan 14, 2021

Choose a reason for hiding this comment

neiljp Nov 23, 2020

Choose a reason for hiding this comment

preetmishra Jan 14, 2021

Choose a reason for hiding this comment

preetmishra commented Jan 14, 2021

preetmishra commented Jan 17, 2021

zulipbot commented Jan 30, 2021

neiljp commented Feb 11, 2021

preetmishra commented Jul 29, 2020 •

edited

Loading

preetmishra Jan 14, 2021 •

edited

Loading