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 the ability to list the fonts detected #45

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

rgreinho
Copy link
Contributor

@rgreinho rgreinho commented Nov 3, 2023

Adds a new CLI flag allowing the user to just list the fonts which are
detected on the system.

Signed-off-by: Rémy Greinhofer remy.greinhofer@gmail.com

@rgreinho rgreinho mentioned this pull request Nov 3, 2023
@rgreinho rgreinho changed the title Add the ability ti list the fonts detected Add the ability to list the fonts detected Nov 3, 2023
@LaurenzV
Copy link
Collaborator

LaurenzV commented Nov 6, 2023

Looks good to me! I'm sure it will be reviewed soon, it's just that the owners of this crate are pretty busy at the moment, so don't be surprised if it takes a while until they manage to review it. But they will review it eventually. 😄

@laurmaedje
Copy link
Member

Needs rebase/merge because of new crate structure. That aside, I'm not sure whether the PostScript name is the most user-friendly thing to print. We could also group by typographic family and print the available variants like typst does. But maybe it's overkill here.

@rgreinho
Copy link
Contributor Author

Rebased 👌

I can change the output no problem. However I am not sure what is the best overall, I just implemented what helped me with my use case.

@reknih
Copy link
Member

reknih commented Nov 21, 2023

Hey!

I think that stuff that does not change the behavior of the CLI but does something different altogether should be a subcommand, not a flag. I agree with @laurmaedje that the font families should be sorted by and displayed with their typographical families. I also get that showing PostScript names is useful, especially since the library is subject to two separate font resolution algorithms (CSS standalone and Typst/consumer). How about an output like this?

Helvetica
├ Helvetica
├ Helvetica-Bold
├ Helvetica-BoldOblique
├ Helvetica-Light
├ Helvetica-LightOblique
└ Helvetica-Oblique

svg2pdf fonts could have the same output as typst fonts and an --all option would switch to this view.

@rgreinho
Copy link
Contributor Author

Yep, that sounds good! I can have a look at typst fonts, add the command, and implement the output you showed! 👍

(Also, just keeping in mind that we're doing all of this to try to solve #44 🙃).

@rgreinho rgreinho force-pushed the list-font-command branch 3 times, most recently from c978ed6 to e3f2a5d Compare November 24, 2023 03:34
@rgreinho
Copy link
Contributor Author

Alright! I looked at typst and reorganized the code in a similar way.

There are now 2 commands, convert which converts an SVG file into a PDF file, and fonts which lists the fonts detected on the system.

Here is the output of the fonts command:

image

And here the output of the fonts --all command:

image

Let me know what you think, and if this iteration matches what you had in mind. If for any reason that is not the case, just provide some pointers and I'll make the necessary adjustments. :)

@reknih
Copy link
Member

reknih commented Dec 11, 2023

I would, if possible, prefer if svg2pdf file.svg would remain the main convert command and only fonts needs a subcommand. If that's not possible, this LGTM.

@rgreinho
Copy link
Contributor Author

No problem! I'll give it a shot :)

@rgreinho
Copy link
Contributor Author

Ok, so I rearranged it like that:

image

Let me know what you think, and if you like it, I'll start working on rebasing this branch onto master 🙃

@laurmaedje
Copy link
Member

Looks good!

Adds a new CLI flag allowing the user to list the fonts which are
detected on the system.

Keep the pdf conversion as the default commandi. If no command is
provided, an error message will be displayed.

Signed-off-by: Rémy Greinhofer <remy.greinhofer@gmail.com>
@rgreinho
Copy link
Contributor Author

@laurmaedje @reknih rebase complete ✅

@laurmaedje laurmaedje merged commit 327e886 into typst:main Jan 4, 2024
2 checks passed
@laurmaedje
Copy link
Member

Thank you for your work and patience!

@rgreinho rgreinho deleted the list-font-command branch January 9, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants