Made portable creation through the installer or tools menu more flexible, by allowing system variables in paths by XLTechie · Pull Request #14681 · nvaccess/nvda

XLTechie · 2023-02-28T07:13:50Z

Link to issue number:

Summary of the issue:

While it is possible to use system variables (%appdata%, %userprofile%, %temp%, etc.) in the create portable dialog's "browse" sub-dialog, it was not possible to use them in the Create Portable dialog itself. Therefore, when using the installer or the tools menu portable option, one either had to enter full paths, or use the browse dialog to utilize system variables to minimize typing.
For people who create many portables, that can be unnecessary overhead.

Description of user facing changes

It is now possible to use system variables (such as %tmp% or %homepath%) in the path specification while creating portable copies of NVDA.
Additionally, it is no longer necessary to include a drive letter when entering the absolute path to a portable target.

Description of development approach

In gui.installerGUI, there is a method of PortableCreaterDialog class, called onCreatePortable: The path, as entered in the dialog (self.portableDirectoryEdit.Value), was used in this method three times.

Created a local variable, expandedPortableDirectory, containing that value, processed through os.path.expandvars().
Used that local value throughout the remainder of the method.
Added a note to the error which appears if the user doesn't specify an absolute path, stating that system variables may be used.
Rephrased the messages asking for the path, and removed the mention of the required drive letter, per NV Access review comments.
Replaced a % substitution with a .format() string formatter, in an error dialog.
Edited, reformatted, and added missing translator comments.
Miscellaneous linting.
Additional:
In gui.installerGUI.doCreatePortable(): Linting, spelling, added translation comments, replaced substitutions with calls to format

A note regarding no longer checking for drive letter in entered absolute path:

While creating a portable copy, we ask the user for an absolute path. In the os.path module's opinion, a path can be absolute without having a drive letter. Therefore, we previously checked and threw an error if the drive letter was missing.
When @seanbudd suggested removing the drive letter notation from absolute path messages, testing without entering one revealed errors that occurred if there was no drive letter, even though it shouldn't technically matter.

Instead, since os.path.isabs has already determined that the path is absolute, we can just add the drive letter if it's missing.
We can do that by calling os.path.abspath, which absolutizes the (probably mostly) already absolute path, by adding a drive letter if it was missing, and any other path components necessary to have a fully qualified path.

Testing strategy:

Created portables using variables in their paths, from both the installer and the tools menu.
Tested that badly-named variables cause the same error as invalid paths.
Absolute paths with no drive letter work as a user would expect.

Known issues with pull request:

None

Change log entries:

New features
It is now possible to use system variables (such as %temp% or %homepath%) in the path specification while creating portable copies of NVDA.

Bug fixes
gui.installerGUI.PortableCreaterDialog.onCreatePortable now properly checks for system variables used in the entered paths of potential portable copies, instead of automatically showing an invalid path error when system variables are used.

Changes
Portable copy creation no longer requires that a drive letter be entered as part of the absolute path.

Code Review Checklist:

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

seanbudd

Hi @XLTechie - generally looks good to me, added some notes.

XLTechie · 2023-03-27T07:29:06Z

@seanbudd Think I have covered your wishes here. Note that the drive letter is apparently required, per line 431+. I still removed it from the message as you suggested, as I agree it seems redundant to mention given "absolute path".

seanbudd · 2023-03-27T07:53:41Z

 			return
-		drv=os.path.splitdrive(self.portableDirectoryEdit.Value)[0]
+		drv = os.path.splitdrive(expandedPortableDirectory)[0]
 		if drv and not os.path.isdir(drv):


if drv is false-y than this does not trigger, meaning that if there is no drive this continues

@seanbudd You're right, of course, but perhaps it shouldn't. Because when it does, this little piece of joy happens:

DEBUGWARNING - installer.tryCopyFile (05:39:19.980) - gui.ExecAndPump(<function createPortableCopy at 0x041EBA98>) (11828): Unable to copy \\?\C:\Program Files (x86)\NVDA\api-ms-win-core-console-l1-1-0.dll, error 3 DEBUGWARNING - gui.ExecAndPump.run (05:39:19.980) - gui.ExecAndPump(<function createPortableCopy at 0x041EBA98>) (11828): task had errors Traceback (most recent call last): File "gui\__init__.pyc", line 681, in run File "installer.pyc", line 671, in createPortableCopy File "installer.pyc", line 142, in copyProgramFiles File "installer.pyc", line 595, in tryCopyFile OSError: error 3 copying \\?\C:\Program Files (x86)\NVDA\api-ms-win-core-console-l1-1-0.dll to \\?\\users\luke\desktop\badport\api-ms-win-core-console-l1-1-0.dll ERROR - gui.installerGui.doCreatePortable (05:39:20.095) - MainThread (22900): Failed to create portable copy Traceback (most recent call last): File "gui\installerGui.pyc", line 447, in doCreatePortable File "gui\__init__.pyc", line 677, in __init__ File "gui\__init__.pyc", line 681, in run File "installer.pyc", line 671, in createPortableCopy File "installer.pyc", line 142, in copyProgramFiles File "installer.pyc", line 595, in tryCopyFile OSError: error 3 copying \\?\C:\Program Files (x86)\NVDA\api-ms-win-core-console-l1-1-0.dll to \\?\\users\luke\desktop\badport\api-ms-win-core-console-l1-1-0.dll IO - speech.speech.speak (05:39:20.175) - MainThread (22900): Speaking ['Error', 'dialog', 'Failed to create portable copy: error 3 copying \\\\?\\C:\\Program Files (x86)\\NVDA\\api-ms-win-core-console-l1-1-0.dll to \\\\?\\\\users\\luke\\desktop\\badport\\api-ms-win-core-console-l1-1-0.dll', CancellableSpeech (still valid)] IO - speech.speech.speak (05:39:20.175) - MainThread (22900): Speaking ['OK', 'button', CancellableSpeech (still valid)]

