Multiplayer APIv2

[CrsiX]

This may be a larger PR and I'm open to suggestions. Please read on.

Highlights

• in-game chats
• lobbies & invites
• friend system & friend chats
• event notifications
• lazy loading of data
• it works™

What it does

This PR introduces a new library (ktor) and sub-package that speaks with our new multiplayer server, as well as a lot of new UI elements that make use of those new API features, bundled with some new events to integrate the WebSocket smoothly. It already comes with a new Android turn checker as well. For more details, see #8817.

We have carefully checked the new experience. It works pretty well, except for the list of bugs and other issues I created at our fork. Most importantly, I don't want to break backwards-compatibility with APIv0 and APIv1 (that's how I started to call the Dropbox and all the Dropbox-replacement servers, respectively). That means: at the game start (as well as on server change), the server is queried for the /isalive endpoint to determine if it's an "old-style" server. If not (response code 400), it is checked for /api/version. Only if the apiVersion can be matched, you will see the new features. Otherwise, everything should stay the way it currently is: players would not see any difference if they don't use a new APIv2-compatible server.

Note, however, that the servers are not compatible for a specific game. At the moment, you can't create a game in APIv1 and continue playing in APIv2 (or vice versa), and there are no plans to allow that, yet. That means: a party needs to decide on a server to play on beforehand.

API

I bundled all the details in com.unciv.logic.multiplayer.apiv2. It includes the main class APIv2, which holds a lot of other stuff besides the core functionality, which resides in APIv2Wrapper. Accessing functionality is possible like so: UncivGame.Current.onlineMultiplayer.api.auth.login to use the login endpoint, for example. There's also ApiV2FileStorageEmulator which enabled in-place usage of the new functionality whenever the current APIv0/APIv1 would be used (e.g. saving/loading games).

The "game preview" mechanism is not used at all. The server holds a counter dataID, which is incremented every time some player uploads a new game. Checking the value of that counter is enough to detect changed game states.

UX

There are a bunch of new screens, buttons, tables and other elements around. Some important screens:

• A new LobbyBrowserScreen when you click on "multiplayer" in the main menu, which provides a list of your current games and all open lobbies of the current server, as well as buttons to create lobbies, manage friends, ...
• A new MultiplayerGameScreen which you see when you click on the multiplayer button of the WorldScreen, providing the chat, player operations, access to the friends and recent games
• A new LobbyScreen which replaced the NewGameScreen for our purposes here, it also holds a chat window

The user is usually notified by popups about incoming events (e.g. new chat message, lobby invitation, a friendship request, ...).

Missing things

• Most importantly, any kind of persistence: Whenever the code needs some values, the server is queried for them (except if it's just the current operation, then the values are piggy-backed as long as needed). This means you probably see requests whenever some window is opened, because the data is loaded lazy. It doesn't bother or block the UI, though. At the moment, I didn't bother, because a) the server is fast and b) the data is very small, only a few hundred bytes at most usually.
• Android turn check re-connect fixes, since it's pretty hard to keep the WebSocket alive and the data in sync. I've already spent hours tweaking that.
• A few buttons and other elements will just popup with "not implemented yet". Of course, those things will be addressed in the future.
• Joining a lobby by invitation & one-to-one in-game chats (all-player chat exists, though).

Take note

• Changing the server or checking the connection will drop the instance of OnlineMultiplayer now, so that everything is cleaned up properly.
• The NewGameScreen will use horizontally aligned picker buttons instead of vertically aligned as before (provides much more screen space).
• There are no localized strings anywhere, because a) this is a draft and b) I wasn't sure how to do it properly.
• Kotlin builds take seemingly longer now, or is my machine just slow? I thought about splitting all that apiv2 package stuff out, but that didn't work out easily.
• I think I didn't change any existing data structures, so that everything should just continue working. The only exception is the data format of the uploads to APIv2 servers, which is "full" JSON, but since the PR 9427 is merged, this is no specialty anymore.

