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 configurable font paths #360

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

PgBiel
Copy link
Contributor

@PgBiel PgBiel commented Nov 19, 2023

Adds the equivalent of Typst CLI's --font-path option to the LSP. (I recommend merging #359 first if possible, as it will require a few changes to this PR when merged, but they should be very tiny changes!)

Closes #75 , related to #267

Supported features

  • Specifying a list of absolute paths to font folders in the configuration;
  • Typst recognizes the fonts in the given paths;
  • "Hot reload" of the font path configuration (the LSP doesn't have to be restarted when the configuration is changed);
    • The current implementation is a bit inefficient as it searches all fonts again each time the configuration changes. This shouldn't really be a problem, however, as I believe configuration changes aren't in the hot path. But I could add a simple check of whether the font paths have actually changed before searching for all fonts again.
  • Specifying a list of relative paths to font folders in the configuration;
    • Does this actually need implementation? And what would they be relative to? The workspace root?
  • Support for paths to font folders beginning with ~ to indicate the home folder;
    • Hopefully this shouldn't be too hard to implement? If it is at all desirable, that is...
  • Environment variables (like TYPST_FONT_PATHS in the CLI)?
    • Should we actually support this? I think this isn't necessary for an MVP.

Looking for testers

Tested in:

  • Linux (VSCodium Flatpak in NixOS 23.05)
  • MacOS (comment below if the font paths were successfully loaded for you and recognized by Typst)
  • Windows (comment below if the font paths were successfully loaded for you and recognized by Typst)

Screenshots

VSCode Configuration

font paths config option with some paths

Font is recognized

the font's name is suggested by autocomplete

the font is correctly rendered

@nvarner
Copy link
Owner

nvarner commented Nov 20, 2023

This is looking like a good change. I'm a little wary of searching fonts every time configuration changes, since it's so easy to trigger -- though that can be partially addressed if I have the time to improve how config is handled. Is that something that can happen in the background?

@PgBiel
Copy link
Contributor Author

PgBiel commented Nov 20, 2023

I'm a little wary of searching fonts every time configuration changes, since it's so easy to trigger -- though that can be partially addressed if I have the time to improve how config is handled.

Indeed, I'm not too happy with that either. I thought about caching the font paths in a Set-like object and comparing so we can know when the font paths actually changed. This could be improved further by fully caching system fonts (by e.g. saving the partial Builder somewhere, as long as it's Clone-able). I could make those changes in the following days if that sounds good.

Is that something that can happen in the background?

Likely, yes. Right now, I believe it runs this in its own task (not sure though - if it requires an explicit call to spawn somewhere, then maybe it doesn't), but the problem is mostly that write access to the Workspace is held while it's being done. So perhaps the FontManager could be built separately in its own task (without asking for write access to the Workspace) and then directly handed to the Workspace in one call. What do you think?

@KillTheMule
Copy link
Contributor

This very much feels like it should not be implemented explicitely by typst-lsp, but be exposed by typst itself, so that there's no chance of a discrepancy between what fonts typst-cli would use and typst-lsp.

@nvarner
Copy link
Owner

nvarner commented Nov 29, 2023

@PgBiel I see now that the searching will run in its own async task. However, it looks like the search is done synchronously, so it will block the executor, so it does need something like spawn_blocking to get its own thread which it can block.

Your point about the write lock is a bigger one. It doesn't really matter where the searching happens, since not much else can happen until the lock is released. It is correct to hold some kind of write lock here: if we change fonts, then compile a document which may involve an added/removed font, we can't compile until we know the updated font set.

Using caching to only search changed paths is a good idea, and really should be implemented as part of Config.

The best solution might be fine-grained locking. It would add a lot of complexity, but holding one (or several) write lock(s) internal to the font searching system would allow read access to most of the Workspace, which could service many LSP requests (e.g. semantic highlighting, completions, etc) while searching for fonts.

@PgBiel
Copy link
Contributor Author

PgBiel commented Jan 31, 2024

So, just an update: I ended up not having enough time to keep going with this, since I'm now focusing on multiple PRs to the Typst compiler with improvements to tables. However, it's very much possible for me to come back to this after I'm done.

However, if anyone wants to see this happening sooner and would be interested in picking this up, please let me know!

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.

[Feature Request] Settings for additional font directories
3 participants