Replace nvda_dmp with an integrated implementation#18480
Conversation
|
I think we should remove the old DMP option entirely if we are switching, and trial making the new option default |
|
Agreed with @seanbudd |
|
Agree in principle, but I'd like to have this more thoroughly tested in a non backwards compat breaking release if possible |
|
What makes you think that testing this in a non api breaking release is necessary? |
Sorry, I mean removing the old implementation. |
|
Ah that makes sense. I don't think there's anything API breaking when you move to the native implementation, everything is in private methods. Unless someone uses the process directly in an add-on, but that's just weird. |
cd9b6b5 to
c8dfd90
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a native implementation of the diff-match-patch algorithm to improve terminal text processing performance and reliability by eliminating the need for an external process.
- Adds
DiffMatchPatchNativeclass that calls the fast-diff-match-patch library directly within NVDA - Updates configuration options to include the new "Diff Match Patch (native)" algorithm choice
- Modifies the automatic algorithm preference to favor the native implementation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| user_docs/en/userGuide.md | Documents the new native diff algorithm option in advanced settings |
| user_docs/en/changes.md | Adds changelog entry describing the performance improvement and deprecation notice |
| source/gui/settingsDialogs.py | Updates UI to include new "Diff Match Patch (native)" option in combo box |
| source/diffHandler.py | Implements the core DiffMatchPatchNative class and updates algorithm selection logic |
| source/config/configSpec.py | Adds "dmp-native" as a valid configuration option |
|
The CI failures seem unrelated to this PR? |
|
@codeofdusk - could you please answer @LeonarddeR's latest question? What do you think would be API breaking here? |
The removal of the
In the past, we've typically put terminal changes behind advanced config settings (#9614, #14047, etc.) to allow for long periods of testing time with an easy revert path. This is also standard practice for non-terminal features (#10885, #11214, etc.). Is there a strong need to remove nvda_dmp before 2026.1? |
|
Using that utility directly is not part of the NVDA API. Similarly, we wouldn't consider changes to l10nUtil to be API breaking. |
959b3b0 to
360a372
Compare
|
@seanbudd |
follow-up #18480 follow-up #19105 Follow-up #19196 Summary of the issue: Commit references in projectDocs/dev/createDevEnvironment.md were out of date. There were also references to nvda_dmp and the old ftd2xx.py. Description of user facing changes: None Description of developer facing changes: Description of runtime submodules now more accurately reflects reality. Description of development approach: Updated commit references and removed outdated items.
Link to issue number:
Closes #16633
Summary of the issue:
Right now, NVDA uses nvda_dmp as a proxy to interact with the fast-diff-match-patch library. This approach is inefficient, inflexible, and sometimes unreliable.
Description of how this pull request fixes the issue:
Remove
nvda_dmpcompletely, replacing it with a refactoreddiffHandler.DiffMatchPatchclass that calls DMP directly.Testing strategy:
Tested over a few days of heavy terminal usage across both wt and formatted UIA conhost, including the alt buffer. Alpha testing.
Known issues with pull request:
None known
Code Review Checklist: