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

Provide configurable developer scratchpad dir rather than automatic loading of custom code #9238

Merged
merged 7 commits into from Feb 11, 2019

Conversation

Projects
None yet
5 participants
@michaelDCurran
Copy link
Contributor

michaelDCurran commented Feb 4, 2019

Link to issue number:

None.

Summary of the issue:

Before add-ons, the only way to load custom appModules etc was to place them in a subdir of the NVDA user configuration directory. However, once add-ons became available, these subdires became deprecated in favor of add-ons.
However, up to now, code is still automatically loaded from these subdirs, which means that there is still a good chance that the user may end up with a broken NVDA after upgrading to a version that does not support that particular custom code.
We need to either get rid of auto loading custom code, or at least place it behind a configuration option which would be off by default.

Description of how this pull request fixes the issue:

NVDA no longer automatically loads code from subdires in NVDA's user configuration directory. Rather there is now a new option in the Advanced category of NVDA's settings dialog which enables loading of custom code from a Developer Scratchpad directory.
There is also a button to open this directory when enabled.
This directory is called 'scratchpad' within the NVDA user configuration directory, and is only created if the option is enabled.
When Developer scratchpad is enabled, a message is included early on in the log file at level info, indicating this.

Testing performed:

Started NVDA with a clean configuration. Verified that the scratchpad option was off, and no scratchpad directory existed.
Enabled the scratchpad option and opened the developer scratchpad directory. All the required subdirectories existed.
Copied in the nvSpeechPlayer synthDriver into the synthDrivers subdirectory.
Restarted NVDA and selected nvSpeechPlayer from the Synthesizer dialog. The nvSpeechPlayer synth worked correctly.
Disabled developer scratchpad and restarted NVDA.
NVDA started. It failed to locate nvSpeechPlayer as expected, and fell back to the default synthesizer.
I Re-enabled scratchpad and restarted.
NVDA started with nvSpeechPlayer.

Known issues with pull request:

There are going to be users who have old code in their user config dir which will no longer run without them checking it and moving it to the Developer scratchpad or packaging it as an add-on. It is strongly hoped that for code running day-to-day it will be packaged as an add-on. Developer scratchpad should only be used for developing code.

Change log entry:

Changes:

  • NVDA no longer automatically loads custom appModules, globalPlugins and braille and synth drivers from the NVDA user configuration directory. This code should be instead be packaged as an add-on with correct version information, ensuring that incompatible code is not run with current versions of NVDA. For those developing code however, enable NVDA's developer scratchpad directory in the Advanced category of NVDA settings, and place your code in there.

michaelDCurran added some commits Feb 4, 2019

NVDA no longer automatically loads custom code from package directori…
…es in the NVDA user configuration directory. Rather it will load tem from subdirectories in a new 'scratchpad' directory in the NVDA user configuration directory, but only if the open in the advanced category is enabled.
@Brian1Gaff

This comment has been minimized.

Copy link

Brian1Gaff commented Feb 4, 2019

@michaelDCurran michaelDCurran requested review from feerrenrut and leonardder Feb 4, 2019

@leonardder
Copy link
Collaborator

leonardder left a comment

Here are some comments to start with, I didn't have enough time to finish this yet.


In order to test the code while developing, you can place it in a special 'scratchpad' directory in your NVDA user configuration directory.
You will also need to configure NVDA to enable loading of custom code from the Developer Scratchpad Directory, by enabling this in the Advanced category of NVDA's Settings dialog.
The Advanced category also contains a button to easily open the Developer Scratchpad directory if enabled.

This comment has been minimized.

Copy link
@leonardder

leonardder Feb 5, 2019

Collaborator

Space at start of line.

