Split addonHandler module #17797

ctoth · 2025-03-07T15:42:27Z

Link to issue number:

N/A - Code organization improvement

Summary of the issue:

The addonHandler module was overly large with all functionality in init.py, making it difficult to maintain and understand. This PR reorganizes the code into smaller, more focused modules while preserving the same functionality, and adds tests for the AddonBundle component.

Description of user facing changes

None. This is a purely internal refactoring of the codebase with no user-facing changes.

Description of development approach

This PR splits the large addonHandler/__init__.py file into several logical modules:

addonBase.py - Contains base classes and common exceptions
addon.py - Contains the main Addon class implementation
AddonBundle.py - Contains bundle-related functionality
AddonManifest.py - Contains manifest-related functionality
addonState.py - Manages the state of addons in NVDA

The PR focuses on maintaining functionality while improving code organization. The main additions are unit tests for the AddonBundle functionality, which previously lacked test coverage.

Testing strategy:

Added comprehensive unit tests for the AddonBundle module
Manually verified that addon functionality still works correctly
Ensured that existing tests pass with the new structure

Known issues with pull request:

None

Code Review Checklist:

Documentation:
- Change log entry - N/A (internal code reorganization)
- User Documentation - N/A (no user-facing changes)
- Developer / Technical Documentation - Improved through better docstrings
- Context sensitive help for GUI changes - N/A (no GUI changes)
Testing:
- Unit tests - Added new tests for AddonBundle
- System (end to end) tests - Existing tests validate the functionality
- Manual testing - Verified addon installation/management works as before
UX of all users considered - N/A (internal refactoring only)
API is compatible with existing add-ons - Preserved full API compatibility
Security precautions taken - No changes to security model

@coderabbitai summary

Split addon handling code into separate modules: - AddonBase.py - Base classes AddonBase and AddonError - AddonBundle.py - Bundle class for handling addon packages - AddonManifest.py - Manifest class and manifest handling This improves code organization by: - Separating concerns into logical modules - Reducing complexity of __init__.py No functional changes, only code reorganization.

Move the Addon class from addonHandler/__init__.py into its own module (addon.py) to improve code organization and readability. Also move AddonStateCategory enum and state management from addonStore/models/status.py to addonHandler/addonState.py where it more logically belongs. This is a refactoring change that: - Reduces the size of addonHandler/__init__.py by extracting Addon class - Places addon state management closer to the addon code - Improves overall code modularity and organization

Tests cover bundle initialization, validation, extraction, and creation from directories, including various error handling scenarios. Both mocked and real file tests ensure proper functionality with different file types and path formats.

SaschaCowley · 2025-03-21T02:25:39Z

@ctoth is this PR ready for review?

source/out

seanbudd · 2025-03-28T04:28:43Z

source/addonHandler/AddonBundle.py

@@ -0,0 +1,121 @@
+# A part of NonVisual Desktop Access (NVDA)


please use lowerCamelCase for file names i.e. addonBundle

seanbudd · 2025-03-28T04:29:08Z

source/addonHandler/AddonManifest.py

@@ -0,0 +1,160 @@
+# A part of NonVisual Desktop Access (NVDA)


please use lowerCamelCase for file names i.e. addonManifest

source/addonHandler/AddonManifest.py

source/addonHandler/AddonBundle.py

seanbudd · 2025-03-31T02:59:38Z

source/addonHandler/AddonManifest.py

