Fix multiplayer sometimes duplicating games (#6999)

* Fix multiplayer sometimes duplicating games

* Fix test compilation

android/src/com/unciv/app/MultiplayerTurnCheckWorker.kt

@@ -264,12 +264,10 @@ class MultiplayerTurnCheckWorker(appContext: Context, workerParams: WorkerParame
     init {
         // We can't use Gdx.files since that is only initialized within a com.badlogic.gdx.backends.android.AndroidApplication.
         // Worker instances may be stopped & recreated by the Android WorkManager, so no AndroidApplication and thus no Gdx.files available
-        val files = DefaultAndroidFiles(applicationContext.assets, ContextWrapper(applicationContext), false)
+        val files = DefaultAndroidFiles(applicationContext.assets, ContextWrapper(applicationContext), true)
         // GDX's AndroidFileHandle uses Gdx.files internally, so we need to set that to our new instance
         Gdx.files = files
-        val externalFilesDirForAndroid = applicationContext.getExternalFilesDir(null)?.path
+        gameSaver = GameSaver(files, null, true)
-        Log.d(LOG_TAG, "Creating new GameSaver with externalFilesDir=[${externalFilesDirForAndroid}]")
-        gameSaver = GameSaver(files, null, externalFilesDirForAndroid)
     }
 
     override fun doWork(): Result = runBlocking {

android/src/com/unciv/app/PlatformSpecificHelpersAndroid.kt

@@ -30,7 +30,9 @@ Sources for Info about current orientation in case need:
         if (activity.requestedOrientation != orientation) activity.requestedOrientation = orientation
     }
 
-    override fun getExternalFilesDir(): String? {
+    /**
-        return activity.getExternalFilesDir(null)?.path
+     * On Android, local is some android-internal data directory which may or may not be accessible by the user.
-    }
+     * External is probably on an SD-card or similar which is always accessible by the user.
+     */
+    override fun shouldPreferExternalStorage(): Boolean = true
 }

core/src/com/unciv/UncivGame.kt

@@ -87,7 +87,7 @@ class UncivGame(parameters: UncivGameParameters) : Game() {
             viewEntireMapForDebug = false
         }
         Current = this
-        gameSaver = GameSaver(Gdx.files, customSaveLocationHelper, platformSpecificHelper?.getExternalFilesDir())
+        gameSaver = GameSaver(Gdx.files, customSaveLocationHelper, platformSpecificHelper?.shouldPreferExternalStorage() == true)
 
         // If this takes too long players, especially with older phones, get ANR problems.
         // Whatever needs graphics needs to be done on the main thread,

core/src/com/unciv/logic/GameSaver.kt

@@ -14,6 +14,7 @@ import com.unciv.ui.crashhandling.launchCrashHandling
 import com.unciv.ui.crashhandling.postCrashHandlingRunnable
 import com.unciv.ui.saves.Gzip
 import com.unciv.utils.Log
+import com.unciv.utils.debug
 import kotlinx.coroutines.Job
 import java.io.File
 
@@ -29,10 +30,12 @@ class GameSaver(
      */
     private val files: Files,
     private val customFileLocationHelper: CustomFileLocationHelper? = null,
-    /** When set, we know we're on Android and can save to the app's personal external file directory
+    private val preferExternalStorage: Boolean = false
-     * See https://developer.android.com/training/data-storage/app-specific#external-access-files */
-    private val externalFilesDirForAndroid: String? = null
 ) {
+    init {
+        debug("Creating GameSaver, localStoragePath: %s, externalStoragePath: %s, preferExternalStorage: %s",
+            files.localStoragePath, files.externalStoragePath, preferExternalStorage)
+    }
     //region Data
 
     var autoSaveJob: Job? = null
@@ -48,11 +51,20 @@ class GameSaver(
     }
 
     private fun getSave(saveFolder: String, gameName: String): FileHandle {
-        val localFile = files.local("${saveFolder}/$gameName")
+        debug("Getting save %s from folder %s, preferExternal: %s",
-        if (externalFilesDirForAndroid.isNullOrBlank() || !files.isExternalStorageAvailable) return localFile
+            gameName, saveFolder, preferExternalStorage, files.externalStoragePath)
-        val externalFile = files.absolute(externalFilesDirForAndroid + "/${saveFolder}/$gameName")
+        val location = "${saveFolder}/$gameName"
-        if (localFile.exists() && !externalFile.exists()) return localFile
+        val localFile = files.local(location)
-        return externalFile
+        val externalFile = files.external(location)
+
+        val toReturn = if (preferExternalStorage && files.isExternalStorageAvailable && (externalFile.exists() || !localFile.exists())) {
+            externalFile
+        } else {
+            localFile
+        }
+
+        debug("Save found: %s", toReturn.file().absolutePath)
+        return toReturn
     }
 
     fun getMultiplayerSaves(): Sequence<FileHandle> {
@@ -66,30 +78,48 @@ class GameSaver(
     }
 
     private fun getSaves(saveFolder: String): Sequence<FileHandle> {
-        val localSaves = files.local(saveFolder).list().asSequence()
+        debug("Getting saves from folder %s, externalStoragePath: %s", saveFolder, files.externalStoragePath)
-        if (externalFilesDirForAndroid.isNullOrBlank() || !files.isExternalStorageAvailable) return localSaves
+        val localFiles = files.local(saveFolder).list().asSequence()
-        return localSaves + files.absolute(externalFilesDirForAndroid + "/${saveFolder}").list().asSequence()
+
+        val externalFiles = if (files.isExternalStorageAvailable) {
+            files.external(saveFolder).list().asSequence()
+        } else {
+            emptySequence()
+        }
+
+        debug("Local files: %s, external files: %s",
+            { localFiles.joinToString(prefix = "[", postfix = "]", transform = { it.file().absolutePath }) },
+            { externalFiles.joinToString(prefix = "[", postfix = "]", transform = { it.file().absolutePath }) })
+        return localFiles + externalFiles
     }
 
     fun canLoadFromCustomSaveLocation() = customFileLocationHelper != null
 
-    /** Deletes a save.
+    /**
-     *  @return `true` if successful.
+     * @return `true` if successful.
-     *  @throws SecurityException when delete access was denied
+     * @throws SecurityException when delete access was denied
-      */
+     */
     fun deleteSave(gameName: String): Boolean {
-        return getSave(gameName).delete()
+        return deleteSave(getSave(gameName))
     }
 
-    fun deleteMultiplayerSave(gameName: String) {
+    /**
-        getMultiplayerSave(gameName).delete()
+     * @return `true` if successful.
+     * @throws SecurityException when delete access was denied
+     */
+    fun deleteMultiplayerSave(gameName: String): Boolean {
+        return deleteSave(getMultiplayerSave(gameName))
     }
 
     /**
      * Only use this with a [FileHandle] obtained by one of the methods of this class!
+     *
+     * @return `true` if successful.
+     * @throws SecurityException when delete access was denied
      */
-    fun deleteSave(file: FileHandle) {
+    fun deleteSave(file: FileHandle): Boolean {
-        file.delete()
+        debug("Deleting save %s", file.path())
+        return file.delete()
     }
 
     interface ChooseLocationResult {
@@ -115,6 +145,7 @@ class GameSaver(
      */
     fun saveGame(game: GameInfo, file: FileHandle, saveCompletionCallback: (Exception?) -> Unit = { if (it != null) throw it }) {
         try {
+            debug("Saving GameInfo %s to %s", game.gameId, file.path())
             file.writeString(gameInfoToString(game), false)
             saveCompletionCallback(null)
         } catch (ex: Exception) {
@@ -136,6 +167,7 @@ class GameSaver(
      */
     fun saveGame(game: GameInfoPreview, file: FileHandle, saveCompletionCallback: (Exception?) -> Unit = { if (it != null) throw it }) {
         try {
+            debug("Saving GameInfoPreview %s to %s", game.gameId, file.path())
             json().toJson(game, file)
             saveCompletionCallback(null)
         } catch (ex: Exception) {
@@ -162,6 +194,7 @@ class GameSaver(
             postCrashHandlingRunnable { saveCompletionCallback(CustomSaveResult(exception = ex)) }
             return
         }
+        debug("Saving GameInfo %s to custom location %s", game.gameId, saveLocation)
         customFileLocationHelper!!.saveGame(gameData, saveLocation) {
             if (it.isSuccessful()) {
                 game.customSaveLocation = it.location

core/src/com/unciv/logic/multiplayer/OnlineMultiplayer.kt

@@ -95,16 +95,19 @@ class OnlineMultiplayer {
 
     private fun updateSavesFromFiles() {
         val saves = gameSaver.getMultiplayerSaves()
+
         val removedSaves = savedGames.keys - saves.toSet()
-        removedSaves.forEach(savedGames::remove)
+        for (saveFile in removedSaves) {
+            deleteGame(saveFile)
+        }
+
         val newSaves = saves - savedGames.keys
         for (saveFile in newSaves) {
-            val game = OnlineMultiplayerGame(saveFile)
+            addGame(saveFile)
-            savedGames[saveFile] = game
-            postCrashHandlingRunnable { EventBus.send(MultiplayerGameAdded(game.name)) }
         }
     }
 
+
     /**
      * Fires [MultiplayerGameAdded]
      *
@@ -123,7 +126,7 @@ class OnlineMultiplayer {
      * @throws FileStorageRateLimitReached if the file storage backend can't handle any additional actions for a time
      * @throws FileNotFoundException if the file can't be found
      */
-    suspend fun addGame(gameId: String, gameName: String? = null): String {
+    suspend fun addGame(gameId: String, gameName: String? = null) {
         val saveFileName = if (gameName.isNullOrBlank()) gameId else gameName
         var gamePreview: GameInfoPreview
         try {
@@ -132,7 +135,7 @@ class OnlineMultiplayer {
             // Game is so old that a preview could not be found on dropbox lets try the real gameInfo instead
             gamePreview = onlineGameSaver.tryDownloadGame(gameId).asPreview()
         }
-        return addGame(gamePreview, saveFileName)
+        addGame(gamePreview, saveFileName)
     }
 
     private fun addGame(newGame: GameInfo) {
@@ -140,12 +143,15 @@ class OnlineMultiplayer {
         addGame(newGamePreview, newGamePreview.gameId)
     }
 
-    private fun addGame(preview: GameInfoPreview, saveFileName: String): String {
+    private fun addGame(preview: GameInfoPreview, saveFileName: String) {
         val fileHandle = gameSaver.saveGame(preview, saveFileName)
+        return addGame(fileHandle, preview)
+    }
+
+    private fun addGame(fileHandle: FileHandle, preview: GameInfoPreview = gameSaver.loadGamePreviewFromFile(fileHandle)) {
         val game = OnlineMultiplayerGame(fileHandle, preview, Instant.now())
         savedGames[fileHandle] = game
         postCrashHandlingRunnable { EventBus.send(MultiplayerGameAdded(game.name)) }
-        return saveFileName
     }
 
     fun getGameByName(name: String): OnlineMultiplayerGame? {
@@ -252,9 +258,17 @@ class OnlineMultiplayer {
      * Fires [MultiplayerGameDeleted]
      */
     fun deleteGame(multiplayerGame: OnlineMultiplayerGame) {
-        val name = multiplayerGame.name
+        deleteGame(multiplayerGame.fileHandle)
-        gameSaver.deleteSave(multiplayerGame.fileHandle)
+    }
-        EventBus.send(MultiplayerGameDeleted(name))
+
+    private fun deleteGame(fileHandle: FileHandle) {
+        gameSaver.deleteSave(fileHandle)
+
+        val game = savedGames[fileHandle]
+        if (game == null) return
+
+        savedGames.remove(game.fileHandle)
+        postCrashHandlingRunnable { EventBus.send(MultiplayerGameDeleted(game.name)) }
     }
 
     /**

core/src/com/unciv/ui/utils/GeneralPlatformSpecificHelpers.kt

@@ -19,7 +19,8 @@ interface GeneralPlatformSpecificHelpers {
     fun notifyTurnStarted() {}
 
     /**
-     * @return an additional external directory for save files, if applicable on the platform
+     * If the GDX [com.badlogic.gdx.Files.getExternalStoragePath] should be preferred for this platform,
+     * otherwise uses [com.badlogic.gdx.Files.getLocalStoragePath]
      */
-    fun getExternalFilesDir(): String? { return null }
+    fun shouldPreferExternalStorage(): Boolean
 }

desktop/src/com/unciv/app/desktop/PlatformSpecificHelpersDesktop.kt

@@ -13,4 +13,6 @@ class PlatformSpecificHelpersDesktop(config: Lwjgl3ApplicationConfiguration) : G
         turnNotifier.turnStarted()
     }
 
+    /** On desktop, external is likely some document folder, while local is the game directory. We'd like to keep everything in the game directory */
+    override fun shouldPreferExternalStorage(): Boolean = false
 }