@@ -295,6 +307,7 @@ def getConfigDirs(subpath=None):
@return: The configuration directories in the order in which they should be searched.
@rtype: list of str
"""
log.warning("getConfigDirs is deprecated. Use globalVars.appArgs.configPath instead")

This comment has been minimized.

Copy link
@leonardder

leonardder Feb 5, 2019

Collaborator

I'd use warnings.warn here, which is the pythonic way to raise a warning. See also the validate module in the source repository (not the configobj one). Using warnings.warn also eases in cleanup when we would like to remove those deprecated functions at some point.

This comment has been minimized.

Copy link
@michaelDCurran

michaelDCurran Feb 7, 2019

Author Contributor

Err... where do these warnings show up? I thought they got mapped to debugWarning in the log, but I can't seem to make them appear.

# Python 2.x doesn't properly handle unicode import paths, so convert them.
dirs = [dir.encode("mbcs") for dir in getConfigDirs(subdir)]
dirs.extend(module.__path__ )
module.__path__ = dirs
# FIXME: this should not be coupled to the config module....

This comment has been minimized.

Copy link
@leonardder

leonardder Feb 5, 2019

Collaborator

Is this something to fix as part of this pr, may be? I came up with an alternative in #9151, though I can't yet find the particular comment.

This comment has been minimized.

Copy link
@michaelDCurran

michaelDCurran Feb 7, 2019

Author Contributor

As there is still code that is unrelated to add-ons (I.e. the scratchpad directories) I don't think there is a problem keeping this in config, called from the various modules. Perhaps something we can clean up in future, but I'm not sure I quite see the advantage.

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Feb 8, 2019

Contributor

I think the main goal of cleaning this up would be to simplify / clarify the pathway. At the moment its fairly hard to follow. The mechanism certainly isn't obvious.

That said, I only recommend refactoring it if we can actually make it easier to understand, and it does not require large changes. It would be very easy to change the order of initialization, we would want to think very carefully about whether that might break things.

This comment has been minimized.

Copy link
@leonardder

leonardder Feb 8, 2019

Collaborator

I don't think there's a real advantage, apart from that it might be much more obvious if the addonHandler adds the add-ons to the module paths rather than the config module. I really do not insist on having this changed. It's just the fix me comment that somehow calls us to do something about it while we're at it.

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Feb 8, 2019

Contributor

I'm not convinced it's necessary for this PR, unless there was an easy and safe way to do it.

From a code readability standpoint, I think there is quite an advantage. I expect having a more obvious "enable" mechanism on addons would have saved quite a lot of time for me when working on the addon compatibility checks. I looked into it and gave up a few times, when I eventually decided it was important to know, it probably took a couple of hours of jumping around the code to realize this was essentially the core of it.

@leonardder
Copy link
Collaborator

leonardder left a comment

Review finished

@@ -2545,7 +2572,8 @@ class NVDASettingsDialog(MultiCategorySettingsDialog):
if winVersion.isUwpOcrAvailable():
categoryClasses.append(UwpOcrPanel)
# And finally the Advanced panel which should always be last.
categoryClasses.append(AdvancedPanel)
if not globalVars.appArgs.secure:

This comment has been minimized.

Copy link
@leonardder

leonardder Feb 6, 2019

Collaborator

Nice catch!

@@ -167,16 +167,23 @@ Both App Modules and Global Plugins share a common look and feel.
They are both Python source files (with a .py extension), they both define a special class containing all events, scripts and bindings, and they both may define custom classes to access controls, text content and complex documents.
However, they do differ in some ways.

Custom appModules and globalPlugins can be packaged into NVDA add-ons.
This allows easy distribution, and provides a safe way for the user to install and uninstall the custom code.
Please refer to the Add-ons section later on in this document.

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Feb 8, 2019

Contributor

I think that the concept of an Addon should be introduced first (at least, before appModules and globalPlugins). I think it would paint a clearer picture to start by describing the end goal, an addon, which may contain several components AppModules, GlobalPlugins, synth drivers, braille drives. Make it clear what is optional, and what is not. And describe other files required in the addon (eg the manifest). Then mention as a note, that it can be helpful to develop these components using the scratchpad to avoid having to repackage the addon.

At the moment (and I expect this is due to historical reasons) it the dev guide describes the internal components first, then almost says "by the way this is all packaged as an addon". This coupled with the use of the word plugin, I think makes it hard to get a clear picture of how everything fits together.

Perhaps this is not the right PR to address this in. It would be great if someone who really understood the addon development workflow could think about how to explain this top down.

@michaelDCurran michaelDCurran merged commit d1a8a71 into master Feb 11, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2019.1 milestone Feb 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.