soffice: Support text attrs according to IA2 spec

[michaelweghorn]

Link to issue number:

Fixes #15648

Summary of the issue:

So far, LibreOffice was using custom attribute and value
names for reporting text attributes. Spelling errors were
not reported via any attribute. NVDA was using the presence
of a specific underline as heuristic to detect and report
spelling errors.
This works for some cases, but e.g. does not cause misspelled
words on a line being annonced as such when reading a
line in LibreOffice Writer (issue #15648).

Description of user facing changes

Announcement of text attributes also works with
LibreOffice version 24.2 and above.
When announcing a line in LibreOffice Writer, misspelled
words are announced as such with LibreOffice version 24.2
and above.

Description of development approach

Switch LibreOffice from using custom text attribute names
and values to using attributes according to the IAccessible2
text attributes specification
( https://wiki.linuxfoundation.org/accessibility/iaccessible2/textattributes )
instead and implement reporting of the "invalid:spelling;"
attribute for misspelled words:

https://gerrit.libreoffice.org/c/core/+/157804
https://gerrit.libreoffice.org/c/core/+/157845
https://gerrit.libreoffice.org/c/core/+/157867
https://gerrit.libreoffice.org/c/core/+/157939
https://gerrit.libreoffice.org/c/core/+/158088
https://gerrit.libreoffice.org/c/core/+/158089
https://gerrit.libreoffice.org/c/core/+/158090

These changes are contained in LibreOffice >= 24.2.

Adapt NVDA to evaluate those text attributes by
using the already existing implementation from
the IA2TextTextInfo base class in
SymphonyTextInfo._getFormatFieldAndOffsets.

For backwards-compatibility with LibreOffice
versions <= 7.6, keep support for the legacy
attributes and move the handling for that into
a new helper method
SymphonyTextInfo_getFormatFieldFromLegacyAttributesString.

For the case where the legacy attributes are used,
the text attribute string starts with "Version:1;"
(s. the LibreOffice code dropped in
https://gerrit.libreoffice.org/c/core/+/158090 ),
so use that as a criterion what code path to take.

Extract another helper method and address some of the
pre-existing lint issues, but silence
the C901 one for the method that was extracted to handle
the legacy attributes
("'SymphonyTextInfo._getFormatFieldFromLegacyAttributesString' is
too complex (27)").
It's at least already less complex than the single
one was before.

Testing strategy:

Test that incorrect spelling in the middle of a line in LibreOffice
Writer gets announced when testing the scenario described in
issue #15648 with both, the LibreOffice and the NVDA changes
in place.

Test that the character attributes from the sample document
attached to
https://bugs.documentfoundation.org/show_bug.cgi?id=157696
works with these changes in NVDA in place, and both,

• a current LibreOffice development version containing
  the above-mentioned LibreOffice changes

• LibreOffice 7.6.2 which does not contain the above-mentioned
  changes and therefore triggers the "legacy" code path.

  Known issues with pull request:

Requires the above-mentioned LibreOffice changes in addition
to actually make the scenario described in issue #15648
work.

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.

[seanbudd]

Thanks @michaelweghorn - just minor changes suggested

[seanbudd]

Given C901 - could you ensure that we are reducing the function complexity in this change?
This could be done by factoring out the added code into a separate helper function

[michaelweghorn]

I'm not sure how to address this, can can you please be more specific what you want me to move?

The only thing added is this block:

		# LibreOffice >= 24.2 uses IAccessible2 text attributes, earlier versions use
		# custom attributes, with the attrtibutes string starting with "Version:1;"
		if attribsString and attribsString.startswith('Version:1;'):
			formatField = self._getFormatFieldFromLegacyAttributesString(
				attribsString,
				offset
			)
		else:
			formatField, (startOffset, endOffset) = super()._getFormatFieldAndOffsets(
				offset,
				formatConfig,
				calculateOffsets
			)

, for which I'm not sure whether it really makes things more readable to move that to a separate method.

In my eyes, the change already reduces complexity, since it already splits most of the existing method into a helper method (for the legacy handling). As mentioned in the description, for me, the easiest way to simplify that further would be by dropping the legacy handling once it's no more needed. Is there a guideline/policy on whether/how that's done, (e.g. can that be dropped half a year after an old version is end-of-life?), or does compatibility with other versions have to be maintained forever?

[lukaszgo1]

In my eyes, the change already reduces complexity, since it already splits most of the existing method into a helper method (for the legacy handling). As mentioned in the description, for me, the easiest way to simplify that further would be by dropping the legacy handling once it's no more needed. Is there a guideline/policy on whether/how that's done, (e.g. can that be dropped half a year after an old version is end-of-life?), or does compatibility with other versions have to be maintained forever?

There is no clear policy - usually code is kept for as long as practical (i.e. it does not cause errors / require extensive maintenance). That is because there is no way to determine what versions of what applications are used by our users, and also because other screen readers on Windows are usually pretty good at keeping backward compatible with older versions. In this specific case however, we cannot really remove this code, as it is shared between LibreOffice, and Apache OpenOffice, I have no idea if these two product exchange the code, but if not removing the legacy handling would cause regressions for OpenOffice users.

[michaelweghorn]

Thanks for the explanation, @lukaszgo1 ! No, there's no systematic code exchange between LibreOffice and OpenOffice. My personal impression (also from a quick look at the git log) is that OpenOffice is currently mostly in some kind of maintenance mode, so I currently wouldn't expect any larger changes regarding accessibility there.

[seanbudd]

A C901 indicates that code is too complex - long functions are always a readability issue as well as a general complexity issue, as it leads to methods that perform multiple logical actions.
I would encourage moving that new block into a helper function.
With a large amount of inline code, i.e. a C901, removing groupable chunks into helper functions
The overall goal with a C901 is to reduce code complexity of the whole function.
This PR essentially introduces a new C901, while improving an existing one.

When adding noqa comments for C901, please see other examples, like below.
This indicates the procedure we follow to reduce C901 issues in the code base.

# C901 'setFocusObject' is too complex
# Note: when working on setFocusObject, look for opportunities to simplify
# and move logic out into smaller helper functions.

[michaelweghorn]

Thanks, I've extracted another helper method now with the new block of code and everything that was above it and added another comment for the C901 for the method handling the legacy attributes.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/fw8jhxagjer1yxx1/artifacts/output/nvda_snapshot_pr15649-29673,71980e49.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.9,
  INSTALL_END 1.0,
  BUILD_START 0.0,
  BUILD_END 26.0,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.4,
  TEST_START 0.0,
  TEST_END 7.9,
  FINISH_END 0.2

See test results for failed build of commit 71980e49f7

[seanbudd]

Could you please avoid force pushing where possible? When reviewing, a reviewer can review changes since the last review. When a force push occurs, the previous review is lost. This means instead of checking that review comments have been actioned, the review must occur from scratch.

[seanbudd]

Thanks @michaelweghorn

[AppVeyorBot]

• PASS: Translation comments 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/qh2f0lknfy6kpveo/artifacts/output/nvda_snapshot_pr15649-29700,3308a437.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.8,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 23.1,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.3,
  TEST_START 0.0,
  TEST_END 5.1,
  FINISH_END 0.1

See test results for failed build of commit 3308a437a5

[michaelweghorn]

Thanks for the review, @seanbudd !

Could you please avoid force pushing where possible? When reviewing, a reviewer can review changes since the last review. When a force push occurs, the previous review is lost. This means instead of checking that review comments have been actioned, the review must occur from scratch.

Sorry for that, I'll try to keep that in mind for any upcoming PRs. (The background for force-pushing is that for other projects I contribute to, the commits from the PR/MR/Gerrit changes are merged to the target branch as is and having a single "nice" commit is IMHO much more useful for reading and understanding the git history and using tools like git bisect or git blame. If I understand correctly, that's not a problem for NVDA, though, since the commits from the PR branch are not merged as is here anyway.)