Fix braille next line command for documents of Notepad++ #17431
Conversation
nvdaes
changed the title
|
I think that this is an API break change since the collapse method is changed in Scintilla TextInfo. So I'll document this in changes for developers. |
See test results for failed build of commit 2591d8d3c0 |
|
@coderabbitai review |
WalkthroughThe changes introduce a new method called Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
source/NVDAObjects/window/scintilla.py (2)
316-321: Document this change in NVDA's developer documentation.Since this overrides the
collapsemethod behavior, it should be documented in the developer changes for NVDA 2024.1 to inform other developers about this change in the Scintilla text info API.
322-324: Consider adding unit tests for the collapse method.To ensure the behavior remains correct across future changes, consider adding unit tests covering:
- Collapsing to end with no selection
- Collapsing to end with selection
- Collapsing to start
Would you like me to help generate the unit test code?
user_docs/en/changes.md (1)
119-119: Improve technical documentation clarityThe developer change description could be clearer and more structured.
-* in `NVDAObjects.window.scintilla.ScintillaTextInfo`, if no text is selected, the `collapse` method is overriden to expand to line if the `end` parameter is set to `True` (#17431, @nvdaes) +* In `NVDAObjects.window.scintilla.ScintillaTextInfo`, the `collapse` method is overridden. + When no text is selected and the `end` parameter is `True`, the method expands to line before collapsing. + (#17431, @nvdaes)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
source/NVDAObjects/window/scintilla.py(1 hunks)user_docs/en/changes.md(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
source/NVDAObjects/window/scintilla.py (2)
Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>
Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.
user_docs/en/changes.md (3)
Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>
Pattern **/*.md: Focus on grammar, spelling, and punctuation. Also consider readability and clarity of contents. Ensure that changes follow the style of one sentence per line, suggest changes if this is not met.
Pattern user_docs/en/changes.md: Ensure each change log entry references an issue or pull request number. Change log entries can also include a reference to a GitHub author. Examples of valid change log entries: * Item with sub-items (#123, @username): * sub-item * bar (#342) * Item with. Multiple lines. (#143)
🔇 Additional comments (1)
source/NVDAObjects/window/scintilla.py (1)
316-324: LGTM! Well-documented fix for the braille next line command.
The implementation correctly addresses the issue by expanding to line level before collapsing to end, when no text is selected. The code is clean and the documentation clearly explains the purpose and links to the issue.
Let's verify this change doesn't affect other Scintilla-based editors:
✅ Verification successful
The change is safe and properly scoped to Scintilla-based editors
The verification shows that Notepad++ is the only editor implementation that inherits from ScintillaBase, and it uses the same text information base class. The collapse behavior modification will be consistently applied across all Scintilla instances, maintaining uniform behavior for braille navigation.
- The change affects
ScintillaTextInfowhich is the base class used by Notepad++ throughScintillaTextInfoNpp83 - No other editor implementations were found that could be impacted differently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Scintilla-based editor implementations
rg -l "class.*\(.*Scintilla.*\)" source/
Length of output: 79
Script:
#!/bin/bash
# Let's check the content of notepadPlusPlus.py to understand how it uses Scintilla
rg -A 5 "class.*\(.*Scintilla.*\)" source/appModules/notepadPlusPlus.py
# Also check for any other references to Scintilla in the codebase
rg "Scintilla" source/ -l
Length of output: 815
Script:
#!/bin/bash
# Let's check the inheritance hierarchy to understand the base implementation
rg -A 10 "class ScintillaBase" source/NVDAObjects/window/scintilla.py
# Also check if there are any other editor implementations that might be affected
rg "class.*\(.*ScintillaBase" source/ --type py
Length of output: 298
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
|
@coderabbitai, please suggest unit tests for this PR. |
|
Tip For best results, initiate chat on the files or code changes. I've prepared some unit tests for the modified import unittest
from unittest.mock import patch
from source.NVDAObjects.window.scintilla import ScintillaTextInfo
import textInfos
class MockScintillaObject:
"""Mock object to simulate Scintilla NVDAObject."""
def __init__(self, selection_is_collapsed=True):
self.selection_is_collapsed = selection_is_collapsed
self.windowHandle = None # Substitute with appropriate value if needed
def makeTextInfo(self, position):
class MockTextInfo:
@property
def isCollapsed(self):
return self._is_collapsed
@isCollapsed.setter
def isCollapsed(self, value):
self._is_collapsed = value
text_info = MockTextInfo()
text_info.isCollapsed = self.selection_is_collapsed
return text_info
class TestScintillaTextInfoCollapse(unittest.TestCase):
def setUp(self):
self.mock_obj = MockScintillaObject(selection_is_collapsed=True)
self.text_info = ScintillaTextInfo(self.mock_obj)
def test_collapse_end_with_collapsed_selection_expands_line(self):
with patch.object(self.text_info, 'expand') as mock_expand, \
patch('textInfos.offsets.OffsetsTextInfo.collapse') as mock_super_collapse:
self.text_info.collapse(end=True)
mock_expand.assert_called_once_with(textInfos.UNIT_LINE)
mock_super_collapse.assert_called_once_with(end=True)
def test_collapse_end_with_non_collapsed_selection_does_not_expand(self):
self.mock_obj.selection_is_collapsed = False
with patch.object(self.text_info, 'expand') as mock_expand, \
patch('textInfos.offsets.OffsetsTextInfo.collapse') as mock_super_collapse:
self.text_info.collapse(end=True)
mock_expand.assert_not_called()
mock_super_collapse.assert_called_once_with(end=True)
def test_collapse_not_end_does_not_expand(self):
with patch.object(self.text_info, 'expand') as mock_expand, \
patch('textInfos.offsets.OffsetsTextInfo.collapse') as mock_super_collapse:
self.text_info.collapse(end=False)
mock_expand.assert_not_called()
mock_super_collapse.assert_called_once_with(end=False)
if __name__ == '__main__':
unittest.main()Explanation:
Notes:
Let me know if you need any further assistance or adjustments to these tests. 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 12366 Script: Length of output: 11864 |
|
Since ScintillaTextInfo doesn't have unit tests, I'll wait for developers comments about this. |
seanbudd
added
the
conceptApproved
Co-authored-by: Sean Budd <[email protected]>
|
Thanks for your review, @seanbudd . I've applied your suggestions. |
Link to issue number:
Fixes #17430
Summary of the issue:
Next braille line command doesn't work in Notepad++ documents when the last line is empty.
Description of user facing changes
Next braille line command works as expected in Notepad++ documents.
Description of development approach
The collapse method is overriden for Scintilla textInfo, so that textInfo is expanded to line before collapsing to end. In this way the cursor is properly moved to the end of the document when running the braille next line command.
Testing strategy:
Tested manually in portable Notepad++ on Windows 11.
Known issues with pull request:
None. Should be tested in different versions of Notepad++ and Windows.
Code Review Checklist:
Summary by CodeRabbit
Release Notes for NVDA 2024.1
New Features
Bug Fixes
Improvements