Normalize bare drive letters in portable copy directory (#20159) by Mubashir78 · Pull Request #20446 · nvaccess/nvda

Mubashir78 · 2026-07-05T10:32:44Z

Fixes #20159

Link to issue number:

#20159

Summary of the issue:

When entering a bare drive letter (e.g. c:) as the destination directory in the Create Portable NVDA dialog, the error message is confusing -- it says the path must start with a drive letter even though the user did provide one.

Description of user facing changes:

Instead of the generic "not absolute" error, the dialog now shows a specific message: "A drive letter without a trailing backslash (e.g. "c:") is not a valid destination directory. Please use "c:" instead." This makes it clear to the user why c: is not accepted and what they should type instead.

Description of developer facing changes:

No developer-facing changes.

Description of development approach:

Detect bare drive letters using os.path.splitdrive (when a drive letter is found with no trailing path), and show a targeted error message explaining that c: refers to the current directory on drive C, not its root, and suggesting c:\ instead.

Testing strategy:

Unit tests:

test_bareDriveLetter_showsSpecificError -- verifies the specific error mentioning c:\ is shown
test_relativePath_showsError -- verifies the generic error is still shown for non-drive relative paths
test_driveLetterWithBackslash_isAccepted -- no regression
test_driveLetterWithPath_isAccepted -- no regression

Manual testing:

Typed "c:" in the portable copy dialog -> confirmed the specific error message appears
Typed "c:" and "c:\folder" -> confirmed they are accepted
Typed "d:" -> confirmed same specific error message appears

Known issues with pull request:

None.

Code Review Checklist:

Documentation:
- Change log entry
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
API is compatible with existing add-ons.
Security precautions taken.

When entering a bare drive letter (e.g. 'c:') as the destination directory in the Create Portable NVDA dialog, os.path.isabs returns False because 'c:' lacks a trailing separator. Normalize by appending os.sep so the path is recognised as absolute and the portable copy proceeds without error. Adds unit tests for the normalization logic, mocking drive-letter semantics to remain platform-independent.

seanbudd · 2026-07-08T02:25:34Z

Hi - please read our contributing guidelines and fill out the PR template. I've added it back to the PR for you.

seanbudd · 2026-07-08T03:10:00Z

Hi @Mubashir78 - this isn't quite the approach suggested in the comments.
Please refer to #20159 (comment)

Mubashir78 · 2026-07-08T06:08:54Z

Thanks for the feedback. I have reworked the approach based on the discussion in #20159 -- instead of normalizing bare drive letters, the dialog now shows a specific error message explaining that 'c:' is not valid and suggesting 'c:' instead. The PR template has been filled out and a changelog entry added.

seanbudd · 2026-07-09T00:04:34Z

Hi - I've added back the code review checklist here as well. can you please confirm what manual testing you have performed to ensure the user facing changes are working as expected?

Mubashir78 · 2026-07-09T00:36:35Z

Filled out the checklist. Manual testing:

Typed "c:" in the portable copy dialog -> confirmed the specific error message appears
Typed "c:" and "c:\folder" -> confirmed they are accepted
Typed "d:" -> confirmed same specific error message appears

I hope all checks out 👍

seanbudd · 2026-07-09T00:41:27Z

You have removed the description of the unit tests, can you make sure to include description of all tests (unit and manual).
You also removed dot points from the code review check list guide. Please don't remove or edit it other than checking boxes. I have added it back yet again.

Mubashir78 · 2026-07-09T00:50:58Z

My bad, I'm having a hard time making sure i make no mistakes in formal English. Usually my strategy of creating PRs has been minimally explaining the changes I've introduced, where I've introduced them, what steps I took to reproduce and how they overall improve the code flow.

Move _mockSplitdrive and _mockIsabs to module level, inline _msgBox as lambda. Addresses review feedback on nvaccess#20446.

Mubashir78 · 2026-07-09T01:13:57Z

Done:

_mockSplitdrive and _mockIsabs moved from nested inside _runCreatePortable to module-level helpers
_msgBox inlined as a lambda instead of a nested function

seanbudd · 2026-07-09T04:57:07Z

 			self.assertEqual(installer._comparePreviousInstall(), installer.ComparisonState.UPGRADE)
+
+
+def _mockSplitdrive(p: str) -> tuple:


Suggested change

def _mockSplitdrive(p: str) -> tuple:

def _mockSplitdrive(p: str) -> tuple[str, str]:

CyrilleB79 · 2026-07-09T07:00:13Z

@Mubashir78 you write:

Typed "c:" and "c:\folder" -> confirmed they are accepted

I guess you meant "c:", not "c:". If yes, please fix it.

Also, have you tested with "c:folder" (without backslash), which is a valid way to define a non absolute folder. What is the error message in this case?

Mubashir78 · 2026-07-09T07:51:32Z

Renamed _mockSplitdrive to _mockSplitDrive with tuple[str, str] return type
Renamed _mockIsabs to _mockIsAbs
Fixed the typo in PR description: c: should be c:\
On c:folder: falls through to the standard "not absolute" error (splitdrive returns ("c:", "folder"))

Mubashir78 · 2026-07-09T07:54:03Z

I guess you meant "c:", not "c:".

I assume you meant to type "c:", which is a valid path, and yes i have fixed that typo now.
Edit: Ah I see, the markdown eats up the blackslash for some reason here in Github comments.

have you tested with "c:folder" (without backslash),

Yes i have, and confirmed that it falls through to the standard not absolute error. Since splitdrive returns ('c:', 'folder'), the bare drive check does not trigger and the existing error path handles it.

SaschaCowley

I'm a little bit confused by the approach here:

The rule isn't that a drive letter, followed by a colon and nothing else refers to the CWD. The drive letter and colon tell Windows which drive you are referring to; what follows is the path on that drive. So anything that follows the drive letter and colon that isn't a root path isn't absolute.

PS C:\Users\SaschaCowley> cd c:
PS C:\Users\SaschaCowley> cd c:documents
PS C:\Users\SaschaCowley\Documents> cd c:nonexistant
Set-Location: Cannot find path 'C:\Users\SaschaCowley\Documents\nonexistant' because it does not exist.
PS C:\Users\SaschaCowley\Documents> cd d:
PS D:\> cd d:projects
PS D:\projects> cd c:
PS C:\Users\SaschaCowley\Documents> cd d:nvda
PS D:\projects\nvda>

I would suggest having a look at the implementation of ntpath.isabs. Or potentially just switching the logic to something like:

if not os.path.isabs(expandedPortableDirectory ):
	if not os.path.splitroot(expandedPortableDirectory)[1]:
		message = ...  # bare mount point
	else:
		message = ...  # Path without mount point
	gui.messageBox(message, ...)
	return

Note that UNC paths are always absolute.

I'm not sure I understand why all the rigmarole around creating mocked os.path functions.
- The functions you're mocking don't actually touch the file system, so there's no need to mock them;
- NVDA's unit tests can only run on Windows, so it's not like there will be issues where the tests fail because posixpath is used inadvertently; and
- Even if you want to be extra sure that you're using Windows paths, you ccan use ntpath.
Rather than mocking the GUI, wouldn't it be easier to extract the path check into a helper function that either returns the expanded and validated path, or raises pre-defined exceptions (possibly via gui.message.DisplayableError)?

…root per SaschaCowley review - Replace splitdrive with splitroot for precise mount point detection - Extract inline validation into _validatePortableDirectory helper - Two distinct error messages: bare drive (c:, c:foo) vs no drive (foo) - onCreatePortable now delegates to helper - Tests directly test helper with ntpath, no os.path mocking needed

Mubashir78 · 2026-07-09T08:34:42Z

Thanks for the reviews. I have reworked the approach based on the feedback:

@SaschaCowley all three points are addressed in the latest push:

splitroot switched from os.path.splitdrive to os.path.splitroot:

mount, root, _ = os.path.splitroot(expandedPortableDirectory)
if mount and not root:
    # Bare mount point (c:, c:folder)

Now correctly catches both c: and c:folder under the same condition.

No os.path mocking removed _mockSplitDrive and _mockIsAbs. Tests now patch directly with ntpath:

patch("gui.installerGui.os.path.isabs", side_effect=ntpath.isabs)
patch("gui.installerGui.os.path.splitroot", side_effect=ntpath.splitroot)

Extracted helper extracted inline validation into _validatePortableDirectory(path: str) -> str | None. It returns the expanded absolute path or None (error shown via messageBox). onCreatePortable now delegates in 3 lines.

@CyrilleB79 re c:folder: with the new splitroot approach it correctly triggers the bare drive error (mount=c:, root=""), rather than the old generic "not absolute" message. Added test_driveWithRelativePath_showsBareDriveError unit test for this case.

@seanbudd kept the code review checklist intact with parent boxes checked and added description of all tests in the PR body.

Mubashir78 · 2026-07-10T07:42:16Z

May I ask why this PR has been drafted @seanbudd ?

Mubashir78 requested a review from a team as a code owner July 5, 2026 10:32

Mubashir78 requested a review from SaschaCowley July 5, 2026 10:32

seanbudd marked this pull request as draft July 8, 2026 02:25

Mubashir78 and others added 3 commits July 8, 2026 06:07

Improve error message for bare drive letters in portable copy dialog

1b3d890

Add changelog entry for bare drive letter error message

843e46f

Pre-commit auto-fix

1033458

Mubashir78 marked this pull request as ready for review July 8, 2026 06:08

seanbudd reviewed Jul 9, 2026

View reviewed changes

Comment thread tests/unit/test_installer.py Outdated

unnest nested functions in _runCreatePortable

54c3cc2

Move _mockSplitdrive and _mockIsabs to module level, inline _msgBox as lambda. Addresses review feedback on nvaccess#20446.

seanbudd reviewed Jul 9, 2026

View reviewed changes

Comment thread tests/unit/test_installer.py Outdated

seanbudd reviewed Jul 9, 2026

View reviewed changes

SaschaCowley requested changes Jul 9, 2026

View reviewed changes

Mubashir78 force-pushed the fix/20159-drive-path-normalization branch from 9decd51 to b49e67f Compare July 9, 2026 08:18

seanbudd marked this pull request as draft July 10, 2026 04:08

Mubashir78 force-pushed the fix/20159-drive-path-normalization branch from b49e67f to 50743c7 Compare July 10, 2026 19:59

Mubashir78 force-pushed the fix/20159-drive-path-normalization branch from 50743c7 to b49e67f Compare July 10, 2026 20:11

		self.assertEqual(installer._comparePreviousInstall(), installer.ComparisonState.UPGRADE)


		def _mockSplitdrive(p: str) -> tuple:

	def _mockSplitdrive(p: str) -> tuple:
	def _mockSplitdrive(p: str) -> tuple[str, str]:

Uh oh!

Uh oh!

Conversation

Mubashir78 commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes:

Description of developer facing changes:

Description of development approach:

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

seanbudd commented Jul 8, 2026

Uh oh!

seanbudd commented Jul 8, 2026

Uh oh!

Mubashir78 commented Jul 8, 2026

Uh oh!

seanbudd commented Jul 9, 2026

Uh oh!

Mubashir78 commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanbudd commented Jul 9, 2026

Uh oh!

Mubashir78 commented Jul 9, 2026

Uh oh!

Uh oh!

Mubashir78 commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

seanbudd Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

CyrilleB79 commented Jul 9, 2026

Uh oh!

Mubashir78 commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mubashir78 commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SaschaCowley left a comment

Choose a reason for hiding this comment

Uh oh!

Mubashir78 commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mubashir78 commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mubashir78 commented Jul 5, 2026 •

edited

Loading

Mubashir78 commented Jul 9, 2026 •

edited

Loading

Mubashir78 commented Jul 9, 2026 •

edited

Loading

Mubashir78 commented Jul 9, 2026 •

edited

Loading

Mubashir78 commented Jul 9, 2026 •

edited

Loading

Mubashir78 commented Jul 9, 2026 •

edited

Loading