Restructure save games into a zipped bundle.

This is the first step toward bundling all assets related to a save game
into a single item. That makes it easier to avoid clobbering "temporary"
assets from other games like the state.json, but also makes it easier
for players to file bug reports, since there's only a single asset to
upload.

This is only the first step because so far it only includes the various
save files: start of turn, end of last turn before results processing,
and "latest" (the game saved explicitly by the player).

.coveragerc

@@ -0,0 +1,4 @@
+[report]
+exclude_lines =
+    pragma: no cover
+    if TYPE_CHECKING:

.github/ISSUE_TEMPLATE/bug-report.yml

@@ -53,9 +53,9 @@ body:
       description: >
         Attach any files needed to reproduce the bug here. **A save game is
         required.** We typically cannot help without a save game (the
-        `.liberation` file found in `%USERPROFILE%/Saved
-        Games/DCS/Liberation/Saves`), so most bugs filed without saved games
-        will be closed without investigation.
+        `.liberation` (or `.liberation.zip`, for 7.x) file found in
+        `%USERPROFILE%/Saved Games/DCS/Liberation/Saves`), so most bugs filed
+        without saved games will be closed without investigation.
 
 
         Other useful files to include are:
@@ -74,7 +74,9 @@ body:
         The `state.json` file for the most recently completed turn, located at
         `<Liberation install directory>/state.json`. This file is essential for
         investigating any issues with end-of-turn results processing. **If you
-        include this file, also include `last_turn.liberation`.**
+        include this file, also include `last_turn.liberation`** (unless the
+        save is from 7.x or newer, which includes that information in the save
+        automatically).
 
 
         You can attach files to the bug by dragging and dropping the file into

.gitignore

