Provide configurable developer scratchpad dir rather than automatic loading of custom code#9238
Conversation
…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.
|
Hmm, I tended to use this idea to simply stop the nvda working for certain
self voicing exe files.
I am aware that you should be able to do much the same with profiles but
I'm sure, like me there are a lot of old installations like mine which were
long forgotten and just work.
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal E-mail to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
-----
|
LeonarddeR
left a comment
There was a problem hiding this 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. |
There was a problem hiding this comment.
Space at start of line.
| @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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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.... |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| categoryClasses.append(UwpOcrPanel) | ||
| # And finally the Advanced panel which should always be last. | ||
| categoryClasses.append(AdvancedPanel) | ||
| if not globalVars.appArgs.secure: |
|
|
||
| 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. |
There was a problem hiding this comment.
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.
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: