View / safeguard against negative hint values to avoid unexpected behaviour

[jahow]

This could happen e.g. in case view.endInteraction() was used too many times, which then might cause unexpected behaviours in dependent logic such as the webgl points renderer.

Fixes #14544

[github-actions]

📦 Preview the website for this branch here: https://deploy-preview-14545--ol-site.netlify.app/.

[ahocevar]

thanks @jahow for taking a stab at this. It looks like your fix is a cure for the symptoms, but not for the root of the problem. In the first place. the view hints should never have a negative value. I'm not sure if it is enough to change endInteractionInternal() to not go below zero, or if the observed behavior is a sign of a more severe problem.

For now, it would probably be ok to safeguard the hints against going below zero, but I'd sleep better if interaction handling would work in a way that the hints would never go below zero without a safeguard.

[jahow]

IIRC the endInteraction method wasn't meant to be called from the application code, but only used by actual interactions such as zoom, pan etc. to signal to the view that it could go back to a stable state. Maybe a better, non public API should be used for this? I don't have much ideas for now.

I agree that this fix isn't great -- especially because other parts of the lib also rely on view hints being specifically zero instead of zero and less.

[ahocevar]

Ah, I see now! I hadn't read the original issue closely enough. So for this case, I think it is enough to safeguard endInteraction() against going below zero. I agree that it shouldn't be an API method, but now that it is, that would be the most sensible fix I think.

[jahow]

I modified the PR to add the safeguard at the View level which is indeed a much more generic fix.

[ahocevar]

I think a better place to add a safeguard would be the API method endInteraction(), by means of checking for getInteracting() . Internally, I hope/think we do things correctly, and we could add a test to ensure that the hint is always >= 0. Otherwise I fear this is prone to problems that are hard to debug.

[jahow]

@ahocevar went with your suggestion! I think this contributes to make the view API safer to use.

[M393]

OpenLayers could output a warning in this case. It's a bug in application code, but maybe shouldn't be ignored silently.

[ahocevar]

To me, this looks great now. Thanks, @jahow !

[jahow]

OpenLayers could output a warning in this case. It's a bug in application code, but maybe shouldn't be ignored silently.

I think making it more permissive is not unreasonable, people might end up writing custom interactions which use begin/endInteraction liberally and calling endInteraction too many times shouldn't cause any serious issue with this PR.

[tschaub]

Apologies for approving without reading the existing discussion. I think this ended up in a good place.