Improve flexibility of appModules discovery by allowing to map arbitrary appModule to a given executable.

[lukaszgo1]

Link to issue number:

Related to #13364

Summary of the issue:

Currently when NVDA tries to load an appModule for a given program it just looks for a Python module of the same name as the program's executable. This has its limitations in particular:

• Some names are incompatible with the Python's import system (for example see App module handler: handle executable names with dots as part of the file name #5323 where we resorted to replacing dots with underscores before attempting the import).
• When a single apModule should be used for a multiple executable's we need as many appModules as there are executable's (this also often causes linter failures as these alias modules just star imports everything from the main module).
• Even if the given module name does not contain symbols which are incompatible with importlib they may contain characters which are invalid in ordinary import statements (for one example see Fix crashes in 64-bit builds of Notepad++ 8.3 and later #13364 where the apModule is called "notepad++"). Since "+" is invalid in Python's import statement add-on developers cannot import this module from nvdaBuiltin which means that it cannot be extended in an add-on.

Description of how this pull request fixes the issue:

This PR introduces a mapping of executable names to appModules which is consulted before the given module is imported. It also adds a convenience functions for add-on developers which can be used to register or unregister a specific module for a given program. All alias apModules currently present in the source are marked as deprecated and application's for which they were loaded are mapped to the right appModule using the new map.
Since we cannot remove the old alias app modules care has been taken not to use them - they' re kept only for add-ons developers.

Testing strategy:

Made sure that right modules are loaded for Visual Studio Code Insiders and Media Player Classic Home Cinema x64 - both of these were using alias appModules previously.

Known issues with pull request:

None known

Change log entries:

For Developers

• A new convenience functions registerExecutableWithAppModule and unregisterExecutable were added to the appModuleHandler module. They can be used to use a single appModule with multiple executable's.
• Some alias appModules are marked as deprecated and would be removed in 2023.1. If your code imported from one of them (consult the list below) you should import from the replacement module.

Removed module name	Replacement module
azuredatastudio	code
azuredatastudio-insiders	code
calculatorapp	calculator
code - insiders	code
commsapps	hxmail
dbeaver	eclipse
digitaleditionspreview	digitaleditions
esybraille	esysuite
hxoutlook	hxmail
miranda64	miranda32
mpc-hc	mplayerc
mpc-hc64	mplayerc
notepad++	notepadPlusPlus
searchapp	searchui
searchhost	searchui
springtoolsuite4	eclipse
sts	eclipse
teamtalk3	teamtalk4classic
textinputhost	windowsinternal_composableshell_experiences_textinput_inputapp
totalcmd64	totalcmd
win32calc	calc
winmail	msimn
zend-eclipse-php	eclipse
zendstudio	eclipse

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

[lukaszgo1]

@seanbudd While this has been submitted pretty late in the release cycle it's necessary to fix #13311 and I'd say it is pretty low risk. I would appreciate if this can be included in 2022.1.

[josephsl]

Hi, another use case: some app executable names conflict with built-in module names e.g. time.exe for Clock app in Windows 10 and later. Thanks for looking into this.

[josephsl]

Hi,

Does the custom app module mapping succeed when NVDA is restarted while focused on an app module requiring a custom mapping for the executable name? A good app to test this is with Microsoft Weather app where it contains periods in the middle of the executable name. Also, note that this change will require changes to global plugins, too which are loaded way later than app module for the focused control.

Thanks.

[lukaszgo1]

Does the custom app module mapping succeed when NVDA is restarted while focused on an app module requiring a custom mapping for the executable name?

Yes, it does.

note that this change will require changes to global plugins, too which are loaded way later than app module for the focused control.

Not really. To test this I've commented out the mapping for Media Player x64 in apModuleHandler, created a simple global plugin which just registers a right binary with a right appModule and tested that the right module is loaded both when the application is started after NVDA,d and when NVDA restarts when focused in MPCHC.
Have you tested this and found any issues?

[josephsl]

Hi,

An issue that should be resolved from add-ons themselves: suppose an app module bundled with an add-on imports everything from an app module that was deleted with this PR. This will cause the add-on version of the app module to fail to load because Python could not import a nonexistent module. This affects Windows App Essentials add-on as my policy is to import from the app module with the executable name that's included in supported Windows 10/11 releases (take modern keyboard, for example where TextInputHost.exe is the executable name of the emoji panel included in Windows 10 Version 2004 and later which are releases supported by the add-on). I will be able to change this from the add-on side as soon as this PR is approved and merged provided that I can also resolve the resulting lint issue from add-on code.

Also, can you provide code examples for use by add-on writers for sake of completeness?

Thanks.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• FAIL: System tests. See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/0s98e0bcl98ckvt2/artifacts/output/nvda_snapshot_pr13366-24754,287d1713.exe

See test results for failed build of commit 287d1713d7

[CyrilleB79]

Please see #13364 (comment) regarding the possibility to import a module containing an unusable character in its name, e.g. '+' or '.'.

It's a bit ugly but this seems doable.

[seanbudd]

Has the new API been tested with an addon yet?

The App Modules sections of the developer guide will need updating and potentially other documentation.

Is it possible that addons rely on extending aliases? If so, we need to include a changelog item for that.

[josephsl]

Hi, yes I have tested this change with Windows App Essentials which relies on aliased app modules. Any add-on which includes app modules that derive from alias app modules from NVDA Core must now derive from base app modules (say, hxmail instead of hxoutlook), a change I think is the responsibility of the add-on authors (I have prepared a change in Windows App Essentials add-on that was tested and ready to go as soon as this PR is reviewed, approved, and merged). Note that it might be possible that the executable name for a derive module may change in the future, in which case add-on authors may need to either register the new executable name with an existing app module or import the base app module from Core. A simpler approach would have been to define a list of characters that should be replaced by underscores or an acceptable character (say, calling Notepad++.exe app module notepad__.py), but this PR does open a possibility where it becomes possible to load an app module with executable names that conflict with built-in Python module names such as Windows 10/11 Clock app (time.exe and time.py cannot be used because it conflicts with built-in time.py module from Python library). I understand that this is a late change, but given that people use Notepad++ for various tasks (including NVDA Core and add-on development), I advise reviewing this soon. Thanks.

[seanbudd]

Note that it might be possible that the executable name for a derive module may change in the future, in which case add-on authors may need to either register the new executable name with an existing app module or import the base app module from Core.

Isn't that a similar problem to existing aliases? If an executable name changes, we need to create a new alias. This is a backwards compatible action.

[josephsl]

Hi, perhaps – I guess real-life data will tell us the full story (after this PR is merged). Thanks.

[seanbudd]

is this an API breaking change? does it have to be?

[lukaszgo1]

is this an API breaking change?

Yes, it is, since some add-on may rely on one of the alias appModules removed in this PR

does it have to be?

No, it is possible not to remove these alias appModules in this PR, and delay the removal to 2023.1.

[lukaszgo1]

I've marked this as a draft for now since an issue with this implementation has been found when testing with the notepad++ add-on. Before this PR appModules in add-ons or in the scratchpad folder were prioritized over the one included in NVDA. While this is still the case when the appModule has the same name as the executable for which it is used, this does not work when there is appModule in NVDA which is registered in appModuleHandler as the registered aliases are prioritized over the modules in the package.
I see two ways of dealing with this:

• Reverse the ordering of appModule lookup - first try to import from the appModules and if there is no such module consult the mapping. The problem with this approach, although it is rather a hypothetical one, is that if there is an appModule for an application in NVDA which has the same name as the executable, and some add-on also bundles an apModule for it but rather than naming it with the name of the executable registers an alias for it in appModuleHandler the core module would have a priority.
• Make the rules of the import priorities more complex that is:
  1. Split the map of appModule aliases into two one for add-ons and the second one for the core modules.
  2. First try to lookup the module in the map for add-ons.
  3. If there is no alias try to import the module from appModules package.
  4. Finally if import fails check for alias in the mapping for the core modules and import from there.

[lukaszgo1]

* Make the rules of the import priorities more complex that is:
  
  1. Split the map of appModule aliases into two one for add-ons and the second one for the core modules.
  2. First try to lookup the module in the map for add-ons.
  3. If there is no alias try to import the module from `appModules` package.
  4. Finally if import fails check for alias in the mapping for the core modules and import from there.

This is now implemented.

[lukaszgo1]

@seanbudd This is ready for review.

Is there a way to log a warning that these alias modules are deprecated when importing them?

Yes, I've implemented this in the latest commits.

Perhaps we should open an issue to track the deprecation process for these, and keep the aliases. i.e. let's not announce deprecation until we have a deprecation plan and are confident in this new API.

As far as I can tell the part about which we may not be confident enough is "Is the API proposed in this PR good enough for add-on authors?". Even if it turns out we need to change the way in which binaries are mapped to the modules this should not make a difference to what happens to the alias modules which are currently in core (they should be removed regardless of how exactly binaries are mapped to the App Modules.

[lukaszgo1]

@XLTechie Many thanks for your linguistic corrections!

[josephsl]

Hi, as far as Windows App Essentials goes (which employs alias app modules), I have resolved compatibility issues in dev snapshots and plan to release needed changes to stable channel this summer. Thanks.

[XLTechie]

@XLTechie Many thanks for your linguistic corrections!

You're welcome. :)

[seanbudd]

Generally LGTM, some minor fixes suggested. I agree that the aliases should be removed, but we should create a new issue to track / discuss the removal plan.

As @XLTechie mentioned, there's several instances where a third casing "app Module" has been introduced. I suggest a case sensitive find and replace to "App Module".

[seanbudd]

Suggested change

	# Broad except since we do not know
	# what exceptions may be thrown during import / construction of the app Module.
	except Exception:
	except Exception:
	# Broad except since we do not know
	# what exceptions may be thrown during import / construction of the app Module.

[lukaszgo1]

I'm not sure I follow why this improves things. Usually the comment explaining why something needs to be done precedes the code which is explained i.e. docstring is at the top of the described method etc.

[seanbudd]

I think this is just a personal opinion sorry, couldn't find anything PEP related.
I've found commenting within the scope to be generally more pythonic.
In your example, the docstring is under the function header, in this case, it's within the scope of the exception.

[lukaszgo1]

Generally LGTM, some minor fixes suggested.

I've applied them.

I agree that the aliases should be removed, but we should create a new issue to track / discuss the removal plan.

Please see #13627

As @XLTechie mentioned, there's several instances where a third casing "app Module" has been introduced. I suggest a case sensitive find and replace to "App Module".

Fixed as well.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests. See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/i2tp4vyiuf2rvhb1/artifacts/output/nvda_snapshot_pr13366-25284,1c000cc4.exe

See test results for failed build of commit 1c000cc410