search

maplibre / maplibre-gl-directions

A plugin to show routing directions on a MapLibre GL JS map
https://maplibre.org/maplibre-gl-directions/
MIT License
75 stars 16 forks source link

Fix #192, #232 and #233. Refactor some comments. Don't disable interactivity for when there's an ongoing routing request. Properly cancel prev. requests #235

Closed smellyshovel closed 3 months ago

smellyshovel commented 3 months ago

There are many things to this commit.

First of all, it fixes #232 by listening for map's idling after re-drawing necessary features on onDrugUp and onClick events, and when the map begins to idle, it calls the onMove method with the same coordinates the click/drug-up happened at. This could be considered an artifitial mouse movement which was previously required to be performed manually before the map could register a new mousemove event and understand that the cursor is still hovering a waypoint.

Next, there's a fix for #233. There are 2 components to the fix. First, the onDrugUp method was asynchronois and was awaiting the fetchDirections call to finish, and only after that (regardless of whether it finished successfully or not) it detached the drag-specific event listeners. This led to long-taking requests creating wierd interaction-artifacts. Now, instead of being async. and actually waiting for the request to finish, the method only initiates it and proceeds straight away to disabling the (already-ended) drag-related event listeners. This created an issue with the try-catch block not being able to detect errors for requests anymore, but this is solved by moving from try-catch to Promise's catch clause.

The second component of the fix is to simplify the interaction handling for ongoing requests. Previously, we tried to control the flow of enabling/disabling the interaction for the plugin while there were requests being performed, but with this PR I decided not to touch it at all. So, starting from now it's possible to keep interacting with the features of the plugin even while there's a request to load routes from the prev. interaction. And an important change here is that it's not possible anymore to create any confusion between what's requested and what's displayed because any previous request is automatically discarded as soon as there's a new one. This is done by removing the abortController de-initialization after requests, as de-initializing it, because of race-conditions, sometimes de-initialized not the old one, but the new one. This is not the issue anymore.

Thanks to that, as a side-effect, this also fixed #192 as interactivity doesn't have to do anything anymore with fetching the directions.

Additionally, there are some small improvements like:

  1. Refactored comments (some of them which catched my eye)
  2. We don't rely anymore anywhere on the private _interactive field and instead only treat the public interactive flag as the only source of truth.
smellyshovel commented 3 months ago

@joleeee might be interesting for you as well. A review is welcome.

smellyshovel commented 3 months ago

I'm also not sure TBH whether it's a patch or a minor release. On one hand, there are no new features and breaking changes, but on the other hand, if the way we work with the interaction while there's an ongoing request has changed, doesn't it mean that the behavior of the plugin is now different and thus it's a minor release? Or even a breaking change?

Marking it as a patch for now though, as I believe that it's a pretty much inner thing and no one will ever notice the change.

joleeee commented 3 months ago

Nice! It seems way more predictable now. Still having an issue which is perhaps not really encapsulated by #232 and #233. If the route updates while dragging, the waypoint get frozen and is "forgotten". As in, the route does not update from where the waypoint now is, it just freezes mid motion.

https://github.com/user-attachments/assets/b60d9588-a639-4667-9d4d-b150385cf482

joleeee commented 3 months ago

I feel like it's a bit big for just a patch, but at the same time, I feel like this just makes the experience so much better. I can't see why anyone would want to not upgrade.

smellyshovel commented 3 months ago

Nice! It seems way more predictable now. Still having an issue which is perhaps not really encapsulated by #232 and #233. If the route updates while dragging, the waypoint get frozen and is "forgotten". As in, the route does not update from where the waypoint now is, it just freezes mid motion.

rec.mp4

Definitely another effort. Feel free to open a ticket for this one.

UPD. I think @Miguel-Sanches worked on live updates. And I remember it being a huge pain in you know where, but mostly because of these setTimeouts and interactivity enabling/disabling. Am I right? What if we now make it simpler? Just on every mousemove we can call the fetchDirections and they would automatically cancel with each follow-up request. This wasn't possible before as the canceling wasn't working properly. Or am I missing something?

Miguel-Sanches commented 3 months ago

UPD. I think @Miguel-Sanches worked on live updates. And I remember it being a huge pain in you know where, but mostly because of these setTimeouts and interactivity enabling/disabling. Am I right? What if we now make it simpler? Just on every mousemove we can call the fetchDirections and they would automatically cancel with each follow-up request. This wasn't possible before as the canceling wasn't working properly. Or am I missing something?

Yeap, you're right. we tried doing it that way back then but it would fail to work once every 3/4 times due to canceling not working properly.

But we can try to make it simpler now. I think the effort wouldn't be in vain