From fee7521ee426e9db1c00e96942062cc87571b186 Mon Sep 17 00:00:00 2001 From: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com> Date: Wed, 31 Jul 2024 13:51:25 +1000 Subject: [PATCH 1/8] Updated to use `executeFlags` and `executeOptions` structures, rather than strings directly, so we can set flags or options multiple times without worrying about duplicates --- source/updateCheck.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/source/updateCheck.py b/source/updateCheck.py index c9f44a778da..623c28c1b4c 100644 --- a/source/updateCheck.py +++ b/source/updateCheck.py @@ -243,19 +243,21 @@ def _executeUpdate(destPath): _setStateToNone(state) saveState() + executeFlags: set[str] = set() + executeOptions: dict[str, str] = {} if config.isInstalledCopy(): - executeParams = "--install -m" + executeFlags.update("--install", "-m") else: portablePath = globalVars.appDir if os.access(portablePath, os.W_OK): - executeParams = ( - '--create-portable --portable-path "{portablePath}" --config-path "{configPath}" -m'.format( - portablePath=portablePath, - configPath=WritePaths.configDir, - ) + executeFlags.update("--create-portable", "-m") + executeOptions.update( + ("--portable-path", f'"{portablePath}"'), + ("--config-path", '"{WritePaths.configDir}"'), ) else: - executeParams = "--launcher" + executeFlags.add("--launcher") + executeParams = f"{' '.join(executeOptions)} {' '.join(' '.join(i) for i in executeOptions.items())}" # #4475: ensure that the new process shows its first window, by providing SW_SHOWNORMAL if not core.triggerNVDAExit(core.NewNVDAInstance(destPath, executeParams)): log.error("NVDA already in process of exiting, this indicates a logic error.") From 313c30f50d92fc6575d9927dabd4ce31102d4295 Mon Sep 17 00:00:00 2001 From: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com> Date: Thu, 1 Aug 2024 11:22:34 +1000 Subject: [PATCH 2/8] Fixed up `_executeUpdate` - Refactored the command line argument generation into a helper function. - Fixed a typo where command line options were included twice and flags weren't included. --- source/updateCheck.py | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/source/updateCheck.py b/source/updateCheck.py index 623c28c1b4c..ba52eeb39af 100644 --- a/source/updateCheck.py +++ b/source/updateCheck.py @@ -243,25 +243,29 @@ def _executeUpdate(destPath): _setStateToNone(state) saveState() - executeFlags: set[str] = set() - executeOptions: dict[str, str] = {} - if config.isInstalledCopy(): - executeFlags.update("--install", "-m") - else: - portablePath = globalVars.appDir - if os.access(portablePath, os.W_OK): - executeFlags.update("--create-portable", "-m") - executeOptions.update( - ("--portable-path", f'"{portablePath}"'), - ("--config-path", '"{WritePaths.configDir}"'), - ) - else: - executeFlags.add("--launcher") - executeParams = f"{' '.join(executeOptions)} {' '.join(' '.join(i) for i in executeOptions.items())}" + executeParams = _generate_executionParameters() + log.info(f"Execution parameters: {executeParams}") # #4475: ensure that the new process shows its first window, by providing SW_SHOWNORMAL if not core.triggerNVDAExit(core.NewNVDAInstance(destPath, executeParams)): log.error("NVDA already in process of exiting, this indicates a logic error.") +def _generate_executionParameters(): + executeFlags: set[str] = set() + executeOptions: dict[str, str] = {} + if config.isInstalledCopy(): + executeFlags.update(("--install", "-m")) + else: + portablePath = globalVars.appDir + if os.access(portablePath, os.W_OK): + executeFlags.update(("--create-portable", "-m")) + executeOptions["--portable-path"] = f'"{portablePath}"' + else: + executeFlags.add("--launcher") + if globalVars.appArgs.disableAddons: + executeFlags.add("--disable-addons") + executeOptions["--config-path"] = f'"{WritePaths.configDir}"' + return f"{' '.join(executeFlags)} {' '.join(' '.join(i) for i in executeOptions.items())}" + class UpdateChecker(garbageHandler.TrackedObject): """Check for an updated version of NVDA, presenting appropriate user interface. From 55785fa83d4c6c8e68d31741a89daa34ccdc7c2f Mon Sep 17 00:00:00 2001 From: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com> Date: Thu, 1 Aug 2024 11:52:34 +1000 Subject: [PATCH 3/8] Tidied - Added type hints - Added docstrings and comments - Renamed some variables - Raise `valueError` if `destPath` isn't passed to `_executeUpdate` - Linted --- source/updateCheck.py | 58 +++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/source/updateCheck.py b/source/updateCheck.py index ba52eeb39af..a1f28061533 100644 --- a/source/updateCheck.py +++ b/source/updateCheck.py @@ -237,34 +237,50 @@ def executePendingUpdate(): _executeUpdate(updateTuple[0]) -def _executeUpdate(destPath): +def _executeUpdate(destPath: str) -> None: + """Execute the update process. + + :param destPath: The path to the update executable. + :raises ValueError: If destPath is empty or not given. + """ if not destPath: - return + raise ValueError("destPath must be a non-empty string") _setStateToNone(state) saveState() - executeParams = _generate_executionParameters() - log.info(f"Execution parameters: {executeParams}") # #4475: ensure that the new process shows its first window, by providing SW_SHOWNORMAL - if not core.triggerNVDAExit(core.NewNVDAInstance(destPath, executeParams)): + if not core.triggerNVDAExit(core.NewNVDAInstance(destPath, _generate_updateParameters())): log.error("NVDA already in process of exiting, this indicates a logic error.") -def _generate_executionParameters(): - executeFlags: set[str] = set() - executeOptions: dict[str, str] = {} - if config.isInstalledCopy(): - executeFlags.update(("--install", "-m")) - else: - portablePath = globalVars.appDir - if os.access(portablePath, os.W_OK): - executeFlags.update(("--create-portable", "-m")) - executeOptions["--portable-path"] = f'"{portablePath}"' - else: - executeFlags.add("--launcher") - if globalVars.appArgs.disableAddons: - executeFlags.add("--disable-addons") - executeOptions["--config-path"] = f'"{WritePaths.configDir}"' - return f"{' '.join(executeFlags)} {' '.join(' '.join(i) for i in executeOptions.items())}" + +def _generate_updateParameters() -> str: + """Generate parameters to pass to the new NVDA instance for the update process. + + We generate parameters that specify: + - Whether to install, update a portable copy, or run the launcher. + - Whether to disable addons. + - The path to the configuration directory. + + :return: The parameters to pass to the new NVDA instance. + """ + executeFlags: set[str] = set() + executeOptions: dict[str, str] = {} + if config.isInstalledCopy(): + executeFlags.update(("--install", "-m")) + else: + portablePath = globalVars.appDir + if os.access(portablePath, os.W_OK): + executeFlags.update(("--create-portable", "-m")) + executeOptions["--portable-path"] = f'"{portablePath}"' + else: + # We can't write to the currently running portable copy's directory, so just run the launcher. + executeFlags.add("--launcher") + if globalVars.appArgs.disableAddons: + executeFlags.add("--disable-addons") + # pass the config path to the new instance, so that if a custom config path is in use, it will be inherited. + # If the default con fig path is in use, the new instance would use it anyway, so there is no harm in passing it. + executeOptions["--config-path"] = f'"{WritePaths.configDir}"' + return f"{' '.join(executeFlags)} {' '.join(' '.join(i) for i in executeOptions.items())}" class UpdateChecker(garbageHandler.TrackedObject): From ce595fb83f986a4176105464aeab8fafbb14ab35 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 1 Aug 2024 02:51:36 +0000 Subject: [PATCH 4/8] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- source/speech/commands.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/speech/commands.py b/source/speech/commands.py index 68a8c0f8769..b6eaf941caa 100644 --- a/source/speech/commands.py +++ b/source/speech/commands.py @@ -80,7 +80,7 @@ def _getFormattedDevInfo(self): def __repr__(self): return ( f"CancellableSpeech (" - f"{ 'cancelled' if self._checkIfCancelled() else 'still valid' }" + f"{ 'cancelled' if self._checkIfCancelled() else 'still valid'}" f"{self._getFormattedDevInfo()}" f")" ) From 7a272d4033225bc873b5482b1f586711df522725 Mon Sep 17 00:00:00 2001 From: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com> Date: Thu, 1 Aug 2024 12:55:56 +1000 Subject: [PATCH 5/8] Added changelog entry --- user_docs/en/changes.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/user_docs/en/changes.md b/user_docs/en/changes.md index 9efc2245235..3eb1a419a09 100644 --- a/user_docs/en/changes.md +++ b/user_docs/en/changes.md @@ -17,6 +17,8 @@ The available options are: ### Changes +* The `-c`/`--config-path` and `--disable-addons` command line options are now respected when launching an update from within NVDA. (#16937) + ### Bug Fixes * NVDA once again relies on events for caret movement in several cases, rather than only on manual querying of the caret position. From 12a10672cf29972ba24a3a56117788d5d594b058 Mon Sep 17 00:00:00 2001 From: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com> Date: Thu, 1 Aug 2024 16:23:34 +1000 Subject: [PATCH 6/8] Log error and return rather than raising. Remove unneeded comment. --- source/updateCheck.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/source/updateCheck.py b/source/updateCheck.py index a1f28061533..762023b5bb5 100644 --- a/source/updateCheck.py +++ b/source/updateCheck.py @@ -241,14 +241,13 @@ def _executeUpdate(destPath: str) -> None: """Execute the update process. :param destPath: The path to the update executable. - :raises ValueError: If destPath is empty or not given. """ if not destPath: - raise ValueError("destPath must be a non-empty string") + log.error("destPath must be a non-empty string.", exc_info=True) + return _setStateToNone(state) saveState() - # #4475: ensure that the new process shows its first window, by providing SW_SHOWNORMAL if not core.triggerNVDAExit(core.NewNVDAInstance(destPath, _generate_updateParameters())): log.error("NVDA already in process of exiting, this indicates a logic error.") From 052dc4b1f0fb84df47b019837d3732da82a72d5c Mon Sep 17 00:00:00 2001 From: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com> Date: Mon, 5 Aug 2024 12:17:32 +1000 Subject: [PATCH 7/8] Updated to use simpler data structures and function more similarly to `core.restart` --- source/updateCheck.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/source/updateCheck.py b/source/updateCheck.py index 762023b5bb5..080c57fb2e9 100644 --- a/source/updateCheck.py +++ b/source/updateCheck.py @@ -33,6 +33,7 @@ import gui.contextHelp # noqa: E402 from gui.dpiScalingHelper import DpiScalingHelperMixinWithoutInit # noqa: E402 import sys # noqa: E402 +import subprocess import os import inspect import threading @@ -262,24 +263,22 @@ def _generate_updateParameters() -> str: :return: The parameters to pass to the new NVDA instance. """ - executeFlags: set[str] = set() - executeOptions: dict[str, str] = {} + executeParams: list[str] = [] if config.isInstalledCopy(): - executeFlags.update(("--install", "-m")) + executeParams.extend(("--install", "-m")) else: portablePath = globalVars.appDir if os.access(portablePath, os.W_OK): - executeFlags.update(("--create-portable", "-m")) - executeOptions["--portable-path"] = f'"{portablePath}"' + executeParams.extend(("--create-portable", "-m", "--portable-path", portablePath)) else: # We can't write to the currently running portable copy's directory, so just run the launcher. - executeFlags.add("--launcher") + executeParams.append("--launcher") if globalVars.appArgs.disableAddons: - executeFlags.add("--disable-addons") + executeParams.append("--disable-addons") # pass the config path to the new instance, so that if a custom config path is in use, it will be inherited. # If the default con fig path is in use, the new instance would use it anyway, so there is no harm in passing it. - executeOptions["--config-path"] = f'"{WritePaths.configDir}"' - return f"{' '.join(executeFlags)} {' '.join(' '.join(i) for i in executeOptions.items())}" + executeParams.extend(("--config-path", WritePaths.configDir)) + return subprocess.list2cmdline(executeParams) class UpdateChecker(garbageHandler.TrackedObject): From 03be8a35e0d6e1932d4bfeae61aa52eaf77ef823 Mon Sep 17 00:00:00 2001 From: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com> Date: Mon, 5 Aug 2024 17:57:19 +1000 Subject: [PATCH 8/8] Updated the installer to propagate -c and --disable-addons --- source/gui/installerGui.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/source/gui/installerGui.py b/source/gui/installerGui.py index 412e1cb41da..19fd80dad28 100644 --- a/source/gui/installerGui.py +++ b/source/gui/installerGui.py @@ -5,6 +5,8 @@ # Bill Dengler, Joseph Lee, Takuya Nishimoto import os +import subprocess +import sys import winUser import wx @@ -131,11 +133,31 @@ def doInstall( if startAfterInstall: newNVDA = core.NewNVDAInstance( filePath=os.path.join(installer.defaultInstallPath, "nvda.exe"), + parameters=_generate_executionParameters(), ) if not core.triggerNVDAExit(newNVDA): log.error("NVDA already in process of exiting, this indicates a logic error.") +def _generate_executionParameters() -> str: + executeParams: list[str] = [] + if globalVars.appArgs.disableAddons: + executeParams.append("--disable-addons") + # pass the config path to the new instance, so that if a custom config path is in use, it will be + # inherited. We can't rely on WritePaths.configDir or appArgs.configPath here, as they could be set to + # the default path, which may be a temporary directory, so we must sniff sys.argv. + # Read sys.argv backwards, since the last instance of a CLI option takes effect. + # We start from len-2 as the -c option must have a value, so it is guaranteed to take up 2 items in argv. + for i in range(len(sys.argv) - 2, 0, -1): + if sys.argv[i] in ("-c", "--config-path"): + # We don't absolutise -c here because if it was provided by a previous copy of NVDA this will + # have already been done, and if it was provided by the user absolutising it now will result in + # it being in a temporary directory, which is almost certainly not what they want. + executeParams.extend(("--config-path", sys.argv[i + 1])) + break + return subprocess.list2cmdline(executeParams) + + def doSilentInstall( copyPortableConfig=False, startAfterInstall=True, @@ -600,6 +622,7 @@ def doCreatePortable( if startAfterCreate: newNVDA = core.NewNVDAInstance( filePath=os.path.join(portableDirectory, "nvda.exe"), + parameters=_generate_executionParameters(), ) if not core.triggerNVDAExit(newNVDA): log.error("NVDA already in process of exiting, this indicates a logic error.")