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 new extension points for keyboard input and speech pause #17428

Merged
merged 6 commits into from
Dec 4, 2024

Conversation

ctoth
Copy link
Contributor

@ctoth ctoth commented Nov 23, 2024

Add new extension points for raw keyboard events and speech pause state

This commit introduces two new extension points:

  1. inputCore.decide_handleRawKey
  • Called before any NVDA processing of raw keyboard events
  • Allows add-ons to intercept and optionally block keyboard events
  • Provides full context: vkCode, scanCode, extended flag, and press/release state
  • Implemented in both keyDown and keyUp event handlers
  • Returns True to allow normal processing, False to block the key
  1. speech.extensions.speechPaused
  • Notifies when speech is paused or resumed
  • Provides boolean 'switch' parameter (True=paused, False=resumed)
  • Added corresponding unit tests to verify functionality
  • Integrated into existing pauseSpeech() function

Technical Details:

  • Added documentation for both extension points in developerGuide.md
  • Updated speech/init.py to expose the new speechPaused extension point
  • Added test case in test_speech.py to verify extension point behavior
  • Maintains backwards compatibility with existing extensions

These additions enable add-ons to:

  • Implement advanced keyboard interception/modification
  • React to speech pause state changes

Link to issue number:

N/A - New feature addition for extensibility

Summary of the issue:

NVDA needed additional extension points to allow add-ons to:

  1. Intercept and control raw keyboard events before NVDA processing
  2. Monitor and react to speech pause state changes

Description of user facing changes

  • No direct user-facing changes
  • Enables add-on developers to create more sophisticated keyboard handling and speech feedback features
  • All changes are API-level additions that maintain backwards compatibility

Description of development approach

  1. Raw Keyboard Extension Point:

    • Added decide_handleRawKey extension point in inputCore.py
    • Integrated into both keyDown and keyUp event handlers in keyboardHandler.py
    • Full keyboard event context provided (vkCode, scanCode, extended, pressed)
    • Boolean return value controls event propagation
  2. Speech Pause Extension Point:

    • Added speechPaused extension point in speech/extensions.py
    • Integrated into existing pauseSpeech() function
    • Provides pause state through boolean parameter
  3. Documentation:

    • Added entries in developerGuide.md
    • Updated relevant module documentation
    • Added changelog entry

Testing strategy:

  1. Unit Tests:

    • Added test cases in test_speech.py for speechPaused extension point
    • Tests verify extension point notification on pause/resume
    • Tests ensure correct parameter passing
  2. Manual Testing:

    • Verified keyboard extension point with test add-on
    • Confirmed speech pause notifications work as expected

Known issues with pull request:

None identified - straightforward API additions with full test coverage

Code Review Checklist:

  • Documentation:

    • Change log entry added in changes.md
    • Developer documentation updated in developerGuide.md
    • Technical documentation added in module docstrings
    • No GUI changes, so no context help needed
  • Testing:

    • Unit tests added for speechPaused
    • Manual testing performed with test add-on
    • System tests not required (API-level changes)
  • UX of all users considered:

    • No direct user impact
    • Enables better add-on support for all output methods
    • No localization impact (API only)
  • API is compatible with existing add-ons

    • New extension points are additive only
    • No breaking changes to existing APIs
    • Follows established extension point patterns
  • Security precautions taken:

    • Discussed raw keystroke decider security implications with NV Access.

@coderabbitai summary

@ctoth ctoth marked this pull request as ready for review November 23, 2024 17:04
@ctoth ctoth requested a review from a team as a code owner November 23, 2024 17:04
@ctoth ctoth requested a review from SaschaCowley November 23, 2024 17:04
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ctoth - this seems almost ready

source/inputCore.py Outdated Show resolved Hide resolved
source/inputCore.py Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
source/speech/extensions.py Outdated Show resolved Hide resolved
ctoth added a commit to nvda-art/nvda that referenced this pull request Nov 26, 2024
- Add type annotations to decide_handleRawKey extension point parameters
- Rename speechPaused to post_speechPaused to follow coding standards for post-event naming
- Add PR reference (nvaccess#17428) to changelog entry
- Format extension point names in changelog with backticks
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Nov 26, 2024
source/inputCore.py Outdated Show resolved Hide resolved
source/speech/extensions.py Outdated Show resolved Hide resolved
@seanbudd
Copy link
Member

Does this close #14520?

ctoth and others added 3 commits November 30, 2024 00:04
- Add decide_handleRawKey extension point to allow intercepting raw keyboard events
- Add speechPaused extension point for speech pause/resume notifications
- Update documentation to reflect new extension points
- Add unit tests for speechPaused extension point
- Add type annotations to decide_handleRawKey extension point parameters
- Rename speechPaused to post_speechPaused to follow coding standards for post-event naming
- Add PR reference (nvaccess#17428) to changelog entry
- Format extension point names in changelog with backticks
@ctoth ctoth force-pushed the remoteExtensionPoints branch from f2db892 to 836de54 Compare November 30, 2024 05:06
@AppVeyorBot

This comment was marked as outdated.

user_docs/en/changes.md Outdated Show resolved Hide resolved
@CyrilleB79
Copy link
Collaborator

Is this PR triggered by the need to remove monkey patching in NVDA remote?
Or have you other add-ons, usages or projects in mind? If yes, would you mind to share it with us? It's of course not mandatory; just my curiosity...

@nvdaes
Copy link
Collaborator

nvdaes commented Nov 30, 2024

Just to say that I'plan to use the keyboard extension point in the pcKbBrl add-on. Thanks for this PR. Hope this is merged when possible to test it.

source/inputCore.py Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
@seanbudd seanbudd marked this pull request as draft December 1, 2024 23:32
@AppVeyorBot
Copy link

  • PASS: Translation comments check.
  • PASS: License check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • FAIL: System tests (tags: installer NVDA). See test results for more information.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/bbjfqnucm6edp5k1/artifacts/output/l10nUtil.exe nvda_snapshot_pr17428-34659,2777ea2c.exe
  • CI timing (mins):
    INIT 0.0,
    INSTALL_START 1.3,
    INSTALL_END 0.9,
    BUILD_START 0.0,
    BUILD_END 23.3,
    TESTSETUP_START 0.0,
    TESTSETUP_END 0.3,
    TEST_START 0.0,
    TEST_END 18.1,
    FINISH_END 0.2

See test results for failed build of commit 2777ea2c3c

@seanbudd seanbudd marked this pull request as ready for review December 2, 2024 03:48
@ctoth
Copy link
Contributor Author

ctoth commented Dec 2, 2024

@seanbudd Re #14520 we still need one more extension point for speech either directly inside or directly before speech.manager.speak

Unfortunately existing Remote monkeypatches speech.manager.speak rather than speech.speak (for which we could trivially use pre_speech) and to maintain backwards compatibility with existing Remote clients we'll need one more there
Still, very close now. #17429 is also almost the functionality we need, though I don't know if it quite does the job in its current form.

Co-authored-by: Cyrille Bougot <[email protected]>
@seanbudd seanbudd merged commit 6c711fd into nvaccess:master Dec 4, 2024
5 checks passed
@github-actions github-actions bot added this to the 2025.1 milestone Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants