Skip to content

Conversation

SaschaCowley
Copy link
Member

@SaschaCowley SaschaCowley commented Jun 26, 2024

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.

…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.
…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.
…as the value of reportFontAttributes. Implemented a test case for same.
@Adriani90
Copy link
Contributor

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

See test results for failed build of commit eeecc628bd

@CyrilleB79
Copy link
Contributor

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
Member Author

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
Member Author

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

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
Copy link
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.

@SaschaCowley SaschaCowley requested a review from seanbudd July 11, 2024 06:14
@seanbudd seanbudd merged commit e1d0d9d into nvaccess:master Jul 12, 2024
@SaschaCowley SaschaCowley deleted the formattingSeparateSpeechandBraille branch August 22, 2024 05:02
seanbudd pushed a commit that referenced this pull request Aug 27, 2024
… `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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate font attribute reporting into speech and braille
7 participants