@@ -6,6 +6,7 @@ venv
 .DS_Store
 .vscode/settings.json
 dist/**
+/.coverage
 # User-specific stuff
 .idea/
 .env

changelog.md

@@ -4,6 +4,7 @@ Saves from 6.x are not compatible with 7.0.
 
 ## Features/Improvements
 
+* **[Engine]** Saved games are now a zip file of save assets for easier bug reporting. The new extension is .liberation.zip. Drag and drop that file into bug reports.
 * **[Mission Generation]** Units on the front line are now hidden on MFDs.
 * **[Modding]** Updated Community A-4E-C mod version support to 2.1.0 release.
 

game/campaignloader/campaign.py

@@ -10,7 +10,7 @@ from typing import Any, Dict, Tuple
 import yaml
 from packaging.version import Version
 
-from game import persistency
+from game import persistence
 from game.profiling import logged_duration
 from game.theater import (
     ConflictTheater,
@@ -171,7 +171,7 @@ class Campaign:
     @classmethod
     def iter_campaign_defs(cls) -> Iterator[Path]:
         yield from cls.iter_campaigns_in_dir(
-            Path(persistency.base_path()) / "Liberation/Campaigns"
+            Path(persistence.base_path()) / "Liberation/Campaigns"
         )
         yield from cls.iter_campaigns_in_dir(Path("resources/campaigns"))
 

game/factions/factionloader.py

@@ -1,10 +1,11 @@
 from __future__ import annotations
+
 import json
 import logging
 from pathlib import Path
 from typing import Dict, Iterator, List, Optional, Type
 
-from game import persistency
+from game import persistence
 from game.factions.faction import Faction
 
 FACTION_DIRECTORY = Path("./resources/factions/")
@@ -30,7 +31,7 @@ class FactionLoader:
 
     @classmethod
     def load_factions(cls: Type[FactionLoader]) -> Dict[str, Faction]:
-        user_faction_path = Path(persistency.base_path()) / "Liberation/Factions"
+        user_faction_path = Path(persistence.base_path()) / "Liberation/Factions"
         files = cls.find_faction_files_in(
             FACTION_DIRECTORY
         ) + cls.find_faction_files_in(user_faction_path)

game/game.py

@@ -21,12 +21,13 @@ from game.ground_forces.ai_ground_planner import GroundPlanner
 from game.models.game_stats import GameStats
 from game.plugins import LuaPluginManager
 from game.utils import Distance
-from . import naming, persistency
+from . import naming
 from .ato.flighttype import FlightType
 from .campaignloader import CampaignAirWingConfig
 from .coalition import Coalition
 from .db.gamedb import GameDb
 from .infos.information import Information
+from .persistence import SaveManager
 from .profiling import logged_duration
 from .settings import Settings
 from .theater import ConflictTheater
@@ -114,7 +115,7 @@ class Game:
         # Culling Zones are for areas around points of interest that contain things we may not wish to cull.
         self.__culling_zones: List[Point] = []
         self.__destroyed_units: list[dict[str, Union[float, str]]] = []
-        self.savepath = ""
+        self.save_manager = SaveManager(self)
         self.current_unit_id = 0
         self.current_group_id = 0
         self.name_generator = naming.namegen
@@ -321,6 +322,9 @@ class Game:
         # *any* state to the UI yet, so it will need to do a full draw once we do.
         self.initialize_turn(GameUpdateEvents())
 
+    def save_last_turn_state(self) -> None:
+        self.save_manager.save_last_turn()
+
     def pass_turn(self, no_action: bool = False) -> None:
         """Ends the current turn and initializes the new turn.
 
@@ -336,7 +340,7 @@ class Game:
             # Only save the last turn state if the turn was skipped. Otherwise, we'll
             # end up saving the game after we've already applied the results, making
             # this useless...
-            persistency.save_last_turn_state(self)
+            self.save_manager.save_last_turn()
 
         events = GameUpdateEvents()
 
@@ -349,8 +353,7 @@ class Game:
 
         EventStream.put_nowait(events)
 
-        # Autosave progress
-        persistency.autosave(self)
+        self.save_manager.save_start_of_turn()
 
     def check_win_loss(self) -> TurnState:
         player_airbases = {

game/layout/layoutloader.py

@@ -1,9 +1,9 @@
 from __future__ import annotations
-from collections import defaultdict
 
 import itertools
 import logging
 import pickle
+from collections import defaultdict
 from concurrent.futures import ThreadPoolExecutor
 from pathlib import Path
 from typing import Iterator
@@ -13,18 +13,18 @@ import yaml
 from dcs import Point
 from dcs.unitgroup import StaticGroup
 
-from game import persistency
+from game import persistence
 from game.data.groups import GroupRole
 from game.layout.layout import (
+    AntiAirLayout,
+    BuildingLayout,
+    DefensesLayout,
+    GroundForceLayout,
+    LayoutUnit,
+    NavalLayout,
     TgoLayout,
     TgoLayoutGroup,
     TgoLayoutUnitGroup,
-    LayoutUnit,
-    AntiAirLayout,
-    BuildingLayout,
-    NavalLayout,
-    GroundForceLayout,
-    DefensesLayout,
 )
 from game.layout.layoutmapping import LayoutMapping
 from game.profiling import logged_duration
@@ -63,7 +63,7 @@ class LayoutLoader:
         """This will load all pre-loaded layouts from a pickle file.
         If pickle can not be loaded it will import and dump the layouts"""
         # We use a pickle for performance reasons. Importing takes many seconds
-        file = Path(persistency.base_path()) / LAYOUT_DUMP
+        file = Path(persistence.base_path()) / LAYOUT_DUMP
         if file.is_file():
             # Load from pickle if existing
             with file.open("rb") as f:
@@ -106,7 +106,7 @@ class LayoutLoader:
         self._dump_templates()
 
     def _dump_templates(self) -> None:
-        file = Path(persistency.base_path()) / LAYOUT_DUMP
+        file = Path(persistence.base_path()) / LAYOUT_DUMP
         dump = (VERSION, self._layouts)
         with file.open("wb") as fdata:
             pickle.dump(dump, fdata)

game/persistence/__init__.py

@@ -0,0 +1,2 @@
+from .paths import base_path, set_dcs_save_game_directory
+from .savemanager import SaveManager

game/persistence/paths.py

@@ -0,0 +1,26 @@
+from __future__ import annotations
+
+from pathlib import Path
+
+_dcs_saved_game_folder: Path | None = None
+
+
+def set_dcs_save_game_directory(user_folder: Path) -> None:
+    global _dcs_saved_game_folder
+    _dcs_saved_game_folder = user_folder
+    if not save_dir().exists():
+        save_dir().mkdir(parents=True)
+
+
+def base_path() -> str:
+    global _dcs_saved_game_folder
+    assert _dcs_saved_game_folder is not None
+    return str(_dcs_saved_game_folder)
+
+
+def save_dir() -> Path:
+    return Path(base_path()) / "Liberation" / "Saves"
+
+
+def mission_path_for(name: str) -> Path:
+    return Path(base_path()) / "Missions" / name

game/persistence/savegamebundle.py

@@ -0,0 +1,103 @@
+from __future__ import annotations
+
+import pickle
+import shutil
+from pathlib import Path
+from tempfile import NamedTemporaryFile
+from typing import TYPE_CHECKING
+from zipfile import ZIP_LZMA, ZipFile
+
+from game.profiling import logged_duration
+from game.zipfileext import ZipFileExt
+
+if TYPE_CHECKING:
+    from game import Game
+
+
+class SaveGameBundle:
+    """The bundle of saved game assets.
+
+    A save game bundle includes the pickled game object (as well as some backups of
+    other game states, like the turn start and previous turn) and the state.json.
+    """
+
+    MANUAL_SAVE_NAME = "player.liberation"
+    LAST_TURN_SAVE_NAME = "last_turn.liberation"
+    START_OF_TURN_SAVE_NAME = "start_of_turn.liberation"
+
+    def __init__(self, bundle_path: Path) -> None:
+        self.bundle_path = bundle_path
+
+    def save_player(self, game: Game, copy_from: SaveGameBundle | None) -> None:
+        """Writes the save game manually created by the player.
+
+        This save is the one created whenever the player presses save or save-as.
+        """
+        with logged_duration("Saving game"):
+            self._update_bundle_member(game, self.MANUAL_SAVE_NAME, copy_from)
+
+    def save_last_turn(self, game: Game) -> None:
+        """Writes the save for the state of the previous turn.
+
+        This save is the state of the game before the state.json changes are applied.
+        This is mostly useful as a debugging tool for bugs that occur in the turn
+        transition, but can also be used by players to "rewind" to the previous turn.
+        """
+        with logged_duration("Saving last turn"):
+            self._update_bundle_member(game, self.LAST_TURN_SAVE_NAME, copy_from=self)
+
+    def save_start_of_turn(self, game: Game) -> None:
+        """Writes the save for the state at the start of the turn.
+
+        This save is the state of the game immediately after the state.json is applied.
+        It can be used by players to "rewind" to the start of the turn.
+        """
+        with logged_duration("Saving start of turn"):
+            self._update_bundle_member(
+                game, self.START_OF_TURN_SAVE_NAME, copy_from=self
+            )
+
+    def load_player(self) -> Game:
+        """Loads the save manually created by the player via save/save-as."""
+        return self._load_from(self.MANUAL_SAVE_NAME)
+
+    def load_start_of_turn(self) -> Game:
+        """Loads the save automatically created at the start of the turn."""
+        return self._load_from(self.START_OF_TURN_SAVE_NAME)
+
+    def load_last_turn(self) -> Game:
+        """Loads the save automatically created at the end of the last turn."""
+        return self._load_from(self.LAST_TURN_SAVE_NAME)
+
+    def _load_from(self, name: str) -> Game:
+        with ZipFile(self.bundle_path) as zip_bundle:
+            with zip_bundle.open(name, "r") as save:
+                game = pickle.load(save)
+                game.save_bundle = self
+                return game
+
+    def _update_bundle_member(
+        self, game: Game, name: str, copy_from: SaveGameBundle | None
+    ) -> None:
+        # Perform all save work in a copy of the current save to avoid corrupting the
+        # save if there's an error while saving.
+        with NamedTemporaryFile(
+            "wb", suffix=".liberation.zip", delete=False
+        ) as temp_save_file:
+            temp_file_path = Path(temp_save_file.name)
+
+        # We don't have all the state to create the temporary save from scratch (no last
+        # turn, start of turn, etc.), so copy the existing save to create the temp save.
+        #
+        # Python doesn't actually support overwriting or removing zipfile members, so we
+        # have to create a new zipfile and copy over only the files that we won't be
+        # writing.
+        if copy_from is not None and copy_from.bundle_path.exists():
+            shutil.copy(copy_from.bundle_path, temp_file_path)
+            ZipFileExt.remove_member(temp_file_path, name, missing_ok=True)
+
+        with ZipFile(temp_file_path, "a", compression=ZIP_LZMA) as zip_bundle:
+            with zip_bundle.open(name, "w") as entry:
+                pickle.dump(game, entry)
+
+        temp_file_path.replace(self.bundle_path)

game/persistence/savemanager.py

@@ -0,0 +1,80 @@
+from __future__ import annotations
+
+from collections.abc import Iterator
+from contextlib import contextmanager
+from pathlib import Path
+from typing import TYPE_CHECKING
+
+from qt_ui import liberation_install
+from .paths import save_dir
+from .savegamebundle import SaveGameBundle
+
+if TYPE_CHECKING:
+    from game.game import Game
+
+
+class SaveManager:
+    def __init__(self, game: Game) -> None:
+        self.game = game
+        self.player_save_location: Path | None = None
+        self.autosave_path = self.default_save_directory() / "autosave.liberation.zip"
+        self.last_saved_bundle: SaveGameBundle | None = None
+
+    @property
+    def default_save_location(self) -> Path:
+        if self.player_save_location is not None:
+            return self.player_save_location
+        return self.autosave_path
+
+    @property
+    def default_save_bundle(self) -> SaveGameBundle:
+        return SaveGameBundle(self.default_save_location)
+
+    def save_player(self, override_destination: Path | None = None) -> None:
+        copy_from = self.last_saved_bundle
+        with self._save_bundle_context(override_destination) as bundle:
+            self.player_save_location = bundle.bundle_path
+            bundle.save_player(self.game, copy_from)
+            liberation_install.setup_last_save_file(str(bundle.bundle_path))
+            liberation_install.save_config()
+
+    def save_last_turn(self) -> None:
+        with self._save_bundle_context() as bundle:
+            bundle.save_last_turn(self.game)
+
+    def save_start_of_turn(self) -> None:
+        with self._save_bundle_context() as bundle:
+            bundle.save_start_of_turn(self.game)
+
+    @contextmanager
+    def _save_bundle_context(
+        self, override_destination: Path | None = None
+    ) -> Iterator[SaveGameBundle]:
+        if override_destination is not None:
+            bundle = SaveGameBundle(override_destination)
+        else:
+            bundle = self.default_save_bundle
+
+        previous_saved_bundle = self.last_saved_bundle
+        try:
+            self.last_saved_bundle = bundle
+            yield bundle
+        except Exception:
+            self.last_saved_bundle = previous_saved_bundle
+            raise
+
+    @staticmethod
+    def load_last_turn(bundle_path: Path) -> Game:
+        return SaveGameBundle(bundle_path).load_last_turn()
+
+    @staticmethod
+    def load_start_of_turn(bundle_path: Path) -> Game:
+        return SaveGameBundle(bundle_path).load_start_of_turn()
+
+    @staticmethod
+    def load_player_save(bundle_path: Path) -> Game:
+        return SaveGameBundle(bundle_path).load_player()
+
+    @staticmethod
+    def default_save_directory() -> Path:
+        return save_dir()

game/persistency.py

@@ -1,87 +0,0 @@
-from __future__ import annotations
-
-import logging
-import pickle
-import shutil
-from pathlib import Path
-from typing import Optional, TYPE_CHECKING
-
-from game.profiling import logged_duration
-
-if TYPE_CHECKING:
-    from game import Game
-
-_dcs_saved_game_folder: Optional[str] = None
-
-
-def setup(user_folder: str) -> None:
-    global _dcs_saved_game_folder
-    _dcs_saved_game_folder = user_folder
-    if not save_dir().exists():
-        save_dir().mkdir(parents=True)
-
-
-def base_path() -> str:
-    global _dcs_saved_game_folder
-    assert _dcs_saved_game_folder
-    return _dcs_saved_game_folder
-
-
-def save_dir() -> Path:
-    return Path(base_path()) / "Liberation" / "Saves"
-
-
-def _temporary_save_file() -> Path:
-    return save_dir() / "tmpsave.liberation"
-
-
-def _autosave_path() -> str:
-    return str(save_dir() / "autosave.liberation")
-
-
-def mission_path_for(name: str) -> Path:
-    return Path(base_path()) / "Missions" / name
-
-
-def load_game(path: str) -> Optional[Game]:
-    with open(path, "rb") as f:
-        try:
-            save = pickle.load(f)
-            save.savepath = path
-            return save
-        except Exception:
-            logging.exception("Invalid Save game")
-            return None
-
-
-def save_game(game: Game, destination: Path | None = None) -> None:
-    if destination is None:
-        destination = Path(game.savepath)
-
-    temp_save_file = _temporary_save_file()
-    with logged_duration("Saving game"):
-        try:
-            with temp_save_file.open("wb") as f:
-                pickle.dump(game, f)
-            shutil.copy(temp_save_file, destination)
-        except Exception:
-            logging.exception("Could not save game")
-
-
-def autosave(game: Game) -> bool:
-    """
-    Autosave to the autosave location
-    :param game: Game to save
-    :return: True if saved succesfully
-    """
-    try:
-        with open(_autosave_path(), "wb") as f:
-            pickle.dump(game, f)
-        return True
-    except Exception:
-        logging.exception("Could not save game")
-        return False
-
-
-def save_last_turn_state(game: Game) -> None:
-    save_game(game, save_dir() / "last_turn.liberation")

game/sim/missionsimulation.py

@@ -5,7 +5,6 @@ from datetime import timedelta
 from pathlib import Path
 from typing import Optional, TYPE_CHECKING
 
-from game import persistency
 from game.debriefing import Debriefing
 from game.missiongenerator import MissionGenerator
 from game.unitmap import UnitMap
@@ -74,7 +73,7 @@ class MissionSimulation:
                 "was generated."
             )
 
-        persistency.save_last_turn_state(self.game)
+        self.game.save_last_turn_state()
         MissionResultsProcessor(self.game).commit(debriefing, events)
 
     def finish(self) -> None:

game/squadrons/squadrondefloader.py

@@ -3,7 +3,7 @@ from __future__ import annotations
 import logging
 from collections import defaultdict
 from pathlib import Path
-from typing import Iterator, Tuple, TYPE_CHECKING
+from typing import Iterator, TYPE_CHECKING, Tuple
 
 from game.dcs.aircrafttype import AircraftType
 from .squadrondef import SquadronDef
@@ -20,9 +20,9 @@ class SquadronDefLoader:
 
     @staticmethod
     def squadron_directories() -> Iterator[Path]:
-        from game import persistency
+        from game import persistence
 
-        yield Path(persistency.base_path()) / "Liberation/Squadrons"
+        yield Path(persistence.base_path()) / "Liberation/Squadrons"
         yield Path("resources/squadrons")
 
     def load(self) -> dict[AircraftType, list[SquadronDef]]:

game/zipfileext.py

@@ -0,0 +1,34 @@
+import shutil
+from pathlib import Path
+from tempfile import TemporaryDirectory
+from zipfile import ZipFile
+
+
+class ZipFileExt:
+    @staticmethod
+    def remove_member(path: Path, name: str, missing_ok: bool = False) -> None:
+        """Replaces the archive with a copy that excludes one member.
+
+        This is needed to workaround Python's lack of a ZipFile.remove() or a way to
+        overwrite existing members. Attempting to update a member in a zipfile will
+        write a duplicate entry: https://github.com/python/cpython/issues/51067.
+        """
+        with ZipFile(path, "r") as zip_file:
+            if name not in zip_file.namelist():
+                if missing_ok:
+                    return
+                raise ValueError(f"Cannot override {name} as it does not exist")
+
+        # Doing this by extracting all the files to a temporary directory is faster than
+        # reading and writing a file at a time (1-5 seconds vs 0.5 seconds for a save
+        # bundle).
+        with TemporaryDirectory() as temp_dir_str:
+            temp_dir = Path(temp_dir_str)
+            shutil.unpack_archive(path, temp_dir)
+            (temp_dir / name).unlink()
+            shutil.make_archive(
+                # shutil.make_archive automatically adds the extension
+                str(path.with_suffix("")),
+                "zip",
+                root_dir=temp_dir_str,
+            )

qt_ui/liberation_install.py

@@ -6,7 +6,7 @@ from shutil import copyfile
 
 import dcs
 
-from game import persistency
+from game import persistence
 
 global __dcs_saved_game_directory
 global __dcs_installation_directory
@@ -61,7 +61,7 @@ def init():
             __dcs_installation_directory = ""
 
         is_first_start = True
-    persistency.setup(__dcs_saved_game_directory)
+    persistence.set_dcs_save_game_directory(Path(__dcs_saved_game_directory))
     return is_first_start
 
 
@@ -70,10 +70,10 @@ def setup(saved_game_dir, install_dir):
     global __dcs_installation_directory
     __dcs_saved_game_directory = saved_game_dir
     __dcs_installation_directory = install_dir
-    persistency.setup(__dcs_saved_game_directory)
+    persistence.set_dcs_save_game_directory(Path(__dcs_saved_game_directory))
 
 
-def setup_last_save_file(last_save_file):
+def setup_last_save_file(last_save_file: str) -> None:
     global __last_save_file
     __last_save_file = last_save_file
 
@@ -114,11 +114,10 @@ def set_ignore_empty_install_directory(value: bool):
     __ignore_empty_install_directory = value
 
 
-def get_last_save_file():
+def get_last_save_file() -> Path | None:
     global __last_save_file
-    print(__last_save_file)
     if os.path.exists(__last_save_file):
-        return __last_save_file
+        return Path(__last_save_file)
     else:
         return None
 

qt_ui/main.py

@@ -13,7 +13,7 @@ from PySide6.QtGui import QPixmap
 from PySide6.QtWidgets import QApplication, QCheckBox, QSplashScreen
 from dcs.payloads import PayloadDirectories
 
-from game import Game, VERSION, logging_config, persistency
+from game import Game, VERSION, logging_config, persistence
 from game.campaignloader.campaign import Campaign, DEFAULT_BUDGET
 from game.data.weapons import Pylon, Weapon, WeaponGroup
 from game.dcs.aircrafttype import AircraftType
@@ -85,14 +85,14 @@ def run_ui(game: Game | None, ui_flags: UiFlags) -> None:
         window = QLiberationFirstStartWindow()
         window.exec_()
 
-    logging.info("Using {} as 'Saved Game Folder'".format(persistency.base_path()))
+    logging.info("Using {} as 'Saved Game Folder'".format(persistence.base_path()))
     logging.info(
         "Using {} as 'DCS installation folder'".format(
             liberation_install.get_dcs_install_directory()
         )
     )
 
-    inject_custom_payloads(Path(persistency.base_path()))
+    inject_custom_payloads(Path(persistence.base_path()))
 
     # Splash screen setup
     pixmap = QPixmap("./resources/ui/splash_screen.png")
@@ -271,7 +271,7 @@ def create_game(
     # Without this, it is not possible to use next turn (or anything that needs to check
     # for loadouts) without saving the generated campaign and reloading it the normal
     # way.
-    inject_custom_payloads(Path(persistency.base_path()))
+    inject_custom_payloads(Path(persistence.base_path()))
     campaign = Campaign.from_file(campaign_path)
     theater = campaign.load_theater(advanced_iads)
     generator = GameGenerator(

qt_ui/widgets/QTopPanel.py

@@ -11,7 +11,7 @@ from PySide6.QtWidgets import (
 )
 
 import qt_ui.uiconstants as CONST
-from game import Game, persistency
+from game import Game, persistence
 from game.ato.package import Package
 from game.ato.traveltime import TotEstimator
 from game.profiling import logged_duration
@@ -290,7 +290,7 @@ class QTopPanel(QFrame):
             with logged_duration("Simulating to first contact"):
                 self.sim_controller.run_to_first_contact()
         self.sim_controller.generate_miz(
-            persistency.mission_path_for("liberation_nextturn.miz")
+            persistence.mission_path_for("liberation_nextturn.miz")
         )
 
         waiting = QWaitingForMissionResultWindow(self.game, self.sim_controller, self)

qt_ui/windows/QLiberationWindow.py

@@ -1,6 +1,7 @@
 import logging
 import traceback
 import webbrowser
+from pathlib import Path
 from typing import Optional
 
 from PySide6.QtCore import QSettings, Qt, Signal
@@ -16,9 +17,10 @@ from PySide6.QtWidgets import (
 )
 
 import qt_ui.uiconstants as CONST
-from game import Game, VERSION, persistency
+from game import Game, VERSION
 from game.debriefing import Debriefing
 from game.layout import LAYOUTS
+from game.persistence import SaveManager
 from game.server import EventStream, GameContext
 from game.server.dependencies import QtCallbacks, QtContext
 from game.theater import ControlPoint, MissionTarget, TheaterGroundObject
@@ -106,11 +108,11 @@ class QLiberationWindow(QMainWindow):
             if last_save_file:
                 try:
                     logging.info("Loading last saved game : " + str(last_save_file))
-                    game = persistency.load_game(last_save_file)
+                    game = SaveManager.load_player_save(last_save_file)
                     self.onGameGenerated(game)
                     self.updateWindowTitle(last_save_file if game else None)
                 except:
-                    logging.info("Error loading latest save game")
+                    logging.exception("Error loading latest save game")
             else:
                 logging.info("No existing save game")
         else:
@@ -316,58 +318,61 @@ class QLiberationWindow(QMainWindow):
         wizard.accepted.connect(lambda: self.onGameGenerated(wizard.generatedGame))
 
     def openFile(self):
-        if self.game is not None and self.game.savepath:
-            save_dir = self.game.savepath
+        if (
+            self.game is not None
+            and self.game.save_manager.player_save_location is not None
+        ):
+            save_dir = str(self.game.save_manager.player_save_location)
         else:
-            save_dir = str(persistency.save_dir())
+            save_dir = str(SaveManager.default_save_directory())
         file = QFileDialog.getOpenFileName(
             self,
             "Select game file to open",
             dir=save_dir,
-            filter="*.liberation",
+            filter="*.liberation.zip",
         )
         if file is not None and file[0] != "":
-            game = persistency.load_game(file[0])
-            GameUpdateSignal.get_instance().game_loaded.emit(game)
+            try:
+                game = SaveManager.load_player_save(Path(file[0]))
+                GameUpdateSignal.get_instance().game_loaded.emit(game)
 
-            self.updateWindowTitle(file[0])
+                self.updateWindowTitle(Path(file[0]))
+            except Exception:
+                logging.exception("Error loading save game %s", file[0])
 
     def saveGame(self):
         logging.info("Saving game")
 
-        if self.game.savepath:
-            persistency.save_game(self.game)
-            liberation_install.setup_last_save_file(self.game.savepath)
-            liberation_install.save_config()
+        if self.game.save_manager.player_save_location is not None:
+            self.game.save_manager.save_player()
         else:
             self.saveGameAs()
 
     def saveGameAs(self):
-        if self.game is not None and self.game.savepath:
-            save_dir = self.game.savepath
+        if (
+            self.game is not None
+            and self.game.save_manager.player_save_location is not None
+        ):
+            save_dir = str(self.game.save_manager.player_save_location)
         else:
-            save_dir = str(persistency.save_dir())
+            save_dir = str(SaveManager.default_save_directory())
         file = QFileDialog.getSaveFileName(
             self,
             "Save As",
             dir=save_dir,
-            filter="*.liberation",
+            filter="*.liberation.zip",
         )
-        if file is not None:
-            self.game.savepath = file[0]
-            persistency.save_game(self.game)
-            liberation_install.setup_last_save_file(self.game.savepath)
-            liberation_install.save_config()
+        if file is not None and file[0]:
+            self.game.save_manager.save_player(override_destination=Path(file[0]))
+            self.updateWindowTitle(Path(file[0]))
 
-            self.updateWindowTitle(file[0])
-
-    def updateWindowTitle(self, save_path: Optional[str] = None) -> None:
+    def updateWindowTitle(self, save_path: Path | None = None) -> None:
         """
         to DCS Liberation - vX.X.X - file_name
         """
         window_title = f"DCS Liberation - v{VERSION}"
         if save_path:  # appending the file name to title as it is updated
-            file_name = save_path.split("/")[-1].split(".liberation")[0]
+            file_name = save_path.name.split(".liberation.zip")[0]
             window_title = f"{window_title} - {file_name}"
         self.setWindowTitle(window_title)
 

qt_ui/windows/QWaitingForMissionResultWindow.py

@@ -1,7 +1,6 @@
 from __future__ import annotations
 
 import logging
-import os
 from pathlib import Path
 from typing import Optional
 
@@ -23,7 +22,6 @@ from jinja2 import Environment, FileSystemLoader, select_autoescape
 
 from game import Game
 from game.debriefing import Debriefing
-from game.persistency import base_path
 from game.profiling import logged_duration
 from qt_ui.simcontroller import SimController
 from qt_ui.windows.GameUpdateSignal import GameUpdateSignal
@@ -221,9 +219,6 @@ class QWaitingForMissionResultWindow(QDialog):
             GameUpdateSignal.get_instance().updateGame(self.game)
         self.close()
 
-    def debriefing_directory_location(self) -> str:
-        return os.path.join(base_path(), "liberation_debriefings")
-
     def closeEvent(self, evt):
         super(QWaitingForMissionResultWindow, self).closeEvent(evt)
         if self.wait_thread is not None:

requirements.txt

@@ -7,6 +7,7 @@ certifi==2022.12.7
 cfgv==3.3.1
 click==8.1.3
 colorama==0.4.6
+coverage==7.0.5
 distlib==0.3.6
 exceptiongroup==1.1.0
 Faker==15.3.4
@@ -39,6 +40,8 @@ PySide6==6.4.1
 PySide6-Addons==6.4.1
 PySide6-Essentials==6.4.1
 pytest==7.2.0
+pytest-cov==4.0.0
+pytest-mock==3.10.0
 python-dateutil==2.8.2
 python-dotenv==0.21.0
 pywin32-ctypes==0.2.0

resources/tools/loadoutviewer.py

@@ -9,7 +9,7 @@ from dcs.helicopters import helicopter_map
 from dcs.planes import plane_map
 from dcs.unittype import FlyingType
 
-from game import persistency
+from game import persistence
 from game.ato import FlightType
 from game.ato.loadouts import Loadout
 from game.dcs.aircrafttype import AircraftType
@@ -122,7 +122,7 @@ def main() -> None:
             "the first run configuration."
         )
 
-    inject_custom_payloads(Path(persistency.base_path()))
+    inject_custom_payloads(Path(persistence.base_path()))
 
     if args.aircraft_id is None:
         show_all_aircraft(args.task)

tests/conftest.py

@@ -0,0 +1,12 @@
+from pathlib import Path
+from zipfile import ZipFile
+
+import pytest
+
+
+@pytest.fixture
+def tmp_zip(tmp_path: Path) -> Path:
+    zip_path = tmp_path / "test.zip"
+    with ZipFile(zip_path, "w"):
+        pass
+    return zip_path

tests/persistence/conftest.py

@@ -0,0 +1,16 @@
+import datetime
+from typing import cast
+
+import pytest
+
+from game import Game
+
+
+class StubGame:
+    def __init__(self) -> None:
+        self.date = datetime.date.min
+
+
+@pytest.fixture
+def game() -> Game:
+    return cast(Game, StubGame())

tests/persistence/test_savegamebundle.py

@@ -0,0 +1,104 @@
+import datetime
+from pathlib import Path
+from zipfile import ZipFile
+
+import pytest
+
+from game import Game
+from game.persistence.savegamebundle import SaveGameBundle
+
+
+@pytest.fixture
+def tmp_bundle(tmp_zip: Path) -> SaveGameBundle:
+    return SaveGameBundle(tmp_zip)
+
+
+def test_save_player_new_save(game: Game, tmp_bundle: SaveGameBundle) -> None:
+    with ZipFile(tmp_bundle.bundle_path, "r") as zip_file:
+        with pytest.raises(KeyError):
+            zip_file.read(SaveGameBundle.MANUAL_SAVE_NAME)
+    tmp_bundle.save_player(game, copy_from=None)
+
+    with ZipFile(tmp_bundle.bundle_path, "r") as zip_file:
+        assert zip_file.namelist() == [SaveGameBundle.MANUAL_SAVE_NAME]
+
+
+def test_save_player_existing_save(game: Game, tmp_bundle: SaveGameBundle) -> None:
+    game.date = datetime.date.min
+    tmp_bundle.save_start_of_turn(game)
+    tmp_bundle.save_player(game, copy_from=tmp_bundle)
+
+    test_date = datetime.date.today()
+    game.date = test_date
+    tmp_bundle.save_player(game, copy_from=tmp_bundle)
+
+    assert tmp_bundle.load_start_of_turn().date == datetime.date.min
+    assert tmp_bundle.load_player().date == test_date
+
+
+def test_save_last_turn(game: Game, tmp_bundle: SaveGameBundle) -> None:
+    with ZipFile(tmp_bundle.bundle_path, "r") as zip_file:
+        with pytest.raises(KeyError):
+            zip_file.read(SaveGameBundle.LAST_TURN_SAVE_NAME)
+    tmp_bundle.save_last_turn(game)
+
+    with ZipFile(tmp_bundle.bundle_path, "r") as zip_file:
+        assert zip_file.namelist() == [SaveGameBundle.LAST_TURN_SAVE_NAME]
+
+
+def test_save_start_of_turn(game: Game, tmp_bundle: SaveGameBundle) -> None:
+    with ZipFile(tmp_bundle.bundle_path, "r") as zip_file:
+        with pytest.raises(KeyError):
+            zip_file.read(SaveGameBundle.START_OF_TURN_SAVE_NAME)
+    tmp_bundle.save_start_of_turn(game)
+
+    with ZipFile(tmp_bundle.bundle_path, "r") as zip_file:
+        assert zip_file.namelist() == [SaveGameBundle.START_OF_TURN_SAVE_NAME]
+
+
+def test_failed_save_leaves_original_intact(
+    game: Game, tmp_bundle: SaveGameBundle
+) -> None:
+    expect_date = datetime.date.today()
+    game.date = expect_date
+    tmp_bundle.save_player(game, copy_from=None)
+
+    # Add some non-pickleable member to the game to cause an error on save.
+    def local_f() -> None:
+        pass
+
+    game.garbage = local_f  # type: ignore
+    with pytest.raises(AttributeError):
+        tmp_bundle.save_player(game, copy_from=tmp_bundle)
+
+    assert tmp_bundle.load_player().date == expect_date
+
+
+def test_load_reads_correct_data(game: Game, tmp_bundle: SaveGameBundle) -> None:
+    last_turn_date = datetime.date.today() - datetime.timedelta(days=2)
+    game.date = last_turn_date
+    tmp_bundle.save_last_turn(game)
+
+    start_of_turn_date = datetime.date.today() - datetime.timedelta(days=1)
+    game.date = start_of_turn_date
+    tmp_bundle.save_start_of_turn(game)
+
+    player_date = datetime.date.today()
+    game.date = player_date
+    tmp_bundle.save_player(game, copy_from=tmp_bundle)
+
+    assert tmp_bundle.load_last_turn().date == last_turn_date
+    assert tmp_bundle.load_start_of_turn().date == start_of_turn_date
+    assert tmp_bundle.load_player().date == player_date
+
+
+def test_load_from_absent_file_raises(tmp_bundle: SaveGameBundle) -> None:
+    tmp_bundle.bundle_path.unlink(missing_ok=True)
+    with pytest.raises(FileNotFoundError):
+        tmp_bundle.load_last_turn()
+
+
+def test_load_from_absent_member_raises(game: Game, tmp_bundle: SaveGameBundle) -> None:
+    tmp_bundle.save_start_of_turn(game)
+    with pytest.raises(KeyError):
+        tmp_bundle.load_last_turn()

tests/persistence/test_savemanager.py

@@ -0,0 +1,152 @@
+import datetime
+from pathlib import Path
+from unittest.mock import Mock
+
+import pytest
+from pytest_mock import MockerFixture
+
+from game import Game
+from game.persistence import SaveManager, set_dcs_save_game_directory
+
+
+@pytest.fixture(autouse=True)
+def mock_setup_last_save_file(mocker: MockerFixture) -> Mock:
+    return mocker.patch("qt_ui.liberation_install.setup_last_save_file")
+
+
+@pytest.fixture(autouse=True)
+def mock_save_config(mocker: MockerFixture) -> Mock:
+    return mocker.patch("qt_ui.liberation_install.save_config")
+
+
+@pytest.fixture(autouse=True)
+def setup_persistence_paths(tmp_path: Path) -> None:
+    set_dcs_save_game_directory(tmp_path)
+
+
+@pytest.fixture
+def save_manager(game: Game) -> SaveManager:
+    return SaveManager(game)
+
+
+def test_new_savemanager_saves_to_autosave(save_manager: SaveManager) -> None:
+    assert save_manager.default_save_location == save_manager.autosave_path
+
+
+def test_savemanager_saves_to_last_save_location(save_manager: SaveManager) -> None:
+    save_manager.player_save_location = Path("Saves/foo.liberation.zip")
+    assert save_manager.default_save_location == save_manager.player_save_location
+
+
+def test_saving_without_name_saves_to_autosave_path(save_manager: SaveManager) -> None:
+    assert not save_manager.autosave_path.exists()
+    save_manager.save_player()
+    assert save_manager.autosave_path.exists()
+
+
+def test_saving_with_name_updates_last_save_location(
+    save_manager: SaveManager, tmp_path: Path
+) -> None:
+    save_path = tmp_path / "foo.liberation.zip"
+    assert not save_path.exists()
+    save_manager.save_player(override_destination=save_path)
+    assert save_path.exists()
+    assert save_manager.last_saved_bundle is not None
+    assert save_manager.last_saved_bundle.bundle_path == save_path
+
+
+def test_player_save_location(save_manager: SaveManager, tmp_path: Path) -> None:
+    assert save_manager.player_save_location is None
+    save_manager.save_last_turn()
+    assert save_manager.player_save_location is None
+    save_manager.save_start_of_turn()
+    assert save_manager.player_save_location is None
+    expect_location = tmp_path / "player.liberation.zip"
+    save_manager.save_player(override_destination=expect_location)
+    assert save_manager.player_save_location == expect_location
+
+
+def test_saving_updates_preferences_with_save_location(
+    save_manager: SaveManager, mock_setup_last_save_file: Mock, mock_save_config: Mock
+) -> None:
+    save_manager.save_player()
+    mock_setup_last_save_file.assert_called_once_with(
+        str(save_manager.default_save_location)
+    )
+    mock_save_config.assert_called_once()
+
+
+def test_non_player_saves_do_not_update_preferences(
+    save_manager: SaveManager, mock_setup_last_save_file: Mock
+) -> None:
+    save_manager.save_last_turn()
+    mock_setup_last_save_file.assert_not_called()
+    save_manager.save_start_of_turn()
+    mock_setup_last_save_file.assert_not_called()
+
+
+def test_load_game_loads_correct_data(save_manager: SaveManager) -> None:
+    test_date = datetime.date.today()
+    assert save_manager.game.date != test_date
+    save_manager.game.date = test_date
+    save_manager.save_player()
+    game = SaveManager.load_player_save(save_manager.default_save_location)
+    assert game.date == test_date
+
+
+def test_loading_missing_save_raises() -> None:
+    with pytest.raises(FileNotFoundError):
+        SaveManager.load_player_save(Path("does not exist"))
+
+
+def test_saving_after_autosave_copies_autosave_members(
+    save_manager: SaveManager, tmp_path: Path
+) -> None:
+    save_manager.save_start_of_turn()
+
+    save_path = tmp_path / "foo.liberation.zip"
+    save_manager.save_player(override_destination=save_path)
+
+    SaveManager.load_start_of_turn(save_path)
+
+
+def test_failed_save_does_not_update_last_saved_path(
+    save_manager: SaveManager, tmp_path: Path
+) -> None:
+    expect_date = datetime.date.today()
+    save_manager.game.date = expect_date
+    save_manager.save_player()
+    assert save_manager.last_saved_bundle is not None
+    expect_path = save_manager.last_saved_bundle.bundle_path
+
+    # Add some non-pickleable member to the game to cause an error on save.
+    def local_f() -> None:
+        pass
+
+    save_manager.game.garbage = local_f  # type: ignore
+    with pytest.raises(AttributeError):
+        save_manager.save_player(
+            override_destination=tmp_path / "badsave.liberation.zip"
+        )
+
+    assert save_manager.last_saved_bundle.bundle_path == expect_path
+
+
+def test_load_reads_correct_data(save_manager: SaveManager) -> None:
+    last_turn_date = datetime.date.today() - datetime.timedelta(days=2)
+    save_manager.game.date = last_turn_date
+    save_manager.save_last_turn()
+
+    start_of_turn_date = datetime.date.today() - datetime.timedelta(days=1)
+    save_manager.game.date = start_of_turn_date
+    save_manager.save_start_of_turn()
+
+    player_date = datetime.date.today()
+    save_manager.game.date = player_date
+    save_manager.save_player()
+
+    assert save_manager.last_saved_bundle is not None
+    bundle_path = save_manager.last_saved_bundle.bundle_path
+    assert SaveManager.load_last_turn(bundle_path).date == last_turn_date
+    assert SaveManager.load_start_of_turn(bundle_path).date == start_of_turn_date
+    assert SaveManager.load_player_save(bundle_path).date == player_date

tests/test_zipfileext.py

@@ -0,0 +1,33 @@
+from pathlib import Path
+from zipfile import ZipFile
+
+import pytest
+
+from game.zipfileext import ZipFileExt
+
+
+def test_remove_member_does_nothing_if_member_is_not_present(tmp_zip: Path) -> None:
+    expect_mtime = tmp_zip.stat().st_mtime
+    ZipFileExt.remove_member(tmp_zip, "c", missing_ok=True)
+    assert tmp_zip.stat().st_mtime == expect_mtime
+
+
+def test_remove_member_raises_if_missing_not_ok(tmp_zip: Path) -> None:
+    with pytest.raises(ValueError):
+        ZipFileExt.remove_member(tmp_zip, "c")
+
+
+def test_remove_member(tmp_zip: Path) -> None:
+    with ZipFile(tmp_zip, "w") as zip_file:
+        zip_file.writestr("a", "foo")
+        zip_file.writestr("b", "bar")
+
+    ZipFileExt.remove_member(tmp_zip, "a")
+
+    with ZipFile(tmp_zip, "r") as zip_file:
+        with pytest.raises(KeyError):
+            zip_file.read("a")
+        # Yes, we wrote a str, but ZipFile.read always returns bytes, and ZipFile.write
+        # requires an intermediate file. It's hard to write bytes, and hard to read str.
+        # This is all the single-byte range of UTF-8 anyway, so it doesn't matter.
+        assert zip_file.read("b") == b"bar"