+
+	configspec = ConfigObj(
+		StringIO(
+			"""


could we perhaps move this to a separate ini file that is read from at run time?

seanbudd · 2025-04-09T07:07:57Z

tests/unit/test_addonHandler/test_addonBundle.py

@@ -0,0 +1,575 @@
+# A part of NonVisual Desktop Access (NVDA)


Note: this file has not been fully reviewed - however the rest of this PR has been

seanbudd · 2025-04-09T07:10:17Z

source/addonHandler/__init__.py

-
-	def __repr__(self) -> str:
-		return f"{self.__class__.__name__} ({self.name!r} at path {self.path!r}, running={self.isRunning!r})"
+def getCodeAddon(obj=None, frameDist=1):


please add type hints

seanbudd · 2025-04-09T07:13:15Z

source/addonHandler/__init__.py

-	log.warning("Error loading manifest:\n%s", manifest.errors)
-
-
-class AddonManifest(ConfigObj):


this appears to be an API breaking change, it's missing from __all__

seanbudd · 2025-04-09T07:13:30Z

source/addonHandler/__init__.py

-		return minRequiredVersion <= lastTested
-
-
-def validate_apiVersionString(value: str) -> Tuple[int, int, int]:


this appears to be an API breaking change, it's missing from __all__

seanbudd · 2025-04-09T07:17:55Z

source/addonHandler/__init__.py

-		return addonAPIVersion.getAPIVersionTupleFromString(value)
-	except ValueError as e:
-		raise ValidateError('"{}" is not a valid API Version string: {}'.format(value, e))
+__all__ = [


Manually verified that addon functionality still works correctly

Can you confirm how you tested this? did you check the diff of dir(addonHandler) from the NVDA python console?

AppVeyorBot · 2025-04-09T09:16:56Z

Build (for testing PR): https://ci.appveyor.com/api/buildjobs/3q0agfdlh0ltpehs/artifacts/output/
CI timing (mins):
INIT 0.0,
INSTALL_START 1.2,
INSTALL_END 1.0,
BUILD_START 0.0,
FINISH_END 18.3

See test results for failed build of commit 8a56a26377

seanbudd · 2025-06-24T06:27:08Z

@ctoth - do you intend to work on this still?

seanbudd · 2025-08-08T07:06:37Z

@ctoth - closing this as abandoned. Feel free to open a new PR if you wish to continue to make these changes

ctoth added 16 commits March 6, 2025 09:22

Move AddonsState class to a dedicated module

6d017cd

Move bundle filename, add __all__

2146c78

docs: Update docstrings to use Sphinx format with type annotations

8fe6675

docs: Update docstrings to use Sphinx format with type annotations

31825a5

docs: Update docstrings to use Sphinx format with type annotations

589ecf8

docs: Update docstrings to use Sphinx format with type annotations

f9f9d4b

Types

c448fbe

Update copyright headers for addon handler files

2625939

typing

a13ea5c

Sort __all__

f97c071

Clean up naming

48a719d

Add unit tests for AddonBundle module

90052cf

Tests cover bundle initialization, validation, extraction, and creation from directories, including various error handling scenarios. Both mocked and real file tests ensure proper functionality with different file types and path formats.

'Styling

4020091

Merge branch 'master' into addonCleanup

e9565b2

ctoth closed this Mar 21, 2025

ctoth reopened this Mar 21, 2025

ctoth added 3 commits March 20, 2025 20:57

Merge branch 'master' into addonCleanup

8c78cc4

Refactor: Cleanup and organize add-on related imports

55b1f22

addonHandler: Simplify import paths and standardize import style

4d7f3fd

ctoth marked this pull request as ready for review March 21, 2025 03:47

ctoth requested a review from a team as a code owner March 21, 2025 03:47

ctoth requested a review from seanbudd March 21, 2025 03:47

CyrilleB79 reviewed Mar 21, 2025

View reviewed changes

source/out Outdated Show resolved Hide resolved

Remove

4f59ef7

seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Apr 1, 2025

seanbudd added this to the 2025.2 milestone Apr 4, 2025

seanbudd reviewed Apr 9, 2025

View reviewed changes

Apply suggestions from code review

8c4bca8

seanbudd marked this pull request as draft April 9, 2025 07:23

seanbudd removed this from the 2025.2 milestone Jun 17, 2025

seanbudd added the blocked/merge-conflicts Merge conflicts exist on this PR label Jun 24, 2025

seanbudd closed this Aug 8, 2025

seanbudd added the Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. label Aug 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Split addonHandler module #17797

Split addonHandler module #17797

Uh oh!

ctoth commented Mar 7, 2025

Uh oh!

SaschaCowley commented Mar 21, 2025

Uh oh!

Uh oh!

seanbudd Mar 28, 2025

Uh oh!

seanbudd Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

seanbudd Mar 31, 2025

Uh oh!

seanbudd Apr 9, 2025

Uh oh!

seanbudd Apr 9, 2025

Uh oh!

seanbudd Apr 9, 2025

Uh oh!

seanbudd Apr 9, 2025

Uh oh!

seanbudd Apr 9, 2025

Uh oh!

AppVeyorBot commented Apr 9, 2025

Uh oh!

seanbudd commented Jun 24, 2025

Uh oh!

seanbudd commented Aug 8, 2025

Uh oh!

Uh oh!

		@@ -0,0 +1,121 @@
		# A part of NonVisual Desktop Access (NVDA)

		@@ -0,0 +1,160 @@
		# A part of NonVisual Desktop Access (NVDA)

		@@ -0,0 +1,575 @@
		# A part of NonVisual Desktop Access (NVDA)

		log.warning("Error loading manifest:\n%s", manifest.errors)


		class AddonManifest(ConfigObj):

		return minRequiredVersion <= lastTested


		def validate_apiVersionString(value: str) -> Tuple[int, int, int]:

Uh oh!

Split addonHandler module #17797

Split addonHandler module #17797

Uh oh!

Conversation

ctoth commented Mar 7, 2025

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

SaschaCowley commented Mar 21, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Apr 9, 2025

Uh oh!

seanbudd commented Jun 24, 2025

Uh oh!

seanbudd commented Aug 8, 2025

Uh oh!

Uh oh!