Text displayed on a multiline braille display will now word wrap on every row in the braille window, not just at the end

[michaelDCurran]

Link to issue number:

None.

Summary of the issue:

Although displaying a long line of braille on a multiline braille display fills up as much of the display as possible, rows may end in the middle of a word.
NVDA should honor the 'avoid splitting words when possible' Braille setting for all rows on the display.

Description of user facing changes

When displaying braille on a multiline braille display, NVDA will avoid splitting words over lines if the 'avoid splitting words when possible' braille setting is on.

Description of development approach

Rather than tracking a braille window as only _windowStartPos and _windowEndPos, internally store a list of window rows which map to start and end offsets in the braille buffer. These offsets are calculated each time the window is moved or the buffer is updated, and ensure that words are not split across window rows.

This PR also adds / changes the filters available for external code such as Remote to dictate the size of the display:

• braille: Added a filter_displayDimensions extension point which takes a namedtuple of numRows and numCols, allowing external code to dictate the number of rows and columns for the display. This should be used now in place of filter_displaySize. This stops the worry of external code changing the number of rows but forgetting to change the over all size.
• braille.handler: Added a displayDimensions property which:
  • creates a DisplayDimensions namedtuple of the display's real numRows and numCols.
  • Filters this with filter_displayDimensions.
  • Calculates displaySize from the product of the filtered numRows and numCols.
  • If at least one handler is registered on the old displaySize filter:
    • Filters this with the old filter_displaySize.
    • If the displaySize changes due to filtering, for backwards compatibility the DisplayDimensions numRows is set to 1 and numCols is set to the filtered displaySize.
    • If the DisplayDimensions have changed at all from the internal cache after all the filters and calculations, then fire the handle_displaySizechanged extension point passing displaySize, numRows and numCols.

This all means that new NVDA / remote code could specify full display dimensions, but for backwards compatibility, if the old displaySize filter is used, then braille is forced to a single row with displaySize cells.

Testing strategy:

• Tested NVDA with a Dot pad braille display. Used NVDA in various standard situations for a length of time, displaying long lines of documents and controls, ensuring that splitting words is avoided.
• Performed the same above test but by dictating display should have 3 rows and 10 columns via the displayDimensions filter. Ensured that content only filled a 3 by 10 space on the DotPad and nothing was cut off and I could still pan to see more.
• Performed the same above test but by dictating display should have a display size of 15 via the old displaySize filter. Ensured that content only filled a single line of 15 cells on the DotPad and nothing was cut off and I could still pan to see more.
• Tested with an Orbit Reader single line display, ensuring that displaying and cursor routing still function as before this PR.

Known issues with pull request:

Code only applies the displaySize filter if at least one handler has registeref for the filter. this is checked by len(list(filter_displaySize.handlers)). There is a technical race condition here as a handler could unregister before the filter is applied. However the worse case is really just that a multiline display will stay singleline for the current display update.
In future it might be nice if the idea of checking for extension point Filter registrations could be standardized better, in particular some kind of atomic call that both communicated if handlers were registered and filtered a value if so.

Code Review Checklist:

• Documentation:
  • Change log entry
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• API is compatible with existing add-ons.
• Security precautions taken.

Summary by CodeRabbit

• New Features

  • Improved handling of braille display windows with enhanced buffer management and row-based display logic.
  • Introduced a new method to convert window-relative positions to buffer-relative positions, offering better content control.

• Bug Fixes

  • Restructured display dimensions management for more accurate rendering, avoiding word splits in multi-row configurations.

• Tests

  • Added unit tests for resizing cell arrays to ensure functionality across various scenarios.
  • Updated existing tests to align with the new display dimensions structure.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: License check.
• FAIL: Unit tests. See test results for more information.
• 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/i0ykiuknl4mea5j6/artifacts/output/nvda_snapshot_pr17011-33388,8595881d.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.2,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 12.4,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.4,
  TEST_START 0.0,
  TEST_END 14.0,
  FINISH_END 0.2

See test results for failed build of commit 8595881dc0

[michaelDCurran]

I'm not too sure what to do about the displaySize property and the failing unit tests yet. Does displaySize as it is now, still make sense now we have numRows and numCols? I assume that displaySize is used for nvda remote for making the size of the two braille displays match. If so, what should we do in the case of multiline displays?

[LeonarddeR]

I think the problem with the unit tests lies in that the new code relies on numCols, wherea's the unit tests enable the braille handler with a registration to braille.filter_displaySize. In that case, numCols on the display will be 0.
I now also understand your question about the displaySize property. May be the handler should also have displayRows and displayCols auto properties.

