fix: IllegalStateException on traffic POI clicks by refactoring map listeners#846
fix: IllegalStateException on traffic POI clicks by refactoring map listeners#846
Conversation
Code Coverage
|
|
Hi @bubenheimer. This attempts to fix #820 With the current code structure, my reasoning is: the renderer does some internal checks when you click a traffic element; if you swap or re-register the listener at that moment (which Compose was doing during recomposition), you break the SDK's internal expectations and trigger that IllegalStateException. The MapApplier registers the listenr exactly once. I am not yet sure of the best solution for this. |
|
The way this code is supposed to work (as far as I remember): the only situation where Compose re-registers a listener is the special case where the listener is toggled between null and non-null. That is special-cased because the SDK treated having a "null" listener specially, with some side effect, and there was a Github issue about it for one of the listeners. There is no re-registering of listeners otherwise; Compose swaps out implementation details (a delegate lambda) behind the scene. You think this swapping may be causing a problem here? I doubt you can get away without either swapping or re-registering the listener, as you will regress on various bugs. The code minimizes re-registering listeners because of potential performance concerns: the SDK is viewed as a black box and Compose recomposition might swap listeners constantly, which could be sluggish if the SDK made that somewhat expensive with synchronization or such. There is a hint of concurrent execution In the stacktrace from #820; the recomposition and swapping approach was written with single-threaded main thread execution in mind, so I wouldn't rule out potential complications from that, if this is what you are thinking. |
|
Looking at it again, the #820 stacktrace looks focused on main thread. |
|
I have been thinking further about this issue. I am still trying to find the best way to get this fixed, but my understanding: traffic-managed POIs like "blocked roads" require the |
|
The SDK would not see changes unless there is a bug or the user explicitly swaps between null and non-null listeners. A bug is certainly a possibility, the code in that area has seen many changes since the listeners redesign, and Compose has changed, too. In any case, just trying to offer helpful pointers, I don't depend on this library anymore. |
Draft PR