Make scope parameter of MapState

[lsrom]

Closes #7

Making CoroutineScope parameter of MapState and removing hardcoded Dispatchers.Main from it so it should be platform independant.

[p-lr]

Thanks, I'll have a look soon. Hopefully tomorrow evening.

[p-lr]

Thanks for looking into this. However, I believe that having scope = CoroutineScope(SupervisorJob() + Dispatchers.Main) in the constructor of the state is too much (it complicates the api). A compose application, when running on desktop platform, uses Swing as top most component. The dependency which brings a Dispatcher.Main implementation for swing applications is kotlinx-coroutines-swing. However, it wasn't added as a dependency of the library itself -- it was added as a dependency of the demo app. Consequently, when using the library for a desktop app, there was the "Module with the Main dispatcher is missing" error you mentionned. In the try-fix-#7 branch, I put the dependency at the library level. This should fix you issue. Can please you confirm?

[lsrom]

Imo forcing new dependency is bad practice, plus its a dependency on swing ui library. Fine if you are using swing ui, but bad otherwise.

[p-lr]

Does kotlinx-coroutines-swing causes issue on your app? And if so, which issues? I'm not convinced about kotlinx-coroutines-swing being required only when using swing components. Like I said, compose on desktop is based on swing already. The documentation explicitly says that it bring an implementation for Dispatcher.Main, which is exactly what the library needs.

[lsrom]

I have never used swing coroutines in any multiplatform project before. Doesn't mean you are wrong, just saying that library you want to use shouldn't force it's ways on you especially when it's not necessary.

Also, firs thing in Google documentation on coroutines, is don't hardcode dispatchers, not sure how much more clear can that be. https://developer.android.com/kotlin/coroutines/coroutines-best-practices

Btw, in regards to forcing dependencies onto library users, I noticed another one, kotlinx-io which seems to be required for tile loading. Requiring RawSource in parameters to TileStreamProvider forces the user to add kotlinx-io just to transform InputStream into RawSource. I didnt really explore the code too much to be honest, but I'd suggest moving to simple Input/Output streams, thats much cleaner imho.

Anyway, I'll update the PR with additional change I needed to make it work, if you don't agree with it, feel free to close this :) I'll probably send over at least one more PR, I found one little bug in tile rendering but I dont want to spend more time on this.

[p-lr]

Regarding dispatchers and not hardcoding them, I totally agree. However, there is one very important thing to understand:

The library is designed to perform some operation in the same thread as the one used by the ui framework. In case of Android, it's Dispatchers.Main. What's the equivalent for desktop platform? Certainly not a user injected one in MapState constructor.

Some design change could be done (that is, reconsider this design), but this would require careful thinking and testing. By experience, this kind of change isn't trivial. Since compose will eventually be multi threaded, the rationale behind using Dispathers.Main will no longer apply. So my plan is to think about the design change, which I believe is the proper solution.

[p-lr]

Regarding kotlinx-io, it's the most official dependency I found to bring some equivalent of java's InputStreams. I won't go over all the details, but this will be necessary when reusing image memory will be possible when using skiko.

[p-lr]

I'd like to make another point. Not hardcoding dispatchers is a good pratice for apps, for testatbility notably. However, imagine letting your users tweak around your library multithreading design by exposing your dispatchers. For example, a core compoent is TileCollector. It works using Dispatchers.Default. Try exposing that and let users chose the same dispatcher as the one you'd like to inject. The library would break. So this rule does not apply everywhere.

Now, back to the temporary fix. The documentation of Dispatchers.Main is helpful, especially the end:

A coroutine dispatcher that is confined to the Main thread operating with UI objects.
Usually such dispatchers are single-threaded.

Access to this property may throw an [IllegalStateException] if no main dispatchers are present in the classpath.

Depending on platform and classpath, it can be mapped to different dispatchers:
   - On JVM it is either the Android main thread dispatcher, JavaFx, or Swing EDT dispatcher. It is chosen by the [`ServiceLoader`](https://docs.oracle.com/javase/8/docs/api/java/util/ServiceLoader.html).
   - On JS it is equivalent to the [Default] dispatcher with [immediate][MainCoroutineDispatcher.immediate] support.
   - On Native Darwin-based targets, it is a dispatcher backed by Darwin's main queue.
   - On other Native targets, it is not available.
   - `Dispatchers.setMain` from the `kotlinx-coroutines-test` artifact can replace the main dispatcher with a mock one for testing.

In order to work with the `Main` dispatcher on the JVM, the following artifact should be added to the project runtime dependencies:
  - `kotlinx-coroutines-android` — for Android Main thread dispatcher
  - `kotlinx-coroutines-javafx` — for JavaFx Application thread dispatcher
  - `kotlinx-coroutines-swing` — for Swing EDT dispatcher

[p-lr]

The more I think about the proposed changes, the more I'm convinced this isn't the way to go. Imagine we let user choose which dispatcher the MapState works with for tasks formely dispatched to the "main" thread. Potentially, we can inject Dispatchers.Default or Dispatchers.IO. Honestly, I'm expecting rendering issues. The library currently needs a single threaded dispatcher, and which does not share threads with Dispatchers.Default.

[lsrom]

Regarding the kotlinx-io, I know nothing about that so I can't cast any judgement. With the dispatcher, I still think passing dispatcher in parameter is the best way but after I actually implemented the library in my app I ended up using org.jetbrains.skiko.MainUIDispatcher which uses SwingDispatcher. This doesn't introduce new dependency as the SwitngDispatcher is a copy paste code. There is also a mention in it's docs about not being really equivalent to Dispatchers.Main which I don't know if it's an issue? I only tested basic map with markers but worked fine. So how about scope in the parameter with default value CoroutineScope(SupervisorJob() + MainUIDispatcher)? Probably needs more testing but it gives the user the choice and also provides (probably) safe way to run the library if they don't want to mess with threading themselves. And sorry for dragging this on for so long :)