Is it possible to postpone the release of 2020.4 to incorporate this fix PR?

hello @lukaszgo1
Recommend to change the merge master to beta
thanks

@feerrenrut wrote:

Could you please run through the testing for #9679 and add the procedure and results to the testing section of this PR? I'm a little concerned this may break cases for that.

Done. As expected all cases from #9679 were working correctly with this PR as it only affects the checkbox being enabled and disabled and not the actual install procedure.

Does this change now allow config to be copied to itself with the --copy-portable-config option?

I'm afraid I don't understand what you're asking here. You cannot copy the config directory to itself obviously.

I think the help for --copy-portable-config is now misleading, to me it sounds like you must also specify --config-path for it to work? Could you confirm or clarify the help please?

No you don't need to specify configPath for it to work. For me current help message is pretty clear - that is if you specify --copy-portable-config NVDA would copy current config to the appdata of the current user. Fell free to suggest better wording for the help message.

Perhaps you can explain the full flow for processes, rather than just the changes you are making. This would help anyone reading this to be on the same page with how the underlying system works.

Since the failsafe in core.py has been removed, looking at just the diff of this PR it seems like it would allow copying from user directory to itself if --copy-portable-config command line argument was provided.
Could you highlight where this is prevented?

The help for --copy-portable-config currently says:

When installing, copy the portable configuration from the provided path (--config-path, -c) to the current user account

It doesn't say what happens when there is no provided path. It should probably mention where the config is copied from when run as a portable copy explicitly. IE don't assume the reader know where a portable config is kept.

@feerrenrut wrote:

Perhaps you can explain the full flow for processes, rather than just the changes you are making. This would help anyone reading this to be on the same page with how the underlying system works.

Since the failsafe in core.py has been removed, looking at just the diff of this PR it seems like it would allow copying from user directory to itself if --copy-portable-config command line argument was provided.
Could you highlight where this is prevented?

In the gui.installerGui.doInstall

The help for --copy-portable-config currently says:

When installing, copy the portable configuration from the provided path (--config-path, -c) to the current user account

It doesn't say what happens when there is no provided path. It should probably mention where the config is copied from when run as a portable copy explicitly. IE don't assume the reader know where a portable config is kept.

I've reworded the help message in a way which avoids mentioning --configPath at all - is it better now?

@feerrenrut Would you be able to review latest changes here? It is pretty annoying to copy config from the portable version after installation each time.

I think the docs for these commands could still be clarified, but first lets make sure we are on the same page about the behavior and intentions here.

My understanding of this is:

• A portable copy has its config stored with it in a subdirectory (userConfig)
• An installed copy has its config stored in %APPDATA%/nvda
• The main use case for --copy-portable-config, is when running a portable copy, and wanting to install and have the newly installed NVDA version configured in the same way as the portable copy. This is done via the "Install NVDA" option in the tools submenu of the NVDA menu, when the 'Copy portable configuration to current user account ' option is checked.
• "Install NVDA" is not shown for installed copies. But it is shown when running from portable or from the launcher.
• When "Install NVDA" is selected, the installation happens from within the current running process. The launcher is not started, and the config option --copy-portable-config is not used.
• However, for some admins or advanced users, it is useful to be able to install in NVDA in a silent manner from a portable copy. In this case the --copy-portable-config option is useful.
• --copy-portable-config should also protect against copying over the currently in use config.
• Using the --copy-portable-config option should also honor the --config-path argument which typically specifies the configuration that NVDA should use to run, and is the source configuration to copy.

The copy portable config code seems to consider running with the launcher, but what happens when this argument is used on an installed version of NVDA. I think it should probably give an error.

I think the docs for these commands could still be clarified, but first lets make sure we are on the same page about the behavior and intentions here.

My understanding of this is:

• A portable copy has its config stored with it in a subdirectory (userConfig)
• An installed copy has its config stored in %APPDATA%/nvda

In both these cases location of the config can be overwritten by providing --configpath from the command line when starting NVDA.

• The main use case for --copy-portable-config, is when running a portable copy, and wanting to install and have the newly installed NVDA version configured in the same way as the portable copy. This is done via the "Install NVDA" option in the tools submenu of the NVDA menu, when the 'Copy portable configuration to current user account ' option is checked.
• "Install NVDA" is not shown for installed copies. But it is shown when running from portable or from the launcher.
• When "Install NVDA" is selected, the installation happens from within the current running process. The launcher is not started, and the config option --copy-portable-config is not used.

That's not true - if --copy-portable-config is provided from the cli the checkbox in the GUI is automatically checked

