Walkthrough

The recent updates primarily enhance the creation of portable NVDA copies. Key improvements include adding a warning for non-empty directories, the option to create a new folder for the portable installation, and better command-line options for silent installation. These enhancements aim to prevent accidental file scatter and improve user flexibility during the installation process.

Changes

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant GUI
    participant Core
    participant Filesystem

    User->>GUI: Select 'Create Portable Copy' option
    GUI-->>User: Prompt user to select directory

    User->>GUI: Select directory
    GUI-->>User: Display 'Create new folder' checkbox

    User->>GUI: Check 'Create new folder'
    GUI->>Core: Call `doCreatePortable` with new folder option
    Core->>Filesystem: Check directory status

    alt Directory Not Empty
        Core->>GUI: Prompt warning for non-empty directory
        GUI-->>User: Display warning for non-empty directory
        User->>GUI: Confirm
    end

    Core->>Filesystem: Create portable copy in new folder

    GUI-->>User: Notify completion

Assessment against linked issues

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

@coderabbitai the checkbox for copying current user configuration during portable copy creation is not new

@seanbudd, thank you for pointing this out. The history check confirms that the copyUserConfigCheckbox has indeed been present before this PR. I apologize for the oversight in the AI-generated summary. I will correct this information for future references.

If there's anything else you need, please let me know!

#!/bin/bash
# Search the history of `installerGui.py` for the introduction or changes related to `copyUserConfigCheckbox`
git log -S'copyUserConfigCheckbox' --source --all -p src/installerGui.py

#!/bin/bash
# Find the correct path for `installerGui.py`
fd installerGui.py

# Assuming the correct path is found, the following command should be used:
# git log -S'copyUserConfigCheckbox' --source --all -p -- <correct_path>

#!/bin/bash
# Search the history of `installerGui.py` for the introduction or changes related to `copyUserConfigCheckbox`
git log -S'copyUserConfigCheckbox' --source --all -p -- source/gui/installerGui.py

@coderabbitai full review

I tested this PR and the GUI part is fine, however #16514 can still be reproduced through the command line --create-portable-silent --portable-path=""
At the same time, there is still the issue of inconsistent GUI and command line experience.

@wmhn1872265132 I'd argue that --create-portable-silent should have no warnings, however this needs to be documented. warnings should definitely happen for the --create-portable case

@coderabbitai full review

@gerald-hartig - I've addressed those comments

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/479onasmho1j7yeo/artifacts/output/nvda_snapshot_pr16686-32444,76aca619.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.0,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 10.4,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.3,
  TEST_START 0.0,
  TEST_END 18.4,
  FINISH_END 0.2

See test results for failed build of commit 76aca61962

cc @gerald-hartig @seanbudd
I think #16514 is still not properly addressed.
Recursively deleting a user's files is inherently dangerous and unexpected behavior.

@cary-rowen - can you explain how it is unexpected with it being clearly flagged now with a warning?
overwriting the contents of the folder is a required action, we can in theory do more to minimize the damage, but it is inherently unavoidable. I also don't think it is entirely unexpected for an action like this to delete or overwrite files, you are essentially declaring the directory for NVDA portable use only, however a warning is clearly needed.

We cannot assume that all users of use this parameter are fully aware of it just because a special case is documented.
Although we document it, I think the reason why accidents are called accidents is because they are unforeseen and lead to completely unexpected results.

The key here is that NVDA will delete the user's files, even if they have absolutely nothing to do with NVDA.
If you want to continue the discussion, I suggest reopening #16514 Thanks.

@cary-rowen - are you talking particularly about the install-portable-silent case? I think it would be unusual if this creates a warning as you are specifying silent in this case. Otherwise, accidents are hard to happen, as a confirmation dialog is required before overwriting files, and it is more than documenting the case.

Question, and I'm not sure if it's intended behavior, but I notice that if I manually clear out a portable NVDA copy of everything but its userConfig directory, NVDA says that "[p]roceeding will delete and replace existing files in the directory." Once I do proceed though, the userConfig remains in existence. From reading the dialog and the issue summary, it sounds like existing files would both be deleted and replaced.

While I am glad that my userConfig data wasn't deleted, should that have been the case? Or should the prompt be shortened to just: "Proceeding will replace existing files in the directory"?