fix review comments

README.md

@@ -29,15 +29,10 @@ In addition to that, Minigalaxy also allows you to:
 Minigalaxy version 1.3.2 and higher change some aspects of windows game installations through wine.
 It will try to adapt already installed games to the new concept when launched through Minigalaxy.
 
-However, this will *likely break games* that save some directory paths in the *windows registry*.
-In that case, only a reinstallation will repair the game. 
-
-**Please make sure to backup any save games you might have within the game folder**
-
-The windows installer in wine uses a 2-step attempt to install games. 
+The windows installer in wine now uses a 2-step attempt to install games. 
 1. An unattended installer.
 2. In case this fails, the regular installation wizard will open. **Please do not change** the  
-install directory 'c:\game' given in the wizard as this an elementary part of the wine fix!
+install directory 'c:\game' given in the wizard as this an elementary part of the wine fix.
 
 ## Supported languages
 

minigalaxy/installer.py

@@ -11,7 +11,7 @@
 from minigalaxy.game import Game
 from minigalaxy.logger import logger
 from minigalaxy.translation import _
-from minigalaxy.launcher import get_execute_command
+from minigalaxy.launcher import get_execute_command, get_wine_path, wine_restore_game_link
 from minigalaxy.paths import CACHE_DIR, THUMBNAIL_DIR, APPLICATIONS_DIR
 
 
