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

Add 'queryable' property to WMS #57

Merged
merged 5 commits into from
Jul 24, 2024

Conversation

RobQuincey-DC
Copy link
Contributor

This PR adds a boolean queryable property to the WMS model to indicate if a layer is queryable via GetFeatureInfo or not.

The queryable property is an optional attribute on the WMS Layer with either a '1' (is queryable) or '0' (not queryable). If the attribute doesn't exist it should be assumed it is not queryable.

Possible Issues

  • The WMS spec mentions that the queryable attribute can be inherited, but I have never seen a real world example where this occurs. This PR does not handle that possibility and will revert to a layer being not queryable if the attribute doesn't exist on the layer. This could be improved but I'm not sure how and whether its worth the effort at this point.
  • The way the check works simply checks to see if a queryable attribute is equal to the string "1". I see no reason this shouldn't work in all cases, but better implementation ideas are welcome
  • All the tests return false for queryable, there are no tests for a 'true'
  • There are other attributes available on the layer (e.g. opaque) which I've ignored, as queryable is currently the only one I care about.

Copy link
Member

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Looking at the 1.3.0 WMS spec, it appears that queryable can either be 1 or true, so I think it'd make sense to handle both values all the time.

As for tests, please feel free to adjust one of the fixtures to have a queryable=true attribute somewhere;

There are other attributes available on the layer (e.g. opaque) which I've ignored, as queryable is currently the only one I care about.

fair enough 🙂

Thanks!

@RobQuincey-DC
Copy link
Contributor Author

Looking at the 1.3.0 WMS spec, it appears that queryable can either be 1 or true, so I think it'd make sense to handle both values all the time.

Good spot, completely missed that. Should be done now.

As for tests, please feel free to adjust one of the fixtures to have a queryable=true attribute somewhere;

I've added a the "true" and "1" options into the fixtures, even though a real server probably would never do both in the same document!

There are other attributes available on the layer (e.g. opaque) which I've ignored, as queryable is currently the only one I care about.

It was simple enough to add 'opaque' (it follows the same format as queryable) and I can see it being useful, so I did that at the same time, hope that's OK. If not we can easily remove that commit

Copy link
Member

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Wonderful!! thanks!

@jahow jahow merged commit 377887e into camptocamp:main Jul 24, 2024
1 check passed
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