Skip to content

Separate font attribute reporting between speech and braille#16748

Merged
seanbudd merged 51 commits intonvaccess:masterfrom
SaschaCowley:formattingSeparateSpeechandBraille
Jul 12, 2024
Merged

Separate font attribute reporting between speech and braille#16748
seanbudd merged 51 commits intonvaccess:masterfrom
SaschaCowley:formattingSeparateSpeechandBraille

Conversation

@SaschaCowley
Copy link
Copy Markdown
Member

@SaschaCowley SaschaCowley commented Jun 26, 2024 •
edited
Loading

Link to issue number:

Closes #16755

Summary of the issue:

NVDA only allows showing font attributes in both speech and braille, or not at all. Some users want this information reported in only one modality.

Description of user facing changes

Font attribute reporting can now be set to off, speech, braille, or speech and braille. The document formatting settings panel has been updated to use a dropdown box rather than a checkbox for "Report font attributes". The toggle report font attributes gesture now cycles among the 4 available options.

Description of development approach

Added a new enum, OutputMode, to store the user's choice. An integer enum was chosen, as we want to enumerate the options. The values were chosen such that it is essentially a feature flag (i.e., SPEECH = 1 and BRAILLE = 2), so that we can test for the bit we're interested in.

Added a new configuration key, documentFormatting.fontAttributeReporting, which uses this enum. Added a configuration migration that sets this new key to SPEECH_AND_BRAILLE if documentFormatting.reportFontAttributes is True and OFF if it's False, so that without configuration modification there is no change to NVDA's behaviour.

To maintain backwards compatibility, added an alias between documentFormatting.reportFontAttributes and documentFormatting.fontAttributeReporting and vice-versa:

  • documentFormatting.reportFormatting will be set to True if documentFormatting.fontAttributeReporting is set to SPEECH, BRAILLE, or SPEECH_AND_BRAILLE, and False if it is set to OFF.
  • documentFormatting.fontAttributeReporting will be set to SPEECH_AND_BRAILLE if documentFormatting.reportFontAttributes is set to True, or OFF if it is set to False.

Updated speech/speech.py and braille.py to check that the appropriate bit of reportFontAttributes is set before outputting font attributes to that modality.

Updated script_toggleReportFontAttributes to cycle through the available modes, by incrementing the current value mod the number of possible values.

Updated various places where documentFormatting.reportFontAttributes is used as a bool to use documentFormatting.fontAttributeReporting instead.

Testing strategy:

Implemented unit tests for the configuration migration and the aliasing. Manually tested speech and braille output by testing bolded, italicised and underlined text in Word.

Known issues with pull request:

None

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.

@SaschaCowley
Added a output mode config flag
d5af0b8
@SaschaCowley
Updated configuration to separate font attribute reporting into off, …
02f5d3f 
…speech, braille, and speech and braille.

source/config/configSpec.py: Updated the configuration specification to use OutputMode as the type for documentFormatting.reportFontAttributes. Incremented schema version.
source/config/profileUpgradeSteps.py: Added a profile upgrade step from 11 to 12.
source/gui/settingsDialogs.py: Updated the settings GUI to reflect the change in config spec.
@SaschaCowley
Updated configuration to separate font attribute reporting into off, …
65b12e8 
…speech, braille, and speech and braille.

source/config/configSpec.py: Updated the configuration specification to use OutputMode as the type for documentFormatting.reportFontAttributes. Incremented schema version.
source/config/profileUpgradeSteps.py: Added a profile upgrade step from 11 to 12.
source/gui/settingsDialogs.py: Updated the settings GUI to reflect the change in config spec.
@SaschaCowley
Added unittests for profile upgrade steps
c8ac176
@SaschaCowley
Fixed settings so that they can now be saved again
87828f9
@SaschaCowley
Updated speech and braille to respect the new reportFontAttributes se…
daf7f60 
…tting
@SaschaCowley
Merge branch 'formattingSeparateSpeechAndBraille' of https://github.c…
9233587 
…om/SaschaCowley/nvda into formattingSeparateSpeechAndBraille
@SaschaCowley
Fixed upgrade steps to use as_bool
67092fa
@SaschaCowley
Added error handling for the case where a non-boolean value is given …
5d277f9 
…as the value of reportFontAttributes. Implemented a test case for same.
@SaschaCowley
Updated toggle report font attributes command to cycle through the mo…
cc142ae 
…des that are now available
@SaschaCowley
Merge branch 'master' into formattingSeparateSpeechandBraille
0976433
@Adriani90
Copy link
Copy Markdown
Collaborator

Adriani90 commented Jun 26, 2024

Thanks very much sascha for this nice work.
This addresses only partially #1885, actually the request is to add these combo boxes to all formating related settings, not only to attributes.
And indeed this mihgt make sense to all document formating settings in that category, not only font related reportings as also outlined by @LeonarddeR.

@AppVeyorBot
Copy link
Copy Markdown

AppVeyorBot commented Jun 26, 2024

See test results for failed build of commit eeecc628bd

@CyrilleB79
Copy link
Copy Markdown
Contributor

CyrilleB79 commented Jun 26, 2024

I know the PR is still in draft state.