@@ -58,17 +58,16 @@ def install_game(  # noqa: C901
     tmp_dir = ""
     logger.info("Installing {}".format(game.name))
     try:
-        _use_innoextract = use_innoextract and bool(shutil.which('innoextract'))  # single decision point
         if not error_message:
             error_message = verify_installer_integrity(game, installer)
         if not error_message:
             error_message = verify_disk_space(game, installer)
         if not error_message:
             error_message, tmp_dir = make_tmp_dir(game)
         if not error_message:
-            error_message = extract_installer(game, installer, tmp_dir, language, _use_innoextract)
+            error_message, installed_to_tmp = extract_installer(game, installer, tmp_dir, language)
         if not error_message:
-            error_message = move_and_overwrite(game, tmp_dir, _use_innoextract)
+            error_message = move_and_overwrite(game, tmp_dir, installed_to_tmp)
         if not error_message:
             error_message = copy_thumbnail(game)
         if not error_message and create_desktop_file:
@@ -126,13 +125,12 @@ def make_tmp_dir(game):
     return error_message, temp_dir
 
 
-def extract_installer(game: Game, installer: str, temp_dir: str, language: str, use_innoextract: bool):
+def extract_installer(game: Game, installer: str, temp_dir: str, language: str):
     # Extract the installer
     if game.platform in ["linux"]:
-        err_msg = extract_linux(installer, temp_dir)
+        return extract_linux(installer, temp_dir)
     else:
-        err_msg = extract_windows(game, installer, temp_dir, language, use_innoextract)
-    return err_msg
+        return extract_windows(game, installer, language)
 
 
 def extract_linux(installer, temp_dir):
@@ -144,14 +142,19 @@ def extract_linux(installer, temp_dir):
         err_msg = _("The installation of {} failed. Please try again.").format(installer)
     elif len(os.listdir(temp_dir)) == 0:
         err_msg = _("{} could not be unzipped.".format(installer))
-    return err_msg
+    return err_msg, True
 
 
-def extract_windows(game: Game, installer: str, temp_dir: str, language: str, use_innoextract: bool):
-    err_msg = extract_by_innoextract(installer, temp_dir, language, use_innoextract)
-    if err_msg:
-        err_msg = extract_by_wine(game, installer, temp_dir)
-    return err_msg
+def extract_windows(game: Game, installer: str, language: str):
+    if shutil.which("innoextract"):
+        game_lang = lang_install(installer, language)
+        game_lang = game_lang.split('=')[1]  # lang_install returns '--language=localeCode'
+    else:
+        game_lang = 'en-US'
+
+    logger.info(f'use {game_lang} for installer')
+
+    return extract_by_wine(game, installer, game_lang), False
 
 
 def extract_by_innoextract(installer: str, temp_dir: str, language: str, use_innoextract: bool):
@@ -182,15 +185,14 @@ def extract_by_innoextract(installer: str, temp_dir: str, language: str, use_inn
     return err_msg
 
 
-def extract_by_wine(game, installer, temp_dir, config=Config()):
+def extract_by_wine(game, installer, game_lang, config=Config()):
     # Set the prefix for Windows games
     prefix_dir = os.path.join(game.install_dir, "prefix")
-    game_dir = os.path.join(prefix_dir, "dosdevices", 'c:', 'game')
     wine_env = [
         f"WINEPREFIX={prefix_dir}",
         "WINEDLLOVERRIDES=winemenubuilder.exe=d"
     ]
-    wine_bin = shutil.which('wine')
+    wine_bin = get_wine_path(game)
 
     if not os.path.exists(prefix_dir):
         os.makedirs(prefix_dir, mode=0o755)
@@ -199,13 +201,9 @@ def extract_by_wine(game, installer, temp_dir, config=Config()):
         if not try_wine_command(command):
             return _("Wineprefix creation failed.")
 
-    # calculate relative link from prefix-internal folder to game.install_dir
+    # calculate relative link prefix/c/game to game.install_dir
     # keeping it relative makes sure that the game can be moved around without stuff breaking
-    if not os.path.exists(game_dir):
-        # 'game' directory itself does not count
-        canonical_prefix = os.path.realpath(os.path.join(game_dir, '..'))
-        relative = os.path.relpath(game.install_dir, canonical_prefix)
-        os.symlink(relative, game_dir)
+    wine_restore_game_link(game)
     # It's possible to set install dir as argument before installation
     installer_cmd_basic = [
         'env', *wine_env, wine_bin, installer,
@@ -219,13 +217,15 @@ def extract_by_wine(game, installer, temp_dir, config=Config()):
         f"/LANG={config.lang}",
         "/SAVEINF=c:\\setup.inf",
         # installers can run very long, give at least a bit of visual feedback
-        '/SILENT'
+        # by using /SILENT instead of /VERYSILENT
+        '/SP-', '/SILENT', '/NORESTART', '/SUPPRESSMSGBOXES'
     ]
 
-    # first, try full unattended install
+    # first, try full unattended install.
     success = try_wine_command(installer_cmd_basic + installer_args_full)
     if not success:
-        # some games will reject the /SILENT flag.
+        # some games will reject the /SILENT flag
+        # because they require the user to accept EULA at the beginning
         # Open normal installer as fallback and hope for the best
         print('Unattended install failed. Try install with wizard dialog.', file=sys.stderr)
         success = try_wine_command(installer_cmd_basic)
@@ -239,22 +239,24 @@ def extract_by_wine(game, installer, temp_dir, config=Config()):
 def try_wine_command(command_arr):
     print('trying to run wine command:', shlex.join(command_arr))
     stdout, stderr, exitcode = _exe_cmd(command_arr, True)
-    print(stdout)
     if exitcode not in [0]:
         print(stderr, file=sys.stderr)
         return False
 
     return True
 
 
-def move_and_overwrite(game, temp_dir, use_innoextract):
+def move_and_overwrite(game, temp_dir, installed_to_tmp):
     # Copy the game files into the correct directory
     error_message = ""
     source_dir = (os.path.join(temp_dir, "data", "noarch") if game.platform == 'linux' else
-                  temp_dir if use_innoextract else
-                  os.path.join(temp_dir, os.path.basename(game.install_dir)))
+                  temp_dir)
     target_dir = game.install_dir
-    _mv(source_dir, target_dir)
+
+    if installed_to_tmp:
+        _mv(source_dir, target_dir)
+    else:
+        logger.info(f'installation of {game.name} did not use temporary directory - nothing to move')
 
     # Remove the temporary directory
     shutil.rmtree(temp_dir, ignore_errors=True)
@@ -296,7 +298,7 @@ def create_applications_file(game):
         Path={game_install_dir}
         Name={game_name}
         Icon={game_icon_path}
-        Category=Game""".format(**desktop_context)
+        Categories=Game""".format(**desktop_context)
     if not os.path.isfile(path_to_shortcut):
         try:
             with open(path_to_shortcut, 'w+') as desktop_file:

minigalaxy/launcher.py

@@ -19,6 +19,17 @@ def get_wine_path(game):
     return binary_name
 
 
+# should go into a separate file or into installer, but not possible ATM because
+# it's a circular import otherwise
+def wine_restore_game_link(game):
+    game_dir = os.path.join(game.install_dir, 'prefix', 'dosdevices', 'c:', 'game')
+    if not os.path.exists(game_dir):
+        # 'game' directory itself does not count
+        canonical_prefix = os.path.realpath(os.path.join(game_dir, '..'))
+        relative = os.path.relpath(game.install_dir, canonical_prefix)
+        os.symlink(relative, game_dir)
+
+
 def config_game(game):
     prefix = os.path.join(game.install_dir, "prefix")
     subprocess.Popen(['env', f'WINEPREFIX={prefix}', get_wine_path(game), 'winecfg'])
@@ -142,10 +153,7 @@ def get_windows_exe_cmd(game, files):
     # Backwards compatibility with windows games installed before installer fixes.
     # Will not fix games requiring registry keys, since the paths will already
     # be borked through the old installer.
-    gamelink = os.path.join(prefix, 'dosdevices', 'c:', 'game')
-    if not os.path.exists(gamelink):
-        os.makedirs(os.path.join(prefix, 'dosdevices', 'c:'))
-        os.symlink('../..', gamelink)
+    wine_restore_game_link(game)
 
     return ['env', f'WINEPREFIX={prefix}'] + exe_cmd
 

tests/test_installer.py

@@ -8,10 +8,10 @@
 
 
 class Test(TestCase):
-    @mock.patch('shutil.which')
-    def test_install_game(self, mock_which):
+    @mock.patch('os.path.exists')
+    def test_install_game(self, mock_exists):
         """[scenario: unhandled error]"""
-        mock_which.side_effect = FileNotFoundError("Testing unhandled errors during install")
+        mock_exists.side_effect = FileNotFoundError("Testing unhandled errors during install")
         game = Game("Absolute Drift", install_dir="/home/makson/GOG Games/Absolute Drift", platform="windows")
         exp = "Unhandled error."
         obs = installer.install_game(game, installer="", language="", install_dir="", keep_installers=False, create_desktop_file=True)
@@ -67,7 +67,7 @@ def test1_extract_installer(self, mock_subprocess, mock_listdir, mock_is_file):
         installer_path = "/home/makson/.cache/minigalaxy/download/Beneath a Steel Sky/beneath_a_steel_sky_en_gog_2_20150.sh"
         temp_dir = "/home/makson/.cache/minigalaxy/extract/1207658695"
         exp = ""
-        obs = installer.extract_installer(game, installer_path, temp_dir, "en", use_innoextract=False)
+        obs, use_temp = installer.extract_installer(game, installer_path, temp_dir, "en")
         self.assertEqual(exp, obs)
 
     @mock.patch('os.path.exists')
@@ -83,21 +83,28 @@ def test2_extract_installer(self, mock_subprocess, mock_listdir, mock_is_file):
         installer_path = "/home/makson/.cache/minigalaxy/download/Beneath a Steel Sky/beneath_a_steel_sky_en_gog_2_20150.sh"
         temp_dir = "/home/makson/.cache/minigalaxy/extract/1207658695"
         exp = "The installation of /home/makson/.cache/minigalaxy/download/Beneath a Steel Sky/beneath_a_steel_sky_en_gog_2_20150.sh failed. Please try again."
-        obs = installer.extract_installer(game, installer_path, temp_dir, "en", use_innoextract=False)
+        obs, use_temp = installer.extract_installer(game, installer_path, temp_dir, "en")
         self.assertEqual(exp, obs)
 
+    # TODO: Delete - innoextract not used for installation anymore
+    # test is only made not to fail, but it is pointless, as wine is called
+    # internally anyway
+    @mock.patch('minigalaxy.installer.try_wine_command')
+    @mock.patch('os.path.exists')
     @mock.patch('subprocess.Popen')
     @mock.patch('shutil.which')
-    def test3_extract_installer(self, mock_which, mock_subprocess):
+    def test3_extract_installer(self, mock_which, mock_subprocess, mock_exists, mock_cmd):
         """[scenario: innoextract, unpack success]"""
         mock_which.return_value = True
         mock_subprocess().poll.return_value = 0
-        mock_subprocess().stdout.readlines.return_value = ["stdout", "stderr"]
+        mock_subprocess().stdout.readline.return_value = " - en-US"
+        mock_exists.return_value = True
+        mock_cmd.side_effect = [True, True]
         game = Game("Absolute Drift", install_dir="/home/makson/GOG Games/Absolute Drift", platform="windows")
         installer_path = "/home/makson/.cache/minigalaxy/download/Absolute Drift/setup_absolute_drift_1.0f_(64bit)_(47863).exe"
         temp_dir = "/home/makson/.cache/minigalaxy/extract/1136126792"
         exp = ""
-        obs = installer.extract_installer(game, installer_path, temp_dir, "en", use_innoextract=True)
+        obs, use_temp = installer.extract_installer(game, installer_path, temp_dir, "en")
         self.assertEqual(exp, obs)
 
     @mock.patch('os.path.exists')
@@ -111,19 +118,23 @@ def test_extract_linux(self, mock_subprocess, mock_listdir, mock_is_file):
         installer_path = "/home/makson/.cache/minigalaxy/download/Beneath a Steel Sky/beneath_a_steel_sky_en_gog_2_20150.sh"
         temp_dir = "/home/makson/.cache/minigalaxy/extract/1207658695"
         exp = ""
-        obs = installer.extract_linux(installer_path, temp_dir)
+        obs, temp_used = installer.extract_linux(installer_path, temp_dir)
         self.assertEqual(exp, obs)
 
+    @mock.patch('minigalaxy.installer.try_wine_command')
+    @mock.patch('os.path.exists')
     @mock.patch('subprocess.Popen')
-    def test_extract_windows(self, mock_subprocess):
+    def test_extract_windows(self, mock_subprocess, mock_exists, mock_cmd):
         """[scenario: innoextract, unpack success]"""
         mock_subprocess().poll.return_value = 0
         mock_subprocess().stdout.readlines.return_value = ["stdout", "stderr"]
+        mock_exists.return_value = True
+        mock_cmd.side_effect = [True, True]
         game = Game("Absolute Drift", install_dir="/home/makson/GOG Games/Absolute Drift", platform="windows")
         installer_path = "/home/makson/.cache/minigalaxy/download/Absolute Drift/setup_absolute_drift_1.0f_(64bit)_(47863).exe"
         temp_dir = "/home/makson/.cache/minigalaxy/extract/1136126792"
         exp = ""
-        obs = installer.extract_windows(game, installer_path, temp_dir, "en", use_innoextract=True)
+        obs, uses_tmp = installer.extract_windows(game, installer_path, "en")
         self.assertEqual(exp, obs)
 
     @mock.patch('subprocess.Popen')