#352: Add custom map file watcher

- Address review feedback to fix comments, make handlers private, and switch one-liners to expressions
- Switch MapSharer to download zip to a temp directory to avoid race condition with map file watcher.
- Add https://mapdb.cncnet.org/search/ to `downloadmap` help message

Issue: #352
PR: #358

Author: alexlambson

DXMainClient/DXGUI/Multiplayer/GameLobby/CnCNetGameLobby.cs

@@ -95,7 +95,7 @@ public CnCNetGameLobby(WindowManager windowManager, string iniName,
                 "Change the used CnCNet tunnel server (game host only)".L10N("UI:Main:ChangeTunnel"),
                 true, (s) => ShowTunnelSelectionWindow("Select tunnel server:".L10N("UI:Main:SelectTunnelServer"))));
             AddChatBoxCommand(new ChatBoxCommand("DOWNLOADMAP",
-                "Download a map from CNCNet's map server using a map ID and an optional filename.\nExample: \"/downloadmap MAPID [2] My Battle Map\"".L10N("UI:Main:DownloadMapCommandDescription"),
+                "Download a map from CNCNet's map server using a map ID and an optional filename.\nYou can find maps at https://mapdb.cncnet.org/search/\nExample: \"/downloadmap MAPID [2] My Battle Map\"".L10N("UI:Main:DownloadMapCommandDescription"),
                 false, DownloadMapByIdCommand));
         }
 
@@ -1596,31 +1596,22 @@ private void MapSharer_HandleMapDownloadComplete(SHA1EventArgs e)
             string mapFileName = MapSharer.GetMapFileName(e.SHA1, e.MapName);
             Logger.Log("Map " + mapFileName + " downloaded, parsing.");
             string mapPath = MapLoader.CustomMapsDirectory + mapFileName;
-            Map map = MapLoader.LoadCustomMap(mapPath, out string returnMessage);
-            if (map != null)
-            {
-                AddNotice(returnMessage);
-                if (lastMapSHA1 == e.SHA1)
-                {
-                    GameModeMap = MapLoader.GetLoadedMapBySha1(lastMapSHA1);
-                    ChangeMap(GameModeMap);
-                }
-            }
-            else if (chatCommandDownloadedMaps.Contains(e.SHA1))
-            {
-                // Somehow the user has managed to download an already existing sha1 hash.
-                // This special case prevents user confusion from the file successfully downloading but showing an error anyway.
-                AddNotice(returnMessage, Color.Yellow);
-                AddNotice("Map was downloaded, but a duplicate is already loaded from a different filename. This may cause strange behavior.".L10N("UI:Main:DownloadMapCommandDuplicateMapFileLoaded"),
-                    Color.Yellow);
-            }
-            else
+            GameModeMap gameModeMap = MapLoader.GetLoadedMapBySha1(e.SHA1);
+
+            if (gameModeMap == null)
             {
-                AddNotice(returnMessage, Color.Red);
+                AddNotice($"Failed to download map {e.SHA1}", Color.Red);
                 AddNotice("Transfer of the custom map failed. The host needs to change the map or you will be unable to participate in this match.".L10N("UI:Main:MapTransferFailed"));
                 mapSharingConfirmationPanel.SetFailedStatus();
                 channel.SendCTCPMessage(MAP_SHARING_FAIL_MESSAGE + " " + e.SHA1, QueuedMessageType.SYSTEM_MESSAGE, 9);
             }
+
+            AddNotice($"Map {gameModeMap.Map.Name} loaded succesfully.");
+            if (lastMapSHA1 == e.SHA1)
+            {
+                GameModeMap = MapLoader.GetLoadedMapBySha1(lastMapSHA1);
+                ChangeMap(GameModeMap);
+            }
         }
 
         private void MapSharer_MapUploadFailed(object sender, MapEventArgs e) =>