Though, I mention to get it known early:
I know that it has probably not always been respected in the past. But I wonder if changing the type of a value in the config spec is considered as an API-breaking change.
Is an add-on allowed to use config.conf['documentFormatting']['reportFontAttributes']? I mean "allowed" in the sense of using only parts of the public API. If yes, it's type should not change in API-non-breaking releases. If no, there are probably some add-ons which rely on it anyway; what would be the recommended way to know about NVDA's config in the future for them?

Moreover, in the case of document formatting config, there seems to be a quite strong link between NVDA's config (config.conf['documentFormatting']['reportFontAttributes'])
and the parameter passed to the getTextWithFields functions.

@SaschaCowley
Copy link
Copy Markdown
Member Author

SaschaCowley commented Jun 26, 2024

Thanks very much sascha for this nice work. This addresses only partially #1885, actually the request is to add these combo boxes to all formating related settings, not only to attributes. And indeed this mihgt make sense to all document formating settings in that category, not only font related reportings as also outlined by @LeonarddeR.

This work is the first part of our push to improve font attribute support in braille. Since not all of the options in document formatting are supported in braille, not all of them are going to be converted just yet. We are working on supporting more attributes in braille, and the corresponding options will be updated when support is added.

@SaschaCowley
Copy link
Copy Markdown
Member Author

SaschaCowley commented Jun 27, 2024

I wonder if changing the type of a value in the config spec is considered as an API-breaking change. Is an add-on allowed to use config.conf['documentFormatting']['reportFontAttributes']? I mean "allowed" in the sense of using only parts of the public API. If yes, it's type should not change in API-non-breaking releases. If no, there are probably some add-ons which rely on it anyway; what would be the recommended way to know about NVDA's config in the future for them?

Thanks for this feedback. I've updated the PR to have a separate configuration key for this new type, which is transparently aliased to the old one.

Moreover, in the case of document formatting config, there seems to be a quite strong link between NVDA's config (config.conf['documentFormatting']['reportFontAttributes']) and the parameter passed to the getTextWithFields functions.

These functions use config.conf['documentFormatting']['reportFontAttributes'] (actually now fontAttributeReporting) in if statements, where it is implicetly cast to bool, so no action is needed.

@AppVeyorBot
Copy link
Copy Markdown

AppVeyorBot commented Jun 27, 2024

See test results for failed build of commit 9738079287

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jul 2, 2024
@SaschaCowley SaschaCowley marked this pull request as ready for review July 2, 2024 05:38
@SaschaCowley SaschaCowley requested review from a team as code owners July 2, 2024 05:38
@SaschaCowley SaschaCowley marked this pull request as ready for review July 10, 2024 04:13
LeonarddeR
LeonarddeR reviewed Jul 10, 2024
View reviewed changes
Qchristensen
Qchristensen approved these changes Jul 10, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

User guide changes look good. Great work.

LeonarddeR
LeonarddeR approved these changes Jul 10, 2024
View reviewed changes
seanbudd
seanbudd reviewed Jul 11, 2024
View reviewed changes
seanbudd
seanbudd reviewed Jul 11, 2024
View reviewed changes
seanbudd
seanbudd reviewed Jul 11, 2024
View reviewed changes
@SaschaCowley SaschaCowley requested a review from seanbudd July 11, 2024 06:14
seanbudd
seanbudd approved these changes Jul 11, 2024
View reviewed changes
@seanbudd seanbudd merged commit e1d0d9d into nvaccess:master Jul 12, 2024
@SaschaCowley SaschaCowley mentioned this pull request Jul 16, 2024
Closed
@SaschaCowley SaschaCowley mentioned this pull request Aug 21, 2024
Merged
5 tasks
@CyrilleB79 CyrilleB79 mentioned this pull request Aug 21, 2024
Merged
@SaschaCowley SaschaCowley deleted the formattingSeparateSpeechandBraille branch August 22, 2024 05:02
seanbudd pushed a commit that referenced this pull request Aug 27, 2024
@SaschaCowley
Remove unit tests for linking code between reportFontAttributes and…
248e023 
… `fontAttributeReporting` (#17035)

Partial fix for #16802

Summary of the issue:
#16748 introduced new behaviour whereby NVDA can be configured to report font attributes in speech, braille, both or not at all. This required the implementation of a new configuration key, documentFormatting.fontAttributeReporting. To avoid a breaking change to the API, aliasing code was introduced to ensure that the new key and its deprecated counterpart were kept in sync. When the API version is updated, these tests will fail as this code will no-longer run. Therefore, these tests need to be removed.

Description of user facing changes
None.

Description of development approach
Deleted tests.unit.test_config.Config_getitem_alias.
@coderabbitai coderabbitai bot mentioned this pull request Oct 1, 2024
Merged
@SaschaCowley SaschaCowley mentioned this pull request Apr 29, 2025
Merged
5 tasks
@CyrilleB79 CyrilleB79 mentioned this pull request Aug 7, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@Qchristensen Qchristensen Qchristensen approved these changes

@CyrilleB79 CyrilleB79 Awaiting requested review from CyrilleB79

+2 more reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@LeonarddeR LeonarddeR LeonarddeR approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Separate font attribute reporting into speech and braille

7 participants

@SaschaCowley @Adriani90 @AppVeyorBot @CyrilleB79 @seanbudd @LeonarddeR @Qchristensen