Skip to content
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

Add script to enable users to test symbol rendering on their terminals #979

Closed
wants to merge 3 commits into from

Conversation

prah23
Copy link
Member

@prah23 prah23 commented Apr 3, 2021

This PR has 2 commits that do the following:

  • Create a tool to be shipped with ZT that allows users to test symbol rendering on their respective terminal emulators. In the current iteration, the command to run the script is zulip-term-check-symbols. This script has been placed in a new folder called scripts/.
  • FAQ.md has been updated to point users/contributors to the same, to help increase awareness on possible symbol rendering issues.

Tested on Windows Terminal and GNOME Terminal on Ubuntu, the layout is maintained in both.

Screenshots: As observed on Windows Terminal:
Symbols rendering script

@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Apr 3, 2021
docs/FAQ.md Outdated Show resolved Hide resolved
Copy link
Member

@Ezio-Sarthak Ezio-Sarthak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great in production 👍

While we aim to check for symbol support, we'd likely want the UI to be simpler, so as to not cause any other rendering issues :)

Leaving some comments inline :)

zulipterminal/scripts/render_symbols.py Outdated Show resolved Hide resolved
zulipterminal/scripts/render_symbols.py Show resolved Hide resolved
@prah23 prah23 force-pushed the render_symbols_tool branch from cf8e26b to 8208b91 Compare April 4, 2021 21:35
@zulipbot zulipbot added size: M [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Apr 4, 2021
@prah23 prah23 force-pushed the render_symbols_tool branch 3 times, most recently from 107b764 to 9635e2d Compare April 4, 2021 21:46
Copy link
Member

@zee-bit zee-bit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good. 👍

And, although the code is much simpler and rendering fine, we do get a very wide box(at least on my terminal). The names and their corresponding symbols are also very far apart, and my comment below may help reduce the distance?

Apart from that, it's nice work. :)

zulipterminal/scripts/render_symbols.py Outdated Show resolved Hide resolved
zulipterminal/scripts/render_symbols.py Outdated Show resolved Hide resolved
@prah23 prah23 force-pushed the render_symbols_tool branch from 9635e2d to 8c7de56 Compare April 4, 2021 22:39
@prah23
Copy link
Member Author

prah23 commented Apr 4, 2021

@zeebit
With the align left and right hack, with a dividechars of 10, it might look misaligned to the user. I've not included the hack for now and just left it center aligned, which seems to provide consistent alignment in most cases.

@prah23
Copy link
Member Author

prah23 commented Apr 5, 2021

@zulipbot add "PR needs review"

@zulipbot zulipbot added the PR needs review PR requires feedback to proceed label Apr 5, 2021
docs/FAQ.md Outdated Show resolved Hide resolved
@prah23 prah23 force-pushed the render_symbols_tool branch from 8c7de56 to 199efab Compare April 8, 2021 20:57
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prah23 I'm not sure we need to linger on this PR too long, but the spaced-out layout makes it tricky to determine which element aligns with each text description. I think you may be able to get a more compact form of this by explicitly looking for the maximum lengths of each column (or at least the names) and specifying the width. You may be able to just use one column pair rather than one per row? Also given the Padding seems challenging to get right, perhaps cheat a little and put the linebox with fixed width in 3 columns?

Re the FAQ text, I think we need to simplify and maybe just say 'symbols' in place of non-ASCII. The previous problem should be long-gone (0.3.0 is some time/versions ago), and we should be resolving the problem listed by setting the encoding in ZT in any case. So maybe just mostly replace that paragraph? @zee-bit Did you have any issues with characters?

zulipterminal/scripts/render_symbols.py Outdated Show resolved Hide resolved
zulipterminal/scripts/render_symbols.py Outdated Show resolved Hide resolved
@neiljp 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 Apr 9, 2021
@prah23 prah23 force-pushed the render_symbols_tool branch from 199efab to 40e67fb Compare April 9, 2021 21:02
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Apr 9, 2021
@prah23 prah23 force-pushed the render_symbols_tool branch 2 times, most recently from 26f6669 to d6d0371 Compare April 9, 2021 21:45
@prah23
Copy link
Member Author

prah23 commented Apr 9, 2021

@neiljp
I've updated the layout. I've used explicit widths to pack the layout together to make it look more compact and concise. I've used a Padding and have given the widgets inside it just a little over the exact number of characters they need. I've also added comments in the code as to how I've used the widths, in case we add symbols in the future (with longer names) and might need to update the script.

I've tested this on Windows Terminal (updated the description of the PR with the latest screenshot) and the GNOME Terminal, the display renders the same way and does not break on varying the terminal window's layout.

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

@zulipbot zulipbot 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 Apr 9, 2021
@prah23 prah23 requested a review from neiljp April 9, 2021 21:55
@prah23 prah23 force-pushed the render_symbols_tool branch from d6d0371 to 316307e Compare April 13, 2021 22:32
This commit removes the "Unable to render non-ASCII characters" section
from the FAQ which suggested that users set their terminal's encoding
manually. This is no longer required post the version 0.4.0 release, in
which we introduced urwid's set_encoding() method in run.py to set the
user's terminal's encoding internally to render the characters properly.
@neiljp
Copy link
Collaborator

neiljp commented Apr 15, 2021

@prah23 To clarify my previous review, while this is expected to be a less-used script I'd prefer that we don't hard-code the widths but calculate them - we don't update the symbols very often, but it just takes one long name and the table goes awry from wrapping.

The FAQ entry reads fine, but the user needs a way to validate what the script should produce - eg. v1 maybe suggest taking a screenshot and asking on czo if there are problems, or v2 have a trimmed-down screenshot of the script as it stands in the FAQ itself? (v3 ideally itself generated for when we update it and doing diffs in CI?)

@neiljp 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 Apr 15, 2021
@prah23 prah23 force-pushed the render_symbols_tool branch 8 times, most recently from 791e74d to d2f4215 Compare April 17, 2021 06:40
@prah23
Copy link
Member Author

prah23 commented Apr 17, 2021

@neiljp
I've updated the script to check for the maximum symbol name length and structure the layout accordingly, I've set the widths relative to this maximum length.

For the FAQ update, I've mentioned up to the suggested v2, I've shared a screenshot of the tool (taken on the GNOME Terminal) that the users can compare with and then let us know here or on CZO.

@prah23
Copy link
Member Author

prah23 commented Apr 17, 2021

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

@zulipbot zulipbot 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 Apr 17, 2021
@prah23 prah23 force-pushed the render_symbols_tool branch from d2f4215 to 82e4ff1 Compare April 19, 2021 14:02
prah23 added 2 commits April 20, 2021 00:42
This commit adds a new folder `scripts/` to ship scripts that are meant
for users. This commit also adds a new script that enables users to check
how the various symbols listed in `config/symbols.py` render on their
terminal emulator.

The need for this script has arisen from the observed issues on the Windows
Terminal, where with varying fonts, symbol rendering has also varied.
This commit updates the FAQ regarding symbol rendering information
and specifies that the rendering of characters varies based on
emulator and font. It also specifies that we provide a tool for
the user that to conveniently test symbol rendering in their terminal
emulator and font.

A screenshot of the tool run on the GNOME Terminal in Ubuntu is
also displayed so the users can note what the tool is ideally expected
to display.
@prah23 prah23 force-pushed the render_symbols_tool branch from 82e4ff1 to 2c3714c Compare April 19, 2021 19:12
@neiljp
Copy link
Collaborator

neiljp commented Apr 20, 2021

@prah23 I just merged with a few minor textual changes - add that ctrl c == ^C, and some minor commit text tweaks 🎉

@neiljp neiljp closed this Apr 20, 2021
@zulipbot
Copy link
Member

Heads up @prah23, 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 neiljp added this to the Next Release milestone Apr 20, 2021
@neiljp neiljp removed has conflicts PR needs review PR requires feedback to proceed labels Sep 8, 2021
@neiljp neiljp added area: documentation Requires changes in documentation enhancement New feature or request labels Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: documentation Requires changes in documentation enhancement New feature or request size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants