-
-
Notifications
You must be signed in to change notification settings - Fork 218
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
Conversation
…plibre#1443 - Fixed rust formatting
…plibre#1443 - Fixed clippy error
…plibre#1443 - Fixed follow up clippy error
There was a problem hiding this 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}*", |
There was a problem hiding this comment.
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.
"/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
There was a problem hiding this comment.
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.
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.