cc @SomeTroglodyte @touhidurrr

If you checkout and build, I suggest the following patch (it provides better debugging experience when testing on multiple devices):

diff --git a/android/AndroidManifest.xml b/android/AndroidManifest.xml
index e0ef891c2..5cc931a0a 100644
--- a/android/AndroidManifest.xml
+++ b/android/AndroidManifest.xml
@@ -21,7 +21,7 @@
         android:allowBackup="true"
         android:icon="@mipmap/uncivicon"
         android:roundIcon="@mipmap/uncivicon_round"
-        android:label="@string/app_name"
+        android:label="Unciv V2"
         android:isGame="true"
         android:largeHeap="true"
         android:appCategory="game"
diff --git a/android/build.gradle.kts b/android/build.gradle.kts
index cfd212cfd..f35a9c2c9 100644
--- a/android/build.gradle.kts
+++ b/android/build.gradle.kts
@@ -26,7 +26,7 @@ android {
         resources.excludes += "DebugProbesKt.bin"
     }
     defaultConfig {
-        applicationId = "com.unciv.app"
+        applicationId = "com.unciv.app.apiv2"
         minSdk = 21
         targetSdk = 32 // See #5044
         versionCode = BuildConfig.appCodeNumber
diff --git a/android/src/com/unciv/app/AndroidLogBackend.kt b/android/src/com/unciv/app/AndroidLogBackend.kt
index ed32784cc..0efba78c2 100644
--- a/android/src/com/unciv/app/AndroidLogBackend.kt
+++ b/android/src/com/unciv/app/AndroidLogBackend.kt
@@ -18,7 +18,7 @@ class AndroidLogBackend : LogBackend {
     }
 
     override fun isRelease(): Boolean {
-        return !BuildConfig.DEBUG
+        return false // !BuildConfig.DEBUG
     }
 
     override fun getSystemInfo(): String {
diff --git a/core/src/com/unciv/Constants.kt b/core/src/com/unciv/Constants.kt
index 977c70e80..fe9a2f6c2 100644
--- a/core/src/com/unciv/Constants.kt
+++ b/core/src/com/unciv/Constants.kt
@@ -87,6 +87,7 @@ object Constants {
 
     const val dropboxMultiplayerServer = "Dropbox"
     const val uncivXyzServer = "https://uncivserver.xyz"
+    const val runcivServer = "https://runciv.hopfenspace.org"
 
     const val defaultTileset = "HexaRealm"
     const val defaultUnitset = "AbsoluteUnits"
diff --git a/core/src/com/unciv/models/metadata/GameSettings.kt b/core/src/com/unciv/models/metadata/GameSettings.kt
index eafb8e4f8..3be4b6f59 100644
--- a/core/src/com/unciv/models/metadata/GameSettings.kt
+++ b/core/src/com/unciv/models/metadata/GameSettings.kt
@@ -225,7 +225,7 @@ class GameSettingsMultiplayer {
     var passwords = mutableMapOf<String, String>()
     @Suppress("unused")  // @GGuenni knows what he intended with this field
     var userName: String = ""
-    var server = Constants.uncivXyzServer
+    var server = Constants.runcivServer
     var friendList: MutableList<FriendList.Friend> = mutableListOf()
     var turnCheckerEnabled = true
     var turnCheckerPersistentNotificationEnabled = true
diff --git a/core/src/com/unciv/ui/crashhandling/CrashScreen.kt b/core/src/com/unciv/ui/crashhandling/CrashScreen.kt
index 58cdf49ba..6aeedf1cc 100644
--- a/core/src/com/unciv/ui/crashhandling/CrashScreen.kt
+++ b/core/src/com/unciv/ui/crashhandling/CrashScreen.kt
@@ -181,7 +181,7 @@ class CrashScreen(val exception: Throwable): BaseScreen() {
         )
             .onClick {
                 if (copied) {
-                    Gdx.net.openURI("https://github.com/yairm210/Unciv/issues")
+                    Gdx.net.openURI("https://github.com/hopfenspace/Unciv/issues")
                 } else {
                     ToastPopup(
                         "Please copy the error report first.",
diff --git a/desktop/src/com/unciv/app/desktop/DesktopLogBackend.kt b/desktop/src/com/unciv/app/desktop/DesktopLogBackend.kt
index 122c167ef..6cbbe09e2 100644
--- a/desktop/src/com/unciv/app/desktop/DesktopLogBackend.kt
+++ b/desktop/src/com/unciv/app/desktop/DesktopLogBackend.kt
@@ -11,7 +11,7 @@ class DesktopLogBackend : DefaultLogBackend() {
             && System.getProperty("kotlinx.coroutines.debug") == null
 
     override fun isRelease(): Boolean {
-        return release
+        return false
     }
 
     override fun getSystemInfo(): String {

[SomeTroglodyte]

Is that 136 commits in 1 PR? Wow, new record - probably.

I already see one thing to like: CrashScreen.(...).onClick { Gdx.net.openURI("https://github.com/hopfenspace/Unciv/issues") } :joy_cat:

[touhidurrr]

@CrsiX any test version for your it works™? Want to see how it looks like.

[touhidurrr]

Also, I would like the client to send the server some notifications if someone has won, lost, or resigned a game in v2. Also, you are requesting info for all open games I see, But how do you know when a game has been finished? In an users game list, all of his games played ever will be shown if this isn't resolved properly. Also, I would like to implement a rating system.

For all of this reasons, I think this would be a very important feature for v2.

[touhidurrr]

Also, I don't like the naming convention in many cases as it does not clarify what they do. For example: This should be JoinLobbyResponse as it Lobby might have many functions. Which makes what response this is confusing. Similarly:

Not sure what ChatFull does here at all. Not from name or description. Similarly:

Many documentation and naming scheme problems like this.

Also, for clarification, where is the WebSocket docs? Or, schemas = ws docs as I assumed?

[CrsiX]

@CrsiX any test version for your it works™? Want to see how it looks like.

Just compiled for Android and Desktop with the above patch applied now. I would suggest you also compile it locally.

Assuming you use the desktop:

1. Download the .jar
2. Copy it to a second folder
3. Start two Unciv instances in each folder separately
4. Play together with yourself to test the functionality (also extensible to more players, e.g. three or four or five ...)

For all of this reasons, I think this would be a very important feature for v2.

Please add these suggestions to the issue and not this PR, thanks :)

But how do you know when a game has been finished?

I think you got a point here... I may have overlooked something. Indeed, I need to check that out, thanks for spotting. New issue here

Many documentation and naming scheme problems like this.

That's right. I know what the API is doing, since I have implemented that stuff, but of course, it needs clarification and extension. It's not easily understandable just by reading it, yet. However, this is out of scope for that PR as well, please head over to the server repo here for these suggestions.

Also, for clarification, where is the WebSocket docs? Or, schemas = ws docs as I assumed?

Same issue here. At the moment WS docs are only in code: source.

[touhidurrr]

Many documentation and naming scheme problems like this.

That's right. I know what the API is doing, since I have implemented that stuff, but of course, it needs clarification and extension. It's not easily understandable just by reading it, yet. However, this is out of scope for that PR as well, please head over to the server repo here for these suggestions.

In Multiplayer v1, there were only 3 API endpoints initially. Later 2 more endpoints were added for auth. For comparison, in https://runciv.hopfenspace.org/docs/#/ there are 33 endpoints in total. In https://github.com/hopfenspace/runciv/blob/ca1ad4297c8bf5f77a9a475f9f7bd996e306032a/src/chan/ws_manager_chan.rs#L73 there are 15+ ws messages. All in all it's ~50 new types of communication between the client and the server. That's, 10x more complexity added to the table.

For all these reasons, I don't think it is ideal in any way to merge this pr to Unciv without completed documentation. I don't understand why are you treating it as just another feature request. It is clearly not that. Your statement:

However, this is out of scope for that PR as well, please head over to the server repo here for these suggestions. does not make any sense to me.

The necessity of documentation is clearly not out of scope for this pr. It should be a requirement for this pr specifically. We already have a requirement for documenting new Mod features. Multiplayer v2 is not less complex than that. So, a situation where only you understand does not seem ideal to me.

I think this pr needs proper documentation before merging. It already has 6k+ lines and adding some docs for oblivious people like me isn't going to hurt much.

[CrsiX]

For comparison, in https://runciv.hopfenspace.org/docs/#/ there are 33 endpoints in total.

To be fair, we also provide a lot more functionality. It needs to have a place somewhere ;)

I don't think it is ideal in any way to merge this pr to Unciv without completed documentation

You got an absolute right point there. That's one reason I marked this PR as a draft at the moment: it is to collect your ideas and suggestions. I've forwarded your request to the server repo.

I think this pr needs proper documentation before merging. It already has 6k+ lines and adding some docs for oblivious people like me isn't going to hurt much.

Okay, so the aforementioned request for docs was about the API, but that is about the client code, I assume. I already added docs to some of the core classes and methods used in the apiv2 package, but I will also have a look at other places and extend it. Also, I will add a section about it in the docs/ folder, which looks like a good place to hold general overview of what it's doing (even though the docs there might not target the overly technical person, but it would be a good starting point for developers & other interested people).

[github-actions[bot]]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[CrsiX]

@SomeTroglodyte Did you have a look into what I've produced here already? I would like to fix some remaining little quirks and get a working and merge-able state that can be reviewed. I know it's a lot of stuff, that's why I will add docs later that day or the day after. Addressing the merge conflicts would only be the very last step to avoid doing it twenty times.

@yairm210 Do you have any objections against this PR in general?

[SomeTroglodyte]

Did you have a look into what I've produced here already

Oh my, you know I don't do multiplayer, so my motivation is... _low. Add to that 70 files changed in 140 commits, and I get a headache :hammer: .

I ran your branch a while ago, and, well, saw nothing unusual. Browsing the diff I could probably find dozens to hundreds of tiny nitpicks, but from a quick impression nothing having an actual effect (like 'private val' a constructor parameter when it's only used in the constructor; namespace readability - 'Common.thing' when it's actually not very common - needs some qualifier; the new const not applied to old code where appropriate; I would have hidden the MultiplayerStatusButton distinction from the WorldScreen code,...) so, nothing serious.

Point me to some specific design decision to evaluate, then maybe I'll actually look.

By the way, my version of fun nowadays is "did you know - those libraries with a lot of classname.INSTANCE things? open class Thing { companion object : Thing() } works much prettier for classes requiring a global singleton but also temporary loose instances... cool fríjoles... except reflection sees the changed classname..."

[CrsiX]

Oh my, you know I don't do multiplayer, so my motivation is... _low. Add to that 70 files changed in 140 commits, and I get a headache :hammer:

Would it be okay if I try splitting up the bunch of changes into smaller diffs for reviewing? Like, one PR adding the new API stuff, one for integration into existing OnlineMultiplayer functionality, one adding and changing UI and one for the Android stuff? (It would still be thousands of changed lines, though.) Because, I want to produce meaningful & useful code that is actually used later, so I would be happy to help reduce your workload when attempting to merge it. Of course, something like 8000 lines of changes is huge, so if you have any advise for me, I would be happy to help.

By the way, my version of fun nowadays is "did you know ...

I think I don't quite get what you want to say here... Could you please clarify that paragraph? :)

---

Additionally, you can address me in the future if multiplayer-related problems occur.

[SomeTroglodyte]

clarify that paragraph

Really? Just kotlin architecture curiosities... You know companion object { ... } is sold as corresponding to Java class static members, and val foo = object : Bar() { ... } creates a one-off anonymous subclass of Bar - well, turns out the two words in "companion object" haveseparate, independent effects, and you can combine these examples. You can define your class with fields and methods, then add an anonymous subclass of yourself as companion, and that is then a singleton exhibiting all public fields and methods - directly. So with open class Thing { fun that() = "This"; companion object : Thing() } you can call Thing.that() which runs on the singleton, but you can also pull new instances... As said, just a curiosity, and not useful where I tried it, as clean serialization gets so much more of a hassle.

splitting

Of course that would help us all, just not you - I know splitting such a beast can be an incredible chore... And I just PR'ed stuff that moves stuff around, so those if merged first would make your job soooo much harder still. You'll see when you attempt a merge. Parts that go through unchallenged are probably those easy to split off - your new framework - but other stuff will flag tons of little conflicts, each seemingly trivial... And those are likely also candidates for being hard to find criteria for how to actually split so the individual PR's stay functional...

[CrsiX]

Of course that would help us all, just not you

I am very aware of that. But the thing is, that I want it to get merged, so I am the one to go the extra mile... I mean, you "could" accept such a huge PR, but I understand if it's nothing you want to review throughout. So, either I split it up as good as possible or I don't. I would be fine with both. Which path should I choose? ^^

Really? Just kotlin architecture curiosities

Thanks, understood. IMHO, those object singletons should be used sparely, shouldn't they? Thinks like the UncivGame.Current are fine, but having instances of the other things seems much cleaner to me.

[SomeTroglodyte]

that I want it to get merged

I'm all for it, too - I am prejudiced against all online games thanks to some pure spam and spyware vehicles out there, but if at all then quality please... Not to disparage the existing code, but that was done well under minimal KISS premises.

I'm unsure about current merge policy, but relatively certain that while the Boss is in the house he's the one who decides. There was a time his time was limited such that the rule "two collaborators formally approve is enough" was in place, but one - Yair is pretty active and two - I know of only one other collaborator that has been heard in the last weeks... I have phases, so I missed a lot from maybe last August to February.

If nobody vetoes; I'd suggest - you split into at least two parts: First is groundwork concentrating on new files containing non-conflicting objects and other big additions comprising the framework, in a way that the game runs with as little UX changes as possible. I approve based only on playtesting singleplayer, superficially but with good coverage. Maybe you find another tester with the ability to pull and compile to test the MP aspects. You request Yair's review, I comment that I approve pulling a lot of short-term dead code into a release, hopefully he can merge with not too many hours of review work but nevertheless some confidence. The rest - unsure whether splitting major new UI elements off from the lots of small glue changes is worth it, but I imagine one PR affecting many files in small changes - that one should go in shortly after a release, and if planned nobody merges anything in in-between. Same - I only playtest for my approval, this time thoroughly. Lastly, if there's components left out, they'll be easy to merge.

One reason is - architectural analysis would be one thing I sometimes do thoroughly, and then sometimes do come up with insights avoiding future problems, but - my general impression is that you code well and well-organized, so I'm willing to go on pure prejudice for that aspect, as reviewing every single line, even if just superficially, is too much. Or, I'm too lazy. Still haven't watched Marlowe or Sisu...

As for preparation - The Merge to current master will be painful. I'd probably delete this online branch from git, so that it knows the local branch is not tracking a copy, then do a filesystem backup of the tree for good measure (care not to copy links as such but by content), then try a rebase. If too much horror, abort and start a new branch from scratch merging in the backup files with Meld, that's sometimes easier... And also makes meaningful commit separation easier.

object singletons should be used sparely

There's much discussion about that out there, too. As kotlin has no actual namespaces in the same sense as e.g. C# does, using objects as pure namespaces is a valid idea - and if an object has only methods no data, then it will still create that singleton on the heap and initialize it - but all that stays practically a NOP. So for pure method collections they're good.

On the other hand we do have a set of code using top-level declarations without any wrapping class - and one of the authors had valid arguments in favour - but that's very much against my personal taste. I'd wrap everything in meaningful namespaces - at least if there were true support...

Or - enums. Every enum "constant" is actually a full singleton instance of a class. Cool and a good thing :tm: - and extensible. But also very much a matter of taste. (EnumSet is another matter entirely - poor kotlin support, still pure Java, ugly, still using them)

[CrsiX]

I am prejudiced against all online games

Sorry about that, some of them are really great. Any maybe Unciv becomes a little great in that matter, too. That's why I'm doing it :)

Also, thank you for the detailed executions. That clarifies what I'm about to do in the next few days (or weeks, I can't tell, I'm sometimes also pretty slow on coding work). The first PR of adding dependencies & ground work should be comparatively easily to produce, as you already pointed out.

For everything else, well, we will see how painful it becomes :)

---

Regarding the Kotlin stuff. I'm actually new to Kotlin, starting when doing this PR work, so I'm not too into it. I got the essence of it, thanks :) (One may argue that my work should be inspected much more closely, then. But I became used to it rather quickly. After all, it's not entirely different from other languages. It's not like comparing Haskell to Assembly.)

[SomeTroglodyte]

Sorry about that, some of them are really great. Any maybe Unciv becomes a little great in that matter, too. That's why I'm doing it :)

Well, that's why prejudices are fun - or maybe the negative ones not. And Unciv already is a Marble in the dung heap - but go on make it better :partying_face:

Haskell to Assembly

I'm finally forgetting the hexcodes for the 6502 and 8080 instruction sets, so, yeah. Wait - I still remember a few :sob:

[github-actions[bot]]

Conflicts have been resolved.

[SomeTroglodyte]

So that other PR will move ApiV2 forward for realsies, and you keep updating this one to keep the voyage's "destination" current? OK...

Just noticed: Coroutine use that doesn't go through our "Concurrency" module? Will need either documentation why separate or integration. That code reads horribly but so far has proven reliable and easy to use - and gets our CrashScreen into any failure. I can fuzzily imagine that a multiplayer background daemon might want to run outside that framework - but - dox?

[CrsiX]

So that other PR will move ApiV2 forward for realsies, and you keep updating this one to keep the voyage's "destination" current? OK...

Something like this. I think the easiest & most effective way is to keep my track while periodically merging current master and fixing the issues bubbling up during merge. Yesterday, I had a merge with more than 350 affected files, with 12 files having non-trivial conflicts.

So, ideally, I keep up with that branch (dev on my fork) and split its functionality out in smaller PRs to this repo. And when most of them are done, the diff should become small, so that either merging this PR becomes feasible or applying the diff directly is not a too huge headache.

Of course, this also introduces some problems. Like synchronization of changes and so on. But that's why I'm keeping dev updated.

Just noticed: Coroutine use that doesn't go through our "Concurrency" module?

I think I always used Concurrency.run* functionalities to start new coroutines, could you point me where you've been looking? I favor integration about something different, especially since handling concurrency is not so trivial sometimes. Was it in the Android work, possibly? Searching...

[CrsiX]

Android: Since I'm using CoroutineWorker for Android turn checks, the startWork method creates coroutines in another CoroutineScope than what we usually use. I've set that scope to our override val coroutineContext = Dispatcher.DAEMON, is that enough to fix it? If not, I will take a second deeper look.

[SomeTroglodyte]

Just noticed: Coroutine use that doesn't go through our "Concurrency" module?

I think I always used Concurrency.run* functionalities to start new coroutines, could you point me where you've been looking? I favor integration about something different, especially since handling concurrency is not so trivial sometimes. Was it in the Android work, possibly? Searching...

Not sure where, but ~~you~~ someone used Flows which are not part of our wrappers (not yet? or are they), and not in platform-specific code, where I'd understand that you can't use the code helper. Oh - that was already there, you didn't introduce that. So, I trust you to have that quick look why, where exceptions would go, and make it right if even necessary. Or maybe that's planned to be dumped anyway in later PR's, didn't check, sorry.

[CrsiX]

Flows

I've seen them in the init of OnlineMultiplayer. Not sure about their functionality, so I didn't touch them.

didn't check, sorry

No problem, it's valuable input and makes me re-think stuff :)

---

Regarding the crash-handling on Android. My V2 doWork is basically completely wrapped in try-catch, because I don't want things to break. Worst case would some background check error which leads to a crash screen and interrupts a game -- worst user experience. At the moment, I just dump any errors to the logs, where I can read them. Of course, normal players wouldn't, but that's not the relevant part at the moment. :D That being said, I did have some crashes during my tests, that came from deep inside some libraries (once libGDX, once even the Android / Java runtime), but even though they break my turn checker, they shouldn't break the game itself. But since they weren't reproducible, I simply dumped them to the end of the backlog.

[SomeTroglodyte]

makes me re-think stuff

Exactly!

[touhidurrr]

@CrsiX multiplayer v1 is broken it seems.

[CrsiX]

a) Did you use my builds / your own build from my branch? (Just to be sure) b) What exactly did you do? (So that I can reproduce it) c) Can you provide debug logs? (So I can confirm it) d) Are you using https://runciv.hopfenspace.org as the server or something different? I know of problems where you are only using http:// instead, which doesn't work properly yet.

