Skip to content
This repository has been archived by the owner on Jun 16, 2020. It is now read-only.

Adding an option to choose a font #3

Closed
wants to merge 3 commits into from
Closed

Conversation

ylmrx
Copy link

@ylmrx ylmrx commented Jun 26, 2018

Thanks for your awesome work :) I hope you find this patch useful.

@nbedos
Copy link
Owner

nbedos commented Jun 26, 2018

Hey man, thanks for contributing the first pull request :)

I do agree that termtosvg needs a way to customize the font used, but I think it belongs with color themes and should be set by the 'font' resource of Xresources. This way it can be set for both the terminal (xterm, urxvt...) and termtosvg in a single place.

This would probably require the following steps:

  • Extracting the 'font' resource from Xresources and parsing it to extract a font name usable with CSS (term.get_configuration)
  • A font attribute should probably be added to asciicast._AsciiCastTheme, or maybe a new asciicast v2 record type should be created to include the font name so as to not break compatibility with asciinema (see https://github.com/asciinema/asciinema/blob/develop/doc/asciicast-v2.md#notes-on-compatibility)
  • Writing the font to the style portion of the SVG animation, using Deja Vu Sans Mono as a fallback if no font is provided by the theme

One last gotcha: termtosvg uses hardcoded numbers for character cells width and height. I picked those numbers for Deja Vu Sans Mono and a 14 pixels font size, so they are probably not suitable for all fonts. At some point it might be necessary to expose them so that they can be set by the user (or even better, to compute them based on the name of the font, but I can't seem to find a way to do that that gives reliable results)

Let me know if you wish to work on this, or are willing to help testing (I definitely need help for that). In any case I'll probably take a deeper look at this this weekend.

Thanks again

@ylmrx
Copy link
Author

ylmrx commented Jun 27, 2018

Hello !
The font as it is set in .Xresources isn't always a good option, it will often be set to old PCF (the default fixed in xterm (ie lines such as : *-fixed-100-1-*-* )), and just won't work in this context ! We can safely use it if they are set to TTF/OTF though.

  • AsciiCast feels like the best option if it's used correctly by the renderer.
  • The CSS in SVG doesn't feel too good if it can be specified before.
    Regarding the size : as we always use monospace font in this context, I had a reliable and satisfying render even with thinner fonts. I didn't bother further.
    I'll be happy to test any new version, but can't guarantee a sustained contribution further than a line here and there. I'll check the asciicinema thing though.

Does this looks good to you (before we get a better view with asciicast) ?

if('--font'):
  render_with_this_in_css
elif(TTF set in Xresources):
  render_with_this_in_css
else:
  use_default_css

nbedos added a commit that referenced this pull request Jul 1, 2018
With a contribution by ylmrx (Pull request #3)
@nbedos
Copy link
Owner

nbedos commented Jul 1, 2018

I have replaced the configuration via Xresources by a simple INI file, and I have also implemented the font option based on your commits. Thanks!

I did not add the font to Asciicast records since the font is not detected on the system: it's just a configuration option.

I have not released this version to the production pypi server yet, but you can dowload it from the test server:

pip install --extra-index-url https://test.pypi.org/simple/ 'termtosvg>=0.3.0rc0'

The updated README is available here: https://github.com/nbedos/termtosvg/tree/release/0.3.0

Let me know what you think!

@nbedos nbedos added the enhancement New feature or request label Jul 2, 2018
@nbedos
Copy link
Owner

nbedos commented Jul 2, 2018

I have just released version 0.3.0 which includes the font option.

@nbedos
Copy link
Owner

nbedos commented Jul 3, 2018

Don't worry about updating your branch, it's ok. I had to do other changes before including your commits in my tree and I didn't want to bother you with a rebase. Your modifications have been added though the following commit : d930740
Thanks again for contributing!

@nbedos nbedos closed this Jul 3, 2018
@nbedos nbedos added the merged This pull request was merged label Jul 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request merged This pull request was merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants