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

Fix generate Format tag #970

Merged
merged 3 commits into from
Nov 25, 2024
Merged

Fix generate Format tag #970

merged 3 commits into from
Nov 25, 2024

Conversation

tltk90
Copy link
Contributor

@tltk90 tltk90 commented Nov 12, 2024

Could solve #962

@KaiVolland
Copy link
Contributor

Hey @tltk90, thanks for your contribution.

Could you add a new test with a corresponding SLD and geostyler-style?

@@ -1901,13 +1901,19 @@ export class SldStyleParser implements StyleParser<string> {
case 'png':
case 'jpeg':
case 'gif':
graphic[0][ExternalGraphic][0][Format] = [`image/${iconExt}`];
graphic[0][ExternalGraphic][1][Format] = [{
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure the index has to be changed from [ExternalGraphic][0] to one [ExternalGraphic][1] here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. if it is on [ExternalGraphic][0] the Format was ignore.

@tltk90
Copy link
Contributor Author

tltk90 commented Nov 12, 2024

@KaiVolland I think the existing tests are sufficient, I had a spelling error, that's why the tests failed earlier.
I fixed it. The test can write a SLD PointSymbolizer with ExternalGraphic

@KaiVolland
Copy link
Contributor

KaiVolland commented Nov 12, 2024

If the tests were sufficient they should have failed before your MR 😄 . Or in other words; could you add a test that demonstrates the correct translation of a graphic with an extension?

... or just add an SLD to the issue that failed before this PR. I can add it afterwards.

@tltk90
Copy link
Contributor Author

tltk90 commented Nov 16, 2024

Sorry on the delay with the answer,
The "problem" with the tests is the Format tag is not part of the geostyler object,
and in all the tests we you need to compare sld you convert it back to geostyler object,
so you don't catch this problem.

You can see this in the test
can write a SLD 1.1 PointSymbolizer with ExternalGraphic svg
if you print the sldString before convert it back to geostyler you see the format was missing.

@KaiVolland KaiVolland merged commit 0093c6b into geostyler:main Nov 25, 2024
5 of 6 checks passed
@KaiVolland
Copy link
Contributor

Thanks for the contribution @tltk90 🙏

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.

2 participants