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.

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

Add a script to oopen NVDA config directory #10493

Merged
merged 14 commits into from Mar 27, 2020
Merged

Conversation

lukaszgo1
Copy link
Contributor

@lukaszgo1 lukaszgo1 commented Nov 14, 2019

Link to issue number:

Closes #2214

Summary of the issue:

At the moment when someone wants to open config directory for NVDA he has to navigate there manually in case of portable versions, or use appropriate option from the Start Menu. It would be easier to do that by pressing a keyboard shortcut.

Description of how this pull request fixes the issue:

The functionality used to open config dir was moved from NVDA_slave to a separate function in the new systemUtils module. This function is used by unassigned by default script. User guide was updated accordingly.
As suggested by @feerrenrut I've also moved execElevated and hasUiAcces from the config to the systemUtils. To avoid breaking add-ons they still can be invoked from the config, but I've added a commend stating that they would be removed from there in NVDA 2021.1.

Testing performed:

  • Assigned a gesture to the newly added script, and ensured that it works.
  • Ensured that config directory can be opened from the start menu shortcut.
  • Tested some places in which moved functions were used with a self-signed build in particular:
  • Copying of the user config to the systemConfig
  • Execution of COM registration fixing tool
  • Enabling and disabling NVDA on a secure screen.

Known issues with pull request:

None known

Change log entry:

Section: New features,
New script without gesture assigned by default was added to open NVDA configuration directory from anywhere.

Section changes for developers:
execElevated and hasUiAcces are now moved to the new systemUtils module. They still can be invoked from config, but this possibility is going to be removed in NVDA 2021.1

@Brian1Gaff
Copy link

Brian1Gaff commented Nov 15, 2019

@lukaszgo1
Copy link
Contributor Author

lukaszgo1 commented Nov 15, 2019

No, it does not. Both menu item and a keyboard command are not functional when on a secure desktop.

leonardder
leonardder previously requested changes Nov 18, 2019
source/config/__init__.py Outdated Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
source/gui/__init__.py Outdated Show resolved Hide resolved
user_docs/en/userGuide.t2t Outdated Show resolved Hide resolved
@XLTechie
Copy link
Contributor

XLTechie commented Nov 21, 2019

@lukaszgo1
Copy link
Contributor Author

lukaszgo1 commented Nov 25, 2019

@leonardder Do you still think that the name of this option should be changed?

@leonardder
Copy link
Collaborator

leonardder commented Nov 25, 2019

@lukaszgo1
Copy link
Contributor Author

lukaszgo1 commented Jan 30, 2020

As the only remaining think to be done before this can be merged is to decide on naming maybe you could take a quick look @feerrenrut

@feerrenrut
Copy link
Member

feerrenrut commented Jan 30, 2020

I don't think this is appropriate for most users, as such I'm very reluctant for it to be an option in the tools menu. I would accept it added to the the advanced settings tab.

@Adriani90
Copy link
Collaborator

Adriani90 commented Jan 30, 2020

I agree also, this would be consistent with what we have for the developer scratchpad.

@lukaszgo1
Copy link
Contributor Author

lukaszgo1 commented Feb 1, 2020

@feerrenrut wrote:

I don't think this is appropriate for most users, as such I'm very reluctant for it to be an option in the tools menu. I would accept it added to the the advanced settings tab.

While I understand that you are worried about less experienced users opening config directory by mistake we are already exposing it in the start menu, so it is hardly hidden. The tools menu also contains option to open a Python Console which, in the hand of an inexperienced user, can certainly be more disastrous than being able to look into the config. The problem with adding into the advanced tab of settings is that it cannot be accessed quickly and having a quick access to it was the main reason for #2214 to be created in the first place. As an alternative what about removing this option from the GUI completely and leaving the part of this PR which implements a unassigned script to open config directory? We are exposing some more advanced opttions such as WX inspection tool in this way already, so it seems a reasonable compromise between making it too easy for a beginner to corrupt his configuration, and giving power users easy access to it.

@feerrenrut
Copy link
Member

feerrenrut commented Feb 3, 2020

I think there is a bit of tidying up necessary in those menus. As an aside, I'd be interested in introducing a 'developer mode' toggle on the advanced settings page, then when not checked hide the python console.

As an alternative what about removing this option from the GUI completely and leaving the part of this PR which implements a unassigned script to open config directory?

I'm happy with that approach, go ahead.

@lukaszgo1 lukaszgo1 changed the title Allow opening NVDA config directory from the tools menu. Add a script to oopen NVDA config directory Feb 3, 2020
@lukaszgo1
Copy link
Contributor Author

lukaszgo1 commented Feb 3, 2020

@feerrenrut Changed as discussed above.

source/globalCommands.py Outdated Show resolved Hide resolved
source/config/__init__.py Outdated Show resolved Hide resolved
source/config/__init__.py Outdated Show resolved Hide resolved
@AppVeyorBot

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

@lukaszgo1
Copy link
Contributor Author

lukaszgo1 commented Feb 3, 2020

@feerrenrut All your points are now addressed.

user_docs/en/userGuide.t2t Outdated Show resolved Hide resolved
@lukaszgo1
Copy link
Contributor Author

lukaszgo1 commented Feb 3, 2020

Yes, I've tested that the link is functional.

@feerrenrut
Copy link
Member

feerrenrut commented Feb 5, 2020

Hi @lukaszgo1, have you installed this build and tested the open config directory option from the start menu as well?

@lukaszgo1
Copy link
Contributor Author

lukaszgo1 commented Feb 5, 2020

@feerrenrut wrote:

have you installed this build and tested the open config directory option from the start menu as well?

Thanks for reminding me about this. Testing shows that in the current state it is not functional. The log is as follows:

ERROR - RPC process 2920 (nvda_slave.exe) (12:28:20.889) - Dummy-1 (2800):
__main__.main:
slave error
Traceback (most recent call last):
  File "nvda_slave.pyw", line 70, in main
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "ui.pyc", line 18, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "gui\__init__.pyc", line 24, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "speech\__init__.pyc", line 15, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "api.pyc", line 12, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "review.pyc", line 10, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "NVDAObjects\__init__.pyc", line 16, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "eventHandler.pyc", line 12, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "treeInterceptorHandler.pyc", line 10, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "documentBase.pyc", line 7, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "scriptHandler.pyc", line 13, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "sayAllHandler.pyc", line 8, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "synthDriverHandler.pyc", line 18, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "speechDictHandler\__init__.pyc", line 14, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "speechDictHandler\dictFormatUpgrade.pyc", line 15, in <module>
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 668, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 638, in _load_backward_compatible
  File "speechDictHandler\speechDictVars.pyc", line 10, in <module>
AttributeError: 'NoneType' object has no attribute 'configPath'

I believe it is caused by the fact that when ui is imported it imports speech and it cannot be imported in a separate process. When the function responsible for opening config dir is in the config module as it was initially everything works as it should. Thinking about it some more I do not think that ui is a proper place for it - this module contains functions presenting messages to the user and slave works even when screen reader is not running. Should I try to make it work when the function is in ui module anyway, or would having it in config be acceptable?

@feerrenrut
Copy link
Member

feerrenrut commented Feb 6, 2020

Should I try to make it work when the function is in ui module anyway, or would having it in config be acceptable?

No, I also had some concerns about putting it in UI. I still don't think it should live in config either, but looking at it more closely I see there are a number of "systemUtil" related things. In the future I think these should be moved to a new module. Perhaps you would like to do that in this PR? To maintain backwards compat you would have to import the names back into config and leave a comment to say it will be removed in the future. What do you think?

@lukaszgo1
Copy link
Contributor Author

lukaszgo1 commented Feb 6, 2020

@feerrenrut wrote:

No, I also had some concerns about putting it in UI. I still don't think it should live in config either, but looking at it more closely I see there are a number of "systemUtil" related things. In the future I think these should be moved to a new module. Perhaps you would like to do that in this PR? To maintain backwards compat you would have to import the names back into config and leave a comment to say it will be removed in the future. What do you think?

Could you please give some examples of functions which in your opinion should be moved, just to ensure that we are on the same track? While I have no problem with doing this, I am wondering if it gives any real advantages, except, of course, more logical code layout.

@feerrenrut
Copy link
Member

feerrenrut commented Feb 6, 2020

The main advantage in my opinion is to de-tangle the dependencies, making it easier to understand and verify the correctness of the code.

Looking through the files, the most obvious candidates are:

  • def execElevated
  • def hasUiAccess
  • This new method.

If you do this, you'll have to avoid a circular reference. Since config will need to depend on this module (for execElevated) you'll need to remove dependencies on config from the new module. You can do this by making your new method more general EG def OpenDirectory(path) it can then be used like: OpenDirectory(path=config.GetUserConfigPath()) or similar.

I've only looked at this quickly, there are likely details I've missed. Hopefully this gives the general idea.

lukaszgo1 added 3 commits Mar 16, 2020
* openUserConfigurationDirectory now lives in systemUtils
* hasUiAccess and execElevated are now moved to systemUtils
* they still can be imported from config, but it is deprecated
@lukaszgo1
Copy link
Contributor Author

lukaszgo1 commented Mar 21, 2020

@feerrenrut Sory for the delay. This is ready for another look. I've updated description accordingly.

Copy link
Member

@feerrenrut feerrenrut left a comment

Thanks @lukaszgo1 this looks good.

@feerrenrut feerrenrut dismissed leonardder’s stale review Mar 27, 2020

the changes have been addressed

@feerrenrut feerrenrut added deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release feature labels Mar 27, 2020
@feerrenrut feerrenrut merged commit 4778d28 into nvaccess:master Mar 27, 2020
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2020.1 milestone Mar 27, 2020
feerrenrut added a commit that referenced this pull request Mar 27, 2020
@feerrenrut feerrenrut modified the milestones: 2020.1, 2020.2 Mar 27, 2020
@lukaszgo1 lukaszgo1 deleted the I2214 branch Mar 27, 2020
@kvark128
Copy link
Contributor

kvark128 commented Mar 28, 2020

The systemUtils.openUserConfigurationDirectory function uses config.getUserDefaultConfigPath, which always returns the default path. If the configuration path was changed via the command line, it will open wrong directory.

@lukaszgo1
Copy link
Contributor Author

lukaszgo1 commented Mar 28, 2020

@kvark128 Thanks for your commend. I've created #10911 to address this.

lukaszgo1 added a commit to lukaszgo1/nvda that referenced this pull request Dec 12, 2020
michaelDCurran added a commit that referenced this pull request Jan 21, 2021
* Remove  compatibility  wrappers around `hasUiAccess` and `execElevated` introduced in #10493 from the config module

* Remove deprecated `getConfigDirs` from the config module

* Add missing imports  from `typing` to the config module

* Remove deprecated `canStartOnSecureScreens` from the config module

* Update what's new

Co-authored-by: Michael Curran <mick@nvaccess.org>
@seanbudd seanbudd mentioned this pull request Mar 5, 2021
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

user configuration directory accessible from NVDA's menu
9 participants