I would like to help investigate and fix any v1 problems as soon as possible :)

[touhidurrr]

@CrsiX, I was trying out your v5.0-alpha2 release with the server address:

https://uncivserver.xyz

Any existing game can be loaded but not uploaded.

[CrsiX]

I wasn't able to reproduce until now, I have opened up https://github.com/hopfenspace/Unciv/issues/82 to keep track of your issue. Thanks for reporting :)

[CrsiX]

I think you found a bug which I already closed in a later version (1 day after the release you tested). See https://github.com/hopfenspace/Unciv/issues/57 for the original. So, in my current dev, this is fixed, but not in the release you've just tested, because it was slightly older. Anyway, thanks for spotting.

[github-actions[bot]]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[CrsiX]

I'm preparing PR 2.

Point me to some specific design decision to evaluate, then maybe I'll actually look.

Please give me your 5 cents on the way the new API is accessed, functionality is used and replaced when the server URL changes.

1. Access is usually done via UncivGame.Current.onlineMultiplayer.api where api is the current ApiV2 instance of the current server. Accessing it will always do a quick check if it has been initialized and is compatible; if not it raises an error.
2. Compatibility is checked by UncivGame.Current.onlineMultiplayer.isInitialized() && UncivGame.Current.onlineMultiplayer.apiVersion == ApiVersion.APIv2, which is a lateinit created by a specific initialize coroutine. Therefore, there's the helper isInitialized, to avoid errors by looking up that value.
3. All calls to UncivGame.Current.onlineMultiplayer.api are usually guarded by a check that the API is compatible before access to avoid the aforementioned error. Here, "usually" means: if the caller, e.g. UI element, is only accessible after such a check, like LobbyBrowserScreen, then there are no further checks -- I don't expect the server URL to change while such a screen is active, even though it could be the case if someone enforces it by refreshOnlineMultiplayer, see below.
4. The default functionality used by the interface FileStorage has already been added, so that's no problem. What will be added next is a more advanced check which file storage "backend" should be used. I thought about making them all suspendable, since this enforces the use of a coroutine and avoids the @Suppress("RedundantSuspendModifier")... At least on my branch the IDE tells me I can remove it.
5. Replacing the server URL now needs a replacement of the OnlineMultiplayer instance (so there will be a helper for that: refreshOnlineMultiplayer here) -- maybe not the best way to do it, but throwing it away is the simplest way to make sure that there are no properties (e.g. apiVersion or so) that lie around somewhere. Also note that OnlineMultiplayer now is dispose-able for proper cleanup.
6. Since I've named my property api, I currently limit the future extensibility a bit for something like APIv3, if that is a problem :) -- Also, is it perfectly clear what it is just by the name, onlineMultiplayer.api?