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

Improved glyph url handling: accept arbitrary suffixes after {end} #1443 #1444

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bzeiss
Copy link

@bzeiss bzeiss commented Jul 28, 2024

I have attempted to fix #1443 for myself just to challenge myself to write some Rust. I'm a total Rust newbie so please review with care.

This pull requests attempts to solve the problem that many styles out there add suffixes like '.pbf' to the glyph url at the end (see #1443 for examples). The change adds some parsing code to FontRequest that allows arbitrary suffixes after {{end}} and only returns the the leading digits of {{end}} as number for get_font_range. Everything after that is discarded.

I hope that this way, the font server "just works" for most people and don't hit the issue that I did.

Copy link
Contributor

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I have left a different approach below which I think would be simpler

(disclaimer: not a maintainer ^^)

}

#[route(
"/font/{fontstack}/{start}-{end}",
"/font/{fontstack}/{start}-{end}*",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only reason you are adding the above code is to ignore possible trailing exstentsions, right?

Why not restrict this to a pattern like below (and adding it to FontRequest) which should™️ work as far as I read the docs here.

Relevant part:

Replacement markers can optionally specify a regular expression which will be used to decide whether a path segment should match the marker. To specify that a replacement marker should match only a specific set of characters as defined by a regular expression, you must use a slightly extended form of replacement marker syntax. Within braces, the replacement marker name must be followed by a colon, then directly thereafter, the regular expression. The default regular expression associated with a replacement marker [^/]+ matches one or more characters which are not a slash. For example, under the hood, the replacement marker {foo} can more verbosely be spelled as {foo:[^/]+}. You can change this to be an arbitrary regular expression to match an arbitrary sequence of characters, such as {foo:\d+} to match only digits.

I have not tested that it actually works.

Suggested change
"/font/{fontstack}/{start}-{end}*",
"/font/{fontstack}/{start}-{end}{extension:[^\d]*}",

Could you add a testcase to ensure that this works and that there are no regressions?
Actix-webs' testing infratstructure is documented here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good suggestion. I will try this out. Thanks for looking at the change.

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.

Improved handling of glyph urls as commonly used in styles
2 participants