Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create portable dialog should allow use of system variables, as other path entry dialogs in windows do #14680

Closed
XLTechie opened this issue Feb 28, 2023 · 0 comments · Fixed by #14681
Closed

Create portable dialog should allow use of system variables, as other path entry dialogs in windows do #14680

XLTechie opened this issue Feb 28, 2023 · 0 comments · Fixed by #14681
Labels
p5 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority triaged Has been triaged, issue is waiting for implementation.
Milestone
2023.2

Comments

@XLTechie
Copy link
Collaborator

XLTechie commented Feb 28, 2023 •
edited

Is your feature request related to a problem? Please describe.

When creating portable copies of NVDA, as I do often for testing and debugging, having to type full paths into the installer or create portable dialog, when I know there are variables containing those paths and we could be using them, is irksome.

Describe the solution you'd like

When creating portable copies of NVDA, allow use of system variables such as %temp% or %userprofile%, as most/all of the rest of Windows does in its path entry dialogs.

This can be done with a simple call to os.path.expandvars() while path parsing behind the portable creation dialog.

Describe alternatives you've considered

Leave it like it is. It isn't a major problem, just an annoyance; but neither is the fix a difficult one.

Additional context:

Note that the desired behavior is already possible, if system variables are entered in the path field which appears in the "browse" sub-dialog. It is only in the main portable creation dialog that one can't do this.

I remember someone else asking for this, a year or more back. But I couldn't find an issue with the keywords I tried.
@XLTechie XLTechie mentioned this issue Feb 28, 2023
Merged
6 tasks
@seanbudd seanbudd added p5 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority triaged Has been triaged, issue is waiting for implementation. labels Mar 7, 2023
seanbudd added a commit that referenced this issue Mar 30, 2023
@XLTechie @seanbudd
Made portable creation through the installer or tools menu more flexi…
bd9bb48 
…ble, by allowing system variables in paths (#14681)

Fixes #14680

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.

---
Commits:


* Made portable creation through the installer or tools menu more flexible, 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.

* gui.installerGUI.doCreatePortable(): Linting, spelling, added translation comments, replaced substitutions with calls to format

* Stop trying to check for drive letter in entered absolute path.

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.

* Added a blank line (linting)

* Incomplete review suggestion

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

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

* update changes

* update changes

---------

Co-authored-by: Luke Davis <XLTechie+github@newanswertech.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Co-authored-by: Sean Budd <sean@nvaccess.org>
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
p5 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority triaged Has been triaged, issue is waiting for implementation.
Projects
None yet
Milestone
2023.2
3 participants
@seanbudd @XLTechie @nvaccessAuto