Improve translation of pygments styles into urwid

neiljp commented 1 year ago

This work would represent a clean fix for #1431, which was temporarily fixed by pinning pygments at ~=2.15.1 in #1433. However, it could also heavily simplify our need for overrides in the themes.

As per #1433, the bug could be worked around by using our pygments override option, which manually converts the pygments bold italic style into the urwid-compatible bold, italics.

However, instead of using overrides for this, we should aim to automatically convert pygments styles into urwid-compatible versions. Based on the above, we likely want to consider at least:

[x] converting spaces between combined entries to include a comma
[x] converting italic to italics

Instead of manually testing directly with an upgraded version of pygments, initial indications are that we can remove an override from an existing style. If when you do so ZT gives an error, you can compare that string with the removed override to see what differs - and if an automatic translation could replace it. Once we have at least the comma insertion (first task above) we could therefore add

[ ] a runtime check comparing the (translated) pygments style to any override value given, and give a warning if it is redundant (and can be removed)

Once existing themes are simplified and we have an improved translation in place:

[x] explore updating pygments to >=2.16.x, and whether all themes continue to work

As with the majority of PRs, each automatic conversion and new functionality should be accompanied by an updated or new test.

A possible extension or early work, would be to explore writing a test that fails with an invalid pygments style (eg bold italic). That should limit the impact of something like #1431 in future, since we should expect a failure in our tests, not just at runtime.

jrijul1201 commented 11 months ago

Hi all, I would like to work on this issue. @zulipbot claim Thanks!

zulipbot commented 11 months ago

Welcome to Zulip, @jrijul1201! We just sent you an invite to collaborate on this repository at https://github.com/zulip/zulip-terminal/invitations. Please accept this invite in order to claim this issue and begin a fun, rewarding experience contributing to Zulip!

Here's some tips to get you off to a good start:

Join me on the Zulip developers' server, to get help, chat about this issue, and meet the other developers.
Unwatch this repository, so that you don't get 100 emails a day.

As you work on this issue, you'll also want to refer to the Zulip code contribution guide, as well as the rest of the developer documentation on that site.

See you on the other side (that is, the pull request side)!

zulipbot commented 10 months ago

@jrijul1201 You have been unassigned from this issue because you have not made any updates for over 14 days. Please feel free to reclaim the issue if you decide to pick up again. Thanks!

jrijul1201 commented 10 months ago

@zulipbot I am still working on this

jrijul1201 commented 10 months ago

@zulipbot claim

zulipbot commented 10 months ago

@jrijul1201 You have been unassigned from this issue because you have not made any updates for over 14 days. Please feel free to reclaim the issue if you decide to pick up again. Thanks!

neiljp commented 7 months ago

Thanks to @jrijul1201 for completing the first part of this in #1452, which was a great bugfix and allowed an upgrade to the latest version 🎉

The remaining part concerns the checking for redundant overrides in themes, which would help us simplify themes, as well as reduce such overrides being present in user themes.

@jrijul1201 Are you interested in continuing this work?

zulip / zulip-terminal

Improve translation of pygments styles into urwid #1434