I hadn't actually attempted the no drive letter thing before now. I will try to get to the bottom of this later today.
Note that the above was generated from current master, not from this PR.

I think handling the falsey case similar to an invalid drive should fix this.

AppVeyorBot · 2023-03-27T08:56:39Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/9opi3g2nkalo9dbt/artifacts/output/nvda_snapshot_pr14681-27919,4faba732.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.3,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 19.1,
FINISH_END 0.1

See test results for failed build of commit 4faba7320c

…ble, by allowing system variables in paths. (#14681) In the method PortableCreaterDialog.onCreatePortable: The path, as entered in the dialog (`self.portableDirectoryEdit.Value`), was used in this method three times. - Created a local variable, `expandedPortableDirectory`, containing that value, processed through `os.path.expandvars()`. - Used that local value throughout the remainder of the method. - Removed the mention of drive letter in absolute path messages, per NV Access suggestion. - Added a note to the error which appears if the user doesn't specify an absolute path, stating that system variables may be used, and giving examples. - Reformatted a translation comment for the title of that error dialog. - Replaced a % substitution with a .format() string formatter, in an error dialog. - Miscellaneous linting. - Added a few missing translator comments.

…tion comments, replaced substitutions with calls to format

When creating a portable copy, we ask the user for an absolute path. Since technically a path can be absolute without having a drive letter, we previously checked and threw an error if the drive letter was missing, even though Windows doesn't require one to be absolute about the path. Instead, since os.path.isabs has already determined that the path is absolute, all that remains is to add the drive letter if it's missing. We can do that by calling os.path.abspath, which absolutizes the (probably mostly) already absolute path, by adding a drive letter if it was missing, and any other path components necessary to have a pedantically correct path.

XLTechie · 2023-03-28T08:57:20Z

@seanbudd please see revised development approach section of PR comment to explain what I did here and see if this method meets with your approval.

AppVeyorBot · 2023-03-28T09:16:22Z

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/8crcaa1qpvty0599/artifacts/output/nvda_snapshot_pr14681-27931,55b515fe.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.1,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.7,
FINISH_END 0.1

See test results for failed build of commit 55b515fe93

seanbudd

Approach generally LGTM, just some feedback on a comment

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

XLTechie · 2023-03-29T06:36:34Z

@seanbudd, how's that? It's not exactly clear what the drive will be in all possible cases, but it's whatever Windows considers the current drive. Which is probably (always?) the drive NVDA is running on. Short of going back to mandating that a drive be chosen, I'm not sure if we can do any better according to the os.path docs. We can't even confirm that the first component of the path exists, because a user might be using something like "d:\nvda", which isn't yet created.

XLTechie · 2023-03-30T04:28:15Z

Thanks @seanbudd.

XLTechie marked this pull request as ready for review February 28, 2023 07:18

XLTechie requested a review from a team as a code owner February 28, 2023 07:18

XLTechie requested a review from seanbudd February 28, 2023 07:18

seanbudd reviewed Mar 27, 2023

View reviewed changes

Comment thread source/gui/installerGui.py Outdated

Comment thread source/gui/installerGui.py Outdated

Comment thread source/gui/installerGui.py

seanbudd marked this pull request as draft March 27, 2023 04:27

XLTechie marked this pull request as ready for review March 27, 2023 07:25

seanbudd reviewed Mar 27, 2023

View reviewed changes

Luke Davis added 3 commits March 28, 2023 02:19

gui.installerGUI.doCreatePortable(): Linting, spelling, added transla…

26c7e53

…tion comments, replaced substitutions with calls to format

XLTechie requested a review from seanbudd March 28, 2023 08:54

Added a blank line (linting)

2a23ec3

seanbudd reviewed Mar 29, 2023

View reviewed changes

Comment thread source/gui/installerGui.py Outdated

seanbudd reviewed Mar 29, 2023

View reviewed changes

XLTechie and others added 2 commits March 29, 2023 02:15

Incomplete review suggestion

36661e9

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Explain where the os.path.abspath drive letter comes from.

7358507

XLTechie requested a review from seanbudd March 29, 2023 06:41

seanbudd approved these changes Mar 30, 2023

View reviewed changes

seanbudd added 3 commits March 30, 2023 13:27

Merge remote-tracking branch 'origin/master' into fix14680

b2d931c

update changes

f8a5f9c

update changes

d6ffa74

seanbudd merged commit bd9bb48 into nvaccess:master Mar 30, 2023

nvaccessAuto added this to the 2023.2 milestone Mar 30, 2023

XLTechie deleted the fix14680 branch August 3, 2024 00:20

Uh oh!

Conversation

XLTechie commented Feb 28, 2023 • 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 development approach

A note regarding no longer checking for drive letter in entered absolute path:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

XLTechie commented Mar 27, 2023

Uh oh!

seanbudd Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

XLTechie Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

seanbudd Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Mar 27, 2023

Uh oh!

XLTechie commented Mar 28, 2023

Uh oh!

AppVeyorBot commented Mar 28, 2023

Uh oh!

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

XLTechie commented Mar 29, 2023 via email

Uh oh!

XLTechie commented Mar 30, 2023 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

XLTechie commented Feb 28, 2023 •

edited

Loading