@@ -1789,7 +1780,7 @@ private void DownloadMapByIdCommand(string parameters)
             {
                 // The user did not supply a map name.
                 sha1 = parameters;
-                mapName = "user_chat_command_download";
+                mapName = MapLoader.MAP_CHAT_COMMAND_FILENAME_PREFIX;
             }
             else
             {

DXMainClient/DXGUI/Multiplayer/GameLobby/GameLobbyBase.cs

@@ -2325,14 +2325,11 @@ public bool LoadGameOptionPreset(string name)
         /// <summary>
         /// Handle the GameModeMapsUpdated event from the MapLoader.
         ///
-        /// Updates the gamemode dropdown for new maps being added while the client is running
+        /// Updates the gamemode dropdown for new maps being added while the client is running.
         /// </summary>
         /// <param name="sender"></param>
         /// <param name="e"></param>
-        private void MapLoader_GameModeMapsUpdated(object sender, MapLoaderEventArgs e)
-        {
-            RefreshGameModeDropdown();
-        }
+        private void MapLoader_GameModeMapsUpdated(object sender, MapLoaderEventArgs e) => RefreshGameModeDropdown();
 
         /// <summary>
         /// Update the gamemode dropdown.
@@ -2357,7 +2354,7 @@ public void RefreshGameModeDropdown()
             // Add any new game modes.
             foreach (GameMode gm in GameModeMaps.GameModes)
             {
-                //skip the game mode if it is already in the dropdown.
+                // Skip the game mode if it is already in the dropdown.
                 if (existingDdGameModes.Contains(gm.UIName))
                     continue;
 

DXMainClient/Domain/Multiplayer/CnCNet/MapSharer.cs

@@ -365,18 +365,27 @@ public static string GetMapFileName(string sha1, string mapName)
 
         private static string DownloadMain(string sha1, string myGame, string mapName, out bool success)
         {
-            string customMapsDirectory = SafePath.CombineDirectoryPath(ProgramConstants.GamePath, "Maps", "Custom");
+            string customMapsPath = SafePath.CombineDirectoryPath(ProgramConstants.GamePath, "Maps", "Custom");
+            string tempDownloadPath = SafePath.CombineDirectoryPath(ProgramConstants.GamePath, "Maps", "temp");
 
             string mapFileName = GetMapFileName(sha1, mapName);
 
-            FileInfo destinationFile = SafePath.GetFile(customMapsDirectory, FormattableString.Invariant($"{mapFileName}.zip"));
+            // Store the unzipped mapfile in a temp directory while we unzip and rename it to avoid a race condition with the map file watcher
+            DirectoryInfo tempDownloadDirectory = Directory.CreateDirectory(tempDownloadPath);
+            FileInfo zipDestinationFile = SafePath.GetFile(tempDownloadDirectory.FullName, FormattableString.Invariant($"{mapFileName}.zip"));
+            FileInfo tempMapFile = SafePath.GetFile(tempDownloadDirectory.FullName, FormattableString.Invariant($"{mapFileName}{MapLoader.MAP_FILE_EXTENSION}"));
 
-            // This string is up here so we can check that there isn't already a .map file for this download.
-            // This prevents the client from crashing when trying to rename the unzipped file to a duplicate filename.
-            FileInfo newFile = SafePath.GetFile(customMapsDirectory, FormattableString.Invariant($"{mapFileName}{MapLoader.MAP_FILE_EXTENSION}"));
+            if (zipDestinationFile.Exists)
+            {
+                Logger.Log($"DownloadMain: zipDestinationFile already exists, deleting: zipDestinationFile={zipDestinationFile.FullName}");
+                zipDestinationFile.Delete();
+            }
 
-            destinationFile.Delete();
-            newFile.Delete();
+            if (tempMapFile.Exists)
+            {
+                Logger.Log($"DownloadMain: tempMapFile already exists, deleting: tempMapFile={zipDestinationFile.FullName}");
+                tempMapFile.Delete();
+            }
 
             using (TWebClient webClient = new TWebClient())
             {
@@ -385,7 +394,7 @@ private static string DownloadMain(string sha1, string myGame, string mapName, o
                 try
                 {
                     Logger.Log("MapSharer: Downloading URL: " + "http://mapdb.cncnet.org/" + myGame + "/" + sha1 + ".zip");
-                    webClient.DownloadFile("http://mapdb.cncnet.org/" + myGame + "/" + sha1 + ".zip", destinationFile.FullName);
+                    webClient.DownloadFile("http://mapdb.cncnet.org/" + myGame + "/" + sha1 + ".zip", zipDestinationFile.FullName);
                 }
                 catch (Exception ex)
                 {
@@ -404,27 +413,35 @@ private static string DownloadMain(string sha1, string myGame, string mapName, o
                 }
             }
 
-            destinationFile.Refresh();
+            zipDestinationFile.Refresh();
 
-            if (!destinationFile.Exists)
+            if (!zipDestinationFile.Exists)
             {
                 success = false;
                 return null;
             }
 
-            string extractedFile = ExtractZipFile(destinationFile.FullName, customMapsDirectory);
+            string extractedFile = ExtractZipFile(zipDestinationFile.FullName, tempDownloadDirectory.FullName);
 
             if (String.IsNullOrEmpty(extractedFile))
             {
                 success = false;
                 return null;
             }
 
-            // We can safely assume that there will not be a duplicate file due to deleting it
-            // earlier if one already existed.
-            File.Move(SafePath.CombineFilePath(customMapsDirectory, extractedFile), newFile.FullName);
+            FileInfo newMapFile = SafePath.GetFile(customMapsPath, FormattableString.Invariant($"{mapFileName}{MapLoader.MAP_FILE_EXTENSION}"));
+
+            // We need to delete potentially conflicting map files because .Move won't overwrite.
+            if (newMapFile.Exists)
+            {
+                Logger.Log($"DownloadMain: newMapFile already exists, deleting: newMapFile={newMapFile.FullName}");
+                newMapFile.Delete();
+            }
+
+            File.Move(SafePath.CombineFilePath(tempDownloadDirectory.FullName, extractedFile), newMapFile.FullName);
 
-            destinationFile.Delete();
+            zipDestinationFile.Delete();
+            Directory.Delete(tempDownloadDirectory.FullName, true);
 
             success = true;
             return extractedFile;

DXMainClient/Domain/Multiplayer/MapLoader.cs

@@ -14,6 +14,7 @@ namespace DTAClient.Domain.Multiplayer
     public class MapLoader
     {
         public const string MAP_FILE_EXTENSION = ".map";
+        public const string MAP_CHAT_COMMAND_FILENAME_PREFIX = "user_chat_command_download";
         private const string CUSTOM_MAPS_DIRECTORY = "Maps/Custom";
         private static readonly string CUSTOM_MAPS_CACHE = SafePath.CombineFilePath(ProgramConstants.ClientUserFilesPath, "custom_map_cache");
         private const string MultiMapsSection = "MultiMaps";
@@ -64,20 +65,14 @@ public class MapLoader
         /// </summary>
         /// <param name="sha1">The map ID to search the loaded maps for.</param>
         /// <returns></returns>
-        public bool IsMapAlreadyLoaded(string sha1)
-        {
-            return GetLoadedMapBySha1(sha1) != null;
-        }
+        public bool IsMapAlreadyLoaded(string sha1) => GetLoadedMapBySha1(sha1) != null;
 
         /// <summary>
         /// Search the loaded maps for the SHA-1, return the map if a match is found.
         /// </summary>
         /// <param name="sha1">The map ID to search the loaded maps for.</param>
         /// <returns>The map matching the SHA-1 if one was found.</returns>
-        public GameModeMap GetLoadedMapBySha1(string sha1)
-        {
-            return GameModeMaps.Find(gmm => gmm.Map.SHA1 == sha1);
-        }
+        public GameModeMap GetLoadedMapBySha1(string sha1) => GameModeMaps.Find(gmm => gmm.Map.SHA1 == sha1);
 
         /// <summary>
         /// Loads multiplayer map info asynchonously.
@@ -115,16 +110,16 @@ public void StartCustomMapFileWatcher()
         /// </summary>
         /// <param name="sender">Sent by the file system watcher</param>
         /// <param name="e">Sent by the file system watcher</param>
-        public void HandleCustomMapFolder_Created(object sender, FileSystemEventArgs e)
+        private void HandleCustomMapFolder_Created(object sender, FileSystemEventArgs e)
         {
             // Get the map filename without the extension.
             // The extension gets added in LoadCustomMap so we need to excise it to avoid "file.map.map".
             string name = Path.GetFileNameWithoutExtension(e.Name);
 
-            if (name.StartsWith())
-
             string relativeMapPath = SafePath.CombineFilePath(CustomMapsDirectory, name);
+            Logger.Log($"HandleCustomMapFolder_Created: Calling LoadCustomMap: mapPath={relativeMapPath}");
             Map map = LoadCustomMap(relativeMapPath, out string result);
+            Logger.Log($"HandleCustomMapFolder_Created: Ended LoadCustomMap: mapPath={relativeMapPath}");
 
             if (map == null)
             {
@@ -135,11 +130,11 @@ public void HandleCustomMapFolder_Created(object sender, FileSystemEventArgs e)
         /// <summary>
         /// Handle a .map file being removed from the custom map directory.
         ///
-        /// This function will attempt to remove the map from the client if it was deleted from the folder
+        /// This function will attempt to remove the map from the client if it was deleted from the folder.
         /// </summary>
-        /// <param name="sender">Sent by the file system watcher</param>
+        /// <param name="sender">Sent by the file system watcher.</param>
         /// <param name="e">Sent by the file system watcher.</param>
-        public void HandleCustomMapFolder_Deleted(object sender, FileSystemEventArgs e)
+        private void HandleCustomMapFolder_Deleted(object sender, FileSystemEventArgs e)
         {
             Logger.Log($"Map was deleted: map={e.Name}");
             // Use the filename without the extension so we can remove maps that had their extension changed.
@@ -165,7 +160,7 @@ public void HandleCustomMapFolder_Deleted(object sender, FileSystemEventArgs e)
         /// </summary>
         /// <param name="sender"></param>
         /// <param name="e"></param>
-        public void HandleCustomMapFolder_Renamed(object sender, RenamedEventArgs e)
+        private void HandleCustomMapFolder_Renamed(object sender, RenamedEventArgs e)
         {
             string name = Path.GetFileNameWithoutExtension(e.Name);
             string relativeMapPath = SafePath.CombineFilePath(CustomMapsDirectory, name);
@@ -176,27 +171,30 @@ public void HandleCustomMapFolder_Renamed(object sender, RenamedEventArgs e)
             // This is just for logging to help debug.
             if (!oldPathIsMap && newPathIsMap)
             {
-                Logger.Log($"Renaming file changed the file extension. User is likely renaming a '.yrm' from Final Alert 2: old={e.OldName}, new={e.Name}");
+                Logger.Log($"HandleCustomMapFolder_Renamed: Changed the file extension. User is likely renaming a '.yrm' from Final Alert 2: old={e.OldName}, new={e.Name}");
             }
             else if (oldPathIsMap && !newPathIsMap)
             {
                 // A bit hacky, but this is a rare case.
-                Logger.Log($"Renaming file changed the file extension to no longer be '.map' for some reason, removing from map list: old={e.OldName}, new={e.Name}");
+                Logger.Log($"HandleCustomMapFolder_Renamed: Changed the file extension to no longer be '.map' for some reason, removing from map list: old={e.OldName}, new={e.Name}");
                 HandleCustomMapFolder_Deleted(sender, e);
+                return;
             }
-
-            if (!newPathIsMap)
+            else if (!newPathIsMap)
             {
-                Logger.Log($"Renaming file. New extension is not '{MAP_FILE_EXTENSION}', moving on: file={e.Name}");
+                Logger.Log($"HandleCustomMapFolder_Renamed: New extension is not '{MAP_FILE_EXTENSION}', moving on: file={e.Name}");
                 return;
             }
 
             Map map = LoadCustomMap(relativeMapPath, out string result);
 
-            if (map == null)
+            if (map != null)
             {
-                Logger.Log($"Failed to load renamed map file. Map is likely already loaded: original={e.OldName}, new={e.Name}, reason={result}");
+                Logger.Log($"HandleCustomMapFolder_Renamed: Loaded renamed file as map: file={e.Name}, mapName={map.Name}");
+                return;
             }
+
+            Logger.Log($"Failed to load renamed map file. Map is likely already loaded, filepath: original={e.OldName}, new={e.Name}, reason={result}");
         }
 
         /// <summary>
@@ -206,7 +204,7 @@ public void HandleCustomMapFolder_Renamed(object sender, RenamedEventArgs e)
         /// </summary>
         /// <param name="sender"></param>
         /// <param name="e"></param>
-        public void HandleCustomMapFolder_Error(object sender, ErrorEventArgs e)
+        private void HandleCustomMapFolder_Error(object sender, ErrorEventArgs e)
         {
             Exception exc = e.GetException();
             Logger.Log($"The custom map folder file watcher crashed: error={exc.Message}");
@@ -445,7 +443,7 @@ public Map LoadCustomMap(string mapPath, out string resultMessage)
             // This checks the SHA-1, so duplicate maps in two .map files with different filenames can still be detected.
             if (IsMapAlreadyLoaded(map.SHA1))
             {
-                Logger.Log("LoadCustomMap: Custom map " + customMapFile.FullName + " is already loaded!");
+                Logger.Log($"LoadCustomMap: Custom map {customMapFile.FullName} is already loaded! SHA1={map.SHA1}");
                 resultMessage = $"Map {customMapFile.FullName} is already loaded.";
 
                 return null;

DXMainClient/Domain/Multiplayer/MapLoaderEventArgs.cs

@@ -13,6 +13,5 @@ public MapLoaderEventArgs(Map map)
         }
 
         public Map Map { get; private set; }
-
     }
 }