acteng / atip

Active Travel Infrastructure Platform
https://acteng.github.io/atip/
Apache License 2.0
22 stars 4 forks source link

Use current mode from a global store #220

Closed Pete-Y-CS closed 1 year ago

Pete-Y-CS commented 1 year ago

Unfortunately this refactor has surfaced a bug whereby we're handling events multiple times, once for each tool listening to it, regardless of whether the tool is active (sort of we at least send the event to the tool, and let it decide whether it's active or not). This means we have to resolve that event handling and the PR is going to get quite a bit heftier.

Just pushed a initial stab at how we might want to handle events centrally. It is only working for points at the moment, so do not look to routes, polygons, etc.

Short version is that each time a tool wants to register to an event, it goes through a singletonEventManager which basically registers to any events that any components across the entire app care about, and then depending on what mode the user is in, only sends events on to listeners for that event, for that mode

Pete-Y-CS commented 1 year ago

Alright, it is in a somewhat reviewable state @dabreegster @Sparrow0hawk ! I have discovered edit geometry isn't working as I'd hoped, yet. Relatively confident it's just some more plumbing to do but most tools are working as expected as far as I can tell so I think it's a good time for you guys to check how it's working. I've spoken to Alex in person about how we're doing it but if either of you have Qs throw em my way or ask for a call!

Pete-Y-CS commented 1 year ago

Alright, it is in a somewhat reviewable state @dabreegster @Sparrow0hawk ! I have discovered edit geometry isn't working as I'd hoped, yet. Relatively confident it's just some more plumbing to do but most tools are working as expected as far as I can tell so I think it's a good time for you guys to check how it's working. I've spoken to Alex in person about how we're doing it but if either of you have Qs throw em my way or ask for a call!

Yep Geometry mode needs some meaningful plumbing, 😩. I'll get on it but point stands that the rest is reviewable

dabreegster commented 1 year ago

I pushed 3 commits, all showing up at https://github.com/acteng/atip/tree/current-mode-in-global-store. For some reason the PR view here is only showing 2 of them for me.

But anyway, I think with these fixes, this PR is working, including geometry mode. The problem was for addEventListenerSuccess and the others, all the custom event handlers to figure out when point/polygon/route tool has succeeded at something or failed. We need to keep the currentMode == thisMode guard in there. Otherwise from geometry mode, we finish with point tool by clicking, but PointMode.svelte's callback incorrectly fires. I think I misled you on a comment about removing "am I active now?" checks, sorry!

robinlovelace-ate commented 1 year ago

I pushed 3 commits, all showing up at https://github.com/acteng/atip/tree/current-mode-in-global-store. For some reason the PR view here is only showing 2 of them for me.

In case it helps this is what I see:

image

dabreegster commented 1 year ago

Thanks for checking. All 12 commits are now correctly showing up in GH; must've been some partial outage or delays yesterday.

dabreegster commented 1 year ago

Some code review has been happening in DMs on Teams, so summarizing status here. We should fix a few things before merging:

There's some other cleanup that might be possible and useful, but it shouldn't block merging. After this is merged, we can go back to the original feature request in #196 and disable some sidebar actions when we're in drawing modes.

Let me know if you want help with any of this!