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

Can not set stylesheet path from c via the c-api #870

Open
michabay05 opened this issue Dec 25, 2024 · 4 comments · May be fixed by #873
Open

Can not set stylesheet path from c via the c-api #870

michabay05 opened this issue Dec 25, 2024 · 4 comments · May be fixed by #873

Comments

@michabay05
Copy link

Context

Most of the important usvg options written in rust are available in the c api. Some of the accessible options are setting the language, resource directory, rendering modes, and so on. However, the stylesheet option, which is available in the rust api, is not available in the c api.

--stylesheet PATH Inject a stylesheet that should be used when resolving
CSS attributes.

Question

  • Are there plans to add this option to the c-api? Something like this perhaps?
/**
 *
 * @brief Sets a path that will be used when resolving CSS attributes.
 *
 */
void resvg_options_set_stylesheet(resvg_options *opt, const char *path);

P.S.: Great work on the library!

@LaurenzV
Copy link
Contributor

Yeah, this could definitely be added. Feel free to open a PR, although I'm not sure I would be able to review it because my C knowledge isn't great, but I'm sure someone else would be able to.

@michabay05 michabay05 linked a pull request Dec 26, 2024 that will close this issue
@michabay05
Copy link
Author

michabay05 commented Dec 26, 2024

I'm interested in opening a PR to add this feature (here's the PR #873). In trying to do so, I found something to be a bit confusing. As you can see, the command line argument below requests a path to be provided to inject a stylesheet.

--stylesheet PATH Inject a stylesheet that should be used when resolving
CSS attributes.

So, I compiled resvg into a binary, provided the path to the stylesheet, and ran the command. Unsuprisingly, it worked great.

$ target/debug/resvg --stylesheet test.css some.svg some.png

However, I came across some issues when trying to replicate those results when using resvg as a library. I started from the miminal example (provided here: resvg/examples/minimal.rs). For some reason, it didn't work as intended. After some digging, it turned out that resvg as a binary expects a path to the stylesheet; however, resvg as a library expects the content of the stylesheet provided as a string. For more detail about the inconsistency, take a look a the dropdown.

More detail

When running `resvg` as a binary, the path provided from the command line is treated as a `PathBuf` and then, the stylesheet's content is read. https://github.com/linebender/resvg/blob/bda7db0d0b2eb32c5f63f90279cec19613945a9d/crates/resvg/src/main.rs#L242 https://github.com/linebender/resvg/blob/bda7db0d0b2eb32c5f63f90279cec19613945a9d/crates/resvg/src/main.rs#L554-L562 On the other hand, the `usvg` parser, used in the minimal example, treats the stylesheet as a `String`. From that point on, it assumes `stylesheet` contains the contents of the stylesheet. https://github.com/linebender/resvg/blob/6df63519f04580bd74e31b25e9ee82d4ace145da/crates/usvg/src/parser/options.rs#L98-L100

I anticipate the difference when using resvg as a library and binary would cause confusion. In my opinion, the best course of action would be to treat the stylesheet as a PathBuf throughout the entire codebase. In addition to establishing consistency, when using the c-api, the additional layer of issues associated with managing memory to read from the file and save to a string would be eliminated.

If this is unintentional, how would like to handle this issue? I'm willing to open to creating another issue and creating a PR for it, but I wanted to get your opinion on it before I do that. On the other hand, if it's intentional, are there any plans to add a function or method that can accept the path to a stylesheet?

@LaurenzV
Copy link
Contributor

LaurenzV commented Dec 26, 2024

This is intentional, resvg as a library should be pure and not have any interaction with the filesystem whatsoever, so I'm afraid we can't change that.

Why can't we just ask the user of the C api to just load the file from their side and then pass the contents to resvg? (EDIT: Nevermind, didn't read the whole thing. :p But I'm afraid there really isn't much we can do, reading the file in resvg itself is both, bad from a security standpoint as well as a portability one.)

@michabay05
Copy link
Author

Understood. Security and portability are important considerations. I've updated my PR for this issue (#873) to account for this fact.

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