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

Fonts don't load properly when called by python #24

Closed
slifty opened this issue Apr 29, 2021 · 2 comments · Fixed by #27
Closed

Fonts don't load properly when called by python #24

slifty opened this issue Apr 29, 2021 · 2 comments · Fixed by #27

Comments

@slifty
Copy link
Contributor

slifty commented Apr 29, 2021

First things first: I don't love the way fonts are being used in this project to begin with here and it isn't clear to me we need them.

Anyway, right now the PdfGenerator has a generatePdfFromScaffold method where a bunch of fonts are registered in the PdfPrinter object. The problem is is that the font paths break depending on where the script has been initiated from.

To make matters worse, __dirname doesn't actually exist in our current setup (this may be fixed by #20). For now we need to do something like https://stackoverflow.com/a/32707530/159522 when defining the font paths in order to call the mw2pdf script from our flask app.

@frankduncan
Copy link
Collaborator

I think webfonts are probably the correct solution for this. Having the plugin declare the fonts used for rendering seems bad. Having it be checked in to the codebase seems worse. Maybe best way is to have a way for someone configuring the system to put the fonts they want in a deployed directory. That way they can worry about legality of their fonts and whether they own them and whatnot.

@slifty
Copy link
Contributor Author

slifty commented Apr 30, 2021

I do think it's important for the package to be fully functional without requiring the installation of non-packaged dependencies (this isn't opposed to what you're saying, this is just saying that I would want the font configuration / installation step to be optional rather than a required part of setup -- e.g. the user of the module can explicitly pass font configuration as part of the documented API of the module).

In the mean time the current system does use embedded FOSS fonts and the license is included so the ownership question is covered.

TL;DR -- I agree it would be good to remove the need for embedded fonts in the first place. For the scope of this issue I'd like to just fix the code that exists (since we already have the fonts checked in etc etc) but I will create a second issue for that goal.

slifty added a commit that referenced this issue Apr 30, 2021
Fonts were being loaded in a way that would only work if the script was
being called from the mw2pdf root (the folder containing the embedded
fonts).

This adds some (ugly) logic to identify the `__dirname` of the utility
script and access fonts relative to the utility itself as opposed to the
calling path.

As discussed in #24 we will eventually want to remove fonts from the
project altogether, instead trying to have a generic default settings
and allowing font configuration to be configurable for anybody who would
like to override that generic option.

Issue #24
@slifty slifty mentioned this issue Apr 30, 2021
slifty added a commit that referenced this issue Apr 30, 2021
Fonts were being loaded in a way that would only work if the script was
being called from the mw2pdf root (the folder containing the embedded
fonts).

This adds some (ugly) logic to identify the `__dirname` of the utility
script and access fonts relative to the utility itself as opposed to the
calling path.  This logic is necessary because unfortunately NodeJS
doesn't populate __dirname when module mode is enabled.

See: https://stackoverflow.com/questions/32705219/nodejs-accessing-file-with-relative-path/32707530#32707530

As discussed in #24 we will eventually want to remove fonts from the
project altogether, instead trying to have a generic default settings
and allowing font configuration to be configurable for anybody who would
like to override that generic option.

Issue #24
slifty added a commit that referenced this issue Apr 30, 2021
Fonts were being loaded in a way that would only work if the script was
being called from the mw2pdf root (the folder containing the embedded
fonts).

This adds some (ugly) logic to identify the `__dirname` of the utility
script and access fonts relative to the utility itself as opposed to the
calling path.  This logic is necessary because unfortunately NodeJS
doesn't populate __dirname when module mode is enabled.

See: https://stackoverflow.com/questions/32705219/nodejs-accessing-file-with-relative-path/32707530#32707530

As discussed in #24 we will eventually want to remove fonts from the
project altogether, instead trying to have a generic default settings
and allowing font configuration to be configurable for anybody who would
like to override that generic option.

Issue #24
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 a pull request may close this issue.

2 participants