• However, for some admins or advanced users, it is useful to be able to install in NVDA in a silent manner from a portable copy. In this case the --copy-portable-config option is useful.
• --copy-portable-config should also protect against copying over the currently in use config.
• Using the --copy-portable-config option should also honor the --config-path argument which typically specifies the configuration that NVDA should use to run, and is the source configuration to copy.

The copy portable config code seems to consider running with the launcher, but what happens when this argument is used on an installed version of NVDA. I think it should probably give an error.

That is an interesting point. Currently nothing happens but it is no different to other command line options which are not applicable in the current situation. Consider the following two cases:

• nvda --install-silent from the run dialog if NVDA is already installed it tries to install itself again and fails since it cannot overwrite its own files.
• nvda -aaaaaa - there is no indication that invalid option has been given from the command line.

I agree we should clean this up and present an error for options which are not applicable in the given state, however I don't think this should be done as part of tis PR.

That's not true - if --copy-portable-config is provided from the cli the checkbox in the GUI is automatically checked

Can you clarify this. Running which executable from the CLI (launcher, portable NVDA, installed NVDA)?

I seem to have confused two use cases here:

• Using the GUI to install from portable.
• Using the CLI to install from portable NVDA.exe passing the the --copy-portable-config option.

It might be helpful to explicitly mention (and give examples) of some of the CLI use cases in the docs. There are a lot permutations of arguments, ways to run them, and NVDA state. It would be easy for one of these situations to be broken. Anything we can do to reduce this would be good.

It seems to me to only make sense to use the --copy-portable-config command line argument when running a portable copy, when also specifying '--install' or --install-silent. It should also be acceptable to optionally use --portable-path and --config-path to specify the locations for config files for source and destination respectively.

I feel most of the confusion here comes from naming of the argument since --copy-portable-config suggests that the option works only for portable copies which is not true. What this option in fact does is it just copies a configuration used during NVDA install for silent installations unless location of the config equals location to which config would be copied, and for GUI installs from the portable copies it affects state of the checkbox used to decide if portable config should be copied or not. It probably be better to name this option --copy-current-config-during-install or similar.
It is unfortunate these concerns weren't brought up when reviewing #9679 since now it is too late to rename the command line parameter.

It is unfortunate these concerns weren't brought up when reviewing #9679 since now it is too late to rename the command line parameter.

IMO, the flag may be renamed if it adds some clarity; also the flag documentation should be updated to cover all use cases.
The old flag could still be supported during a certain amount of time in order not to break existing scripts and then eventually be deprecated.

I'm in support of adding a new alias for this argument. But it would be good to name it carefully. I don't like the word "current", I think it is ambiguous between the running copy which is potentially very temporary and the already installed or about to be installed copy. What about --copy-config-to-installation or --copy-config-during-installation. Then in the help also add:

optionally use --portable-path and --config-path to specify the locations for config files for source and destination respectively. If not specified the default locations will be used.

Then in the help for --portable-path and --config-path explicitly state the default locations. EG:

• userConfig directory relative to the nvda.exe
• %APPDATA%/nvda

--portable-path is irrelevant here as it is just the folder in which portable copy of NVDA is created - it has no effect when installing.

@feerrenrut While I do share your concerns about better naming of this option, documentation not being extensive enough to cover all possible use cases and giving warnings when options not applicable in the current situation are provided from the CLI I feel it is way more important not to break GUI installations from the portable copies than improving command line arguments. Since it would be great to move this PR forward and include it in 2021.1 would you agree to proceed as follows:

• Merge this as is reverting eb5dd4f
• • create an issue to discuss alternative name for --copy-portable-config, possible improvements to its documentation etc separately.

I'd more than happy to work on this subsequently given I needed to familiarize myself with this code during this PR. Doing it as a follow up ensures that more common use case i.e installing from the GUI is not negatively affected by discussion about way less commonly used CLI options which it seems can take a while.

Ok, I'm fine with that plan. Could you create an new issue to track improving the docs around these CLI arguments?

@feerrenrut As a result of our discussion on this PR I've created:

• NVDA should warn about unknown command line parameters #12795 for warning about unknown CLI params.
• NVDA should warn about command line parameters which makes no sense in the current context #12796 warning aabout parameters not applicable in the current context.
• Documentation of --copy-portable-config should be improved possibly renaming the option in the process #12797 for improvements to the documentation of --copy-portable-config

I'm pretty sure all discussed points are captured in these issues but feel free to add to them if you feel something has been missed.