-
-
Notifications
You must be signed in to change notification settings - Fork 257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
boxes/helper: Add support for rendering tables. #550
Conversation
Hello @preetmishra, it seems like you have referenced #452 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged. Please run An example of a correctly-formatted commit:
Thank you for your contributions to Zulip! |
@preetmishra This is a great feature to have 🎉 It would be useful to be able to support the syntax used by the web-app |
10ab02e
to
97e6e90
Compare
@kaustubh-nair Thanks for the review. 👍 I have sorted out the bug that you found. |
Another thing I have noticed is that in-table styling doesn't work in ZT yet (for instance, alignment). I was thinking that we might want to support that in v2 but if it doesn't seem like a good idea, I would move ahead to support that in v1 itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@preetmishra This is rather good in use for a v1, though I've not dug into the code in depth 👍
Some thoughts:
- The table-like view is clean - you can see each cell - but could we get away with fewer horizontal lines? Then we can see more message lines in the message list at once (v1)
- More tricky, but if the screen is narrow then it breaks up quite badly (v2+)
- We could at least use box-drawing characters for the table to make it look tidier? (v1?)
- Potentially we could use a
LineBox
, to make some of that easier (v2+) - Perhaps split the code into a html->internal and an internal->urwid (render) function?
If alignment is not tricky, it could be v1, but getting a good basic layout would be a great start; for example, we have bugs with quoting, but it works better than not knowing it's a quote, or not rendering it at all!
97e6e90
to
eae6c5c
Compare
eae6c5c
to
5b218a3
Compare
@neiljp Thanks for the review. I have made the changes that you suggested for v1. I have:
I have left comments in the code for explaining a few points, let me know if it is too much or if we need more. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@preetmishra This renders really well, and I love the improvements 👍
I think you could improve the test coverage for the new features/adjustments you made, and there is a small rendering nuance which makes this look a little off due to the way that the bold
is applied.
I've done a first pass of the code, which works well as it is but might benefit from some minor improvements to improve the readability or style.
5b218a3
to
5b1ffae
Compare
@neiljp Thanks. I have added extra test cases (to support different alignments), fixed the styling issue and incorporated the other changes that you suggested. I have restructured the row styling functions to return a list of strings and tuples (style, string). This not only helps in centralizing the padding routine but also helps in setting styles individually. In addition to these, this also makes the work required for adding inline-table-styles in v2 rather straightforward. I would add the tests for the helper functions once we reach a consensus about the implementation. |
5b1ffae
to
517e914
Compare
Update: Extended the docstring for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@preetmishra Looking good 👍
I wouldn't worry about adding tests for the really intermediate functions - we can do, but they are implementation details for render_table
. One alternative would be to keep the table tests for soup2markup
separately as a test for that function. We can discuss.
Could you comment on the need for the first commit?
If we can clarify the conflicts with other PRs, I'd be quite happy to move the MessageBox
into a separate file (and tests too?), and then all of this new work too, as we discussed on the stream - @sumanthvrao Did you try zulip/TinglingGit in the end?
@neiljp This is the TinglingGit Output for All the PR's which clash with the file ui_tools/boxes.py
|
This introduces typing support for adding 'None' display attribute in any text markup in the soup2markup method's `markup` list.
517e914
to
044e64f
Compare
@neiljp Thanks. I have:
If it is not much of an overhead, we can move the MessageBox in a separate file along with the new changes and perhaps separate the table test from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@preetmishra This is looking great :)
Re urwid_Size
I purposely chose this since this is intended to represent a type which should be externally defined. We could potentially handle this differently by providing type hints for urwid.
We should be able to fix at least some of the type ignores; the variable one we can leave - though it may be worth researching if it is a mypy issue.
zulipterminal/helper.py
Outdated
|
||
# [1] Add "type: ignore", to suppress "unsupported operand types" error, | ||
# for row_strip[index] and fill concatenation(s) - `row_strip` is of type | ||
# "StyledTableData" and fill is of type "str". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to fix this myself just now. This works for hard-coded indices via eg. assert isinstance(row_strip[0], str)
(so should work for the first and last element), but not where using a variable index. This is a case where the 'contract' for the function actually requires the '_char' ones to be unstyled, right? So, these assert statements enforce that, and mypy
can confirm this :)
That it doesn't work with a variable could just be a mypy
bug - worth checking, and that can be referenced in the ignore (as done in the main zulip/zulip repo, if you git grep for ignores)
* Added support for rendering tables, in the MessageBox, using helper functions, parse_html_table, render_table, style_row and unicode_border which parse and return the table contents in a list. * Added `table_head` in themes, for default, gruvbox, light and white, to specify a style for table headings. Tests amended and added for different column alignments and types of tables.
044e64f
to
4651b3c
Compare
@neiljp Thanks. I understand the idea for I was able to fix all the |
@preetmishra As I mentioned on the stream, this is now merged with a few minor tweaks 🎉 This is really great :) I'm sure you can find it in the list of commits by now! |
@neiljp Thanks so much, it was fun working on this. :) To address the things that we would like to see in the future iterations, I'll file an issue. |
I have added a helper function, render_table, to process a table and return its content as a list of strings which can then be rendered in the MessageBox and amended the test.
This implementation is v1. Feedbacks are welcomed. :)
There can certainly be follow-ups to improve this further.
Partially fixes #452.
@neiljp