• displayRows should just return self.display.numRows
• displayCols should return self.displaySize // self.numRows
  In that case, backwards compatibility is preserved. However, when I control a remote machine with a 40 cell display attached with my machine that has a multi row display (e.g. two rows of 40 cells), we'll end up having 20 cells per row rather than only one row with 40 cells. This can be fixed by making the displayRows property a bit smarter:
• If display.numRows is 1 or displaySize > numCells, just return numRows
• If display.numRows > 1 and displaySize < display.numCells, return something like min(self.display.numRows, math.ceil(self.displaySize / self.display.numCols))
  This means that in the example above, a displaySize of 20 will use the top row, 40 will use the top row, 50 will use 40 cells of the top row and 10 cells of the second row.

[bramd]

In general this looks good to me. I'll test it on real hardware over the weekend.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: License check.
• FAIL: Unit tests. See test results for more information.
• 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/k0q2g45bn0ikids2/artifacts/output/nvda_snapshot_pr17011-33448,51320040.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.3,
  INSTALL_END 1.0,
  BUILD_START 0.0,
  BUILD_END 12.9,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.4,
  TEST_START 0.0,
  TEST_END 14.0,
  FINISH_END 0.1

See test results for failed build of commit 513200406b

[AppVeyorBot]

• 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/k6scyeoqldcd2c0a/artifacts/output/nvda_snapshot_pr17011-33458,49f88354.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.5,
  INSTALL_END 1.0,
  BUILD_START 0.0,
  BUILD_END 13.3,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.4,
  TEST_START 0.0,
  TEST_END 18.4,
  FINISH_END 0.1

See test results for failed build of commit 49f8835459

[AppVeyorBot]

• 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/efmlxmio8dnm3a6t/artifacts/output/nvda_snapshot_pr17011-33460,9a8022cc.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.3,
  INSTALL_END 1.0,
  BUILD_START 0.0,
  BUILD_END 13.0,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.4,
  TEST_START 0.0,
  TEST_END 18.4,
  FINISH_END 0.1

See test results for failed build of commit 9a8022cc23

[michaelDCurran]

@LeonarddeR Thanks for your ideas re displaySize and displayNumRows.
I extended this to also add a filter_displayNumRows extension point similar to displaySize, which means in your example, your machine can in theory report that it has 2 rows of 40 cells. The controlled machine will only see the first row of 40 cells of course.
As we move forward with multiline, I think it is important to maintain row / column layout when remoted, and just pad or truncate rows and columns where necessary, rather than wrapping over rows etc. As eventually we are going to want something like one region per row, and the ability to pan horizontally to see more of all the regions at the same time.
A part from other fixes and addition to unit tests, the main changes I made around your ideas were:

• braille: add a filter_displayNumRows extension point, allowing external code to override the number of rows on a braille display, similar to displaySize.
• braille.handler: add a displayNumRows property, similar to the displaySize property, which filters display.numRows via filter_displayNumRows, and if it changes from last time, calls handle_displaySizeChanged extension point, with an extra keyword argument of displayNumRows.
• braille.handler: add a displayNumCols property, which calculates number of columns based on displaySize and displayNumRows.
• braille.BrailleBuffer: calculations that use a display's numrows and numCols now use the handlers's displayNumRows and displayNumCols, so that they can be controled by external code (E.g. remote).
• braille.handler: add a _normalizeCellArraySize method, which takes a list of braille cells of up to length oldCellCount and sequencially formatted into oldNumRows, and reformats the cells list so that it becomes newCellCount in length and sequencially formatted into newNumRows, padding or truncating rows where necessary. Examples:
  • If oldCellCount is 100, oldNumRows is 5 (oldNumCols is 20), and newCellCount is 70, newNumRows is 7 (newNumCols is 10), then the content will have two extra blank rows, and each row will be cut off at 10 cells.
  • If oldCellCount is 70, oldNumRows is 7 (oldNumCols is 10), and newCellCount is 100, newNumRows is 5 (newNumCols is 20), then the last two rows will be cut off, and each row will be padded out by another 10 blank cells.
• braille.handler._writeCells: replace the old code that padded out the cells to display size, and instead process the cells with _normalizeCellArraySize.

[michaelDCurran]

As this pr adds a new filter_displayNumRows extension point, it does mean it becomes backward incompatible. However, in the worse case, where the controlled machine had a 10 row display of 20 cells each, and the controlling machine had a singleline display of 40, the controling machine will see a single line of 40 made up of the 2 original rows of 20 placed beside each other.
Of course once the controlling machine was updated and could use the displayNumrows filter, then both machines would correctly see just 1 row of 40 cells.

[seanbudd]

Apologies for the late extra review - caught these when reviewing unresolved conversations