Make config options and GUI settings match

[CyrilleB79]

Link to issue number:

Closes #14170

Summary of the issue:

When setting line indentation reporting on tone in a profile (e.g. Notepad) and on speech for default profile, you end up with line indentation set on Speech and tones instead of tones only when turning back to Notepad (see #14170 for detailed steps).

The issue is due to the following:
Only one combo-box allows to set line indentation reporting in the UI. But the value of this combo-box is controlled by two items in the configuration. These two items are then managed separately when dealing with configuration profile switching.

The following combo-boxes in the GUI are impacted by this issue:

• "Line indentation reporting" in doc formatting settings (see initial description of Line indent - Wrong option selected when switching profile #14170)
• "Cell borders" in doc formatting settings (see Line indent - Wrong option selected when switching profile #14170 (comment))
• "Show messages" combined with "Message timeout" in braille settings (see Line indent - Wrong option selected when switching profile #14170 (comment))
• "Tether Braille" in braille settings (see Line indent - Wrong option selected when switching profile #14170 (comment))

Description of user facing changes

The values of the following settings should not change unexpectedly anymore when switching profile:

• Line indentation in Document formatting settings.
• Cell borders in doc formatting settings
• Show messages in braille settings
• Tether Braille in braille settings

Description of development approach

• One control in the GUI now matches one and only one item in the config
• Upgrade the config adequately
• Used the same approach as Reporting table headers can now be configured separately for rows and columns #14132 (and the fix in Properly convert strings to booleans when upgrading profiles to the new schema #14183)

A note on profile upgrade

In some cases, the config does not allow to know the selected option. Let's take as an example the following config: reportIndent = True and reportIndentWithTones not present in the config.

1. If the config is for default profile, we are sure that the selected option in GUI for line indent is "with speech".
2. If this config is for another profile (profile "foo") stacked on default profile, the GUI line indent option also depends on the option selected in default profile:
   • if line indent in default profile is disabled, the line indent GUI option in foo profile will be "with speech"
   • if line indent in default profile is set on "With tones", the line indent GUI option in foo profile will be "with both speech and tones"

In such situations where one cannot exactly know the option selected in the GUI, the implementation uses the following rules by order of priority:

• assume that the user has modified the config only via the GUI (not via commands in Python console)
• assume first that the upgraded profile is default profile; only if the upgraded config is not possible in default config it is assumed that it can be for another profile.

Testing strategy:

Manual tests:
For each combobox:

• Tested that each option works
• Tested (with a test profile) that config is correctly upgraded for each possible option.
• when applicable, ("line indent", "cell borders" and "tether to") tested the corresponding cycle/toggle script
• Tested the STR of the corresponding issue in Line indent - Wrong option selected when switching profile #14170.

Unit tests added for each GUI option selected from default profile as well as for other corner cases.

Known issues with pull request:

Issue 1: potential issues with the upgrade of non-default profiles

Some issues may remain when upgrading NVDA with profiles. Below is an example on a fresh NVDA:

• Set line indent to "speech" in default profile
• Set line indent to "both speech and tones" in a separate test profile
  Before upgrade, default profile's config will contain the following [documentFormatting] section:

[documentFormatting]
	reportLineIndentation = True

Before upgrade, test profile's config will contain the following [documentFormatting] section:

[documentFormatting]
	reportLineIndentationWithTones = True

Although indentation is reported by speech and tones in test profile, reportLineIndentation = True is not specified in test profile's config file since it is inherited from default profile.

When upgrading, the test profile will have line indent set on "tones" instead of speech and tones.
This is because profiles are upgraded one at a time, but their config is defined according to the config on which they are stacked.

Should I indicate somewhere the following information?

If one of the following parameters have been modified in non-default profiles:

• Line indentation in Document formatting settings.
• Cell borders in doc formatting settings
• Show messages in braille settings
• Tether Braille in braille settings
  in some rare cases, the value of these parameters may be modified unexpectedly when transitioning from NVDA 2022.4 to 2023.1. Please check the configuration of your profiles.

Issue 2: changes for dev / API / deprecation

• should removal of fields in the config be considered API-breaking? I guess yes, but this has not been the case before (maybe by mistake?)
• in any case, should these changes in the config be specified in the change for developers?

Change log entries:

Bug fixes

- Some options are not modified unexpectedly in a profile anymore when modifying another profile in some rare cases. (#14170)
  - In some rare cases, the following options may still be unexpectedly modified when installing this version of NVDA for the following options in profiles: Line indentation and Cell borders in Document formatting settings, Show messages and Tether Braille in braille settings. Please check these options in your profiles with this new version of NVDA.

Deprecation:

- ``braille.BrailleHandler.TETHER_*`` are deprecated. Please use ``configFlags.TetherTo.*.value`` instead.  (#14233)

API breaking changes:

- Some keys in the configuration have been removed or modified (#14233).
  - In ``[documentFormatting]`` section:
    - ``reportLineIndentation`` stores an int value (0 to 3) instead of a boolean and ``reportLineIndentationWithTones`` has been removed.
    - ``reportBorderStyle`` and ``reportBorderColor`` have been removed and are replaced by ``reportCellBorders``.
  - In ``[braille]`` section:
    - ``noMessageTimeout`` has been removed and ``messageTimeout`` cannot take the value 0 anymore; these use cases are replaced by values in ``showMessages``.
    - ``autoTether`` has been removed; ``tetherTo`` can now take the value "auto" instead.

Code Review Checklist:

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

[seanbudd]

It looks like include/sonic was mistakenly changed in this PR

[seanbudd]

Thanks for picking this up and fixing this, looks pretty good

[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/ugter6xqkxunak9l/artifacts/output/nvda_snapshot_pr14233-26826,93f0d787.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.8,
  INSTALL_END 0.8,
  BUILD_START 0.0,
  BUILD_END 22.2,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.2,
  TEST_START 0.0,
  TEST_END 14.0,
  FINISH_END 0.1

See test results for failed build of commit 93f0d78708

[lukaszgo1]

Given some potential issues with upgrading of profiles have you considered modifying the upgrade steps, so that they accept profiles present lower in the stack as an additional parameter? While this introduces additional complexity breaking user config is hardly ideal, and we may encounter similar cases in the future, so solution of some kind would probably be needed later on.

[CyrilleB79]

Thanks @lukaszgo1 for the review.

Given some potential issues with upgrading of profiles have you considered modifying the upgrade steps, so that they accept profiles present lower in the stack as an additional parameter?

The main problem is that the profiles stack is not always the same. For example, if you have created an application profile for Notepad and a manual profile. Then you upgrade NVDA to 2023.1, there are two scenarios:

• Scenario 1: while in notepad, you first activate the manual profile; then the manual profile would be upgraded taking into account the Notepad profile and the default profile.
• Scenario 2: while NOT in notepad, you first activate the manual profile; then the manual profile would be upgraded taking into account only the default profile.
  Thus, the upgrade would depend on the situation when you have activated first the manual profile.

We could make the assumption that only the default profile was active, what probably covers the majority of the cases. But frankly adding more complexity to it just to cover a few upgrades and even not be sure that they will all be correctly covered is not worth it IMO.

@seanbudd your opinion on this?

While this introduces additional complexity breaking user config is hardly ideal, and we may encounter similar cases in the future, so solution of some kind would probably be needed later on.

If we ensure in the future that only one config item controls one GUI control, we should not get such problems in the future (hoping that I have not forgotten any problematic control). Did you have a specific situation in mind when writing this?

[CyrilleB79]

@seanbudd I have added the type hint you suggested as well as other ones. Thanks.

[CyrilleB79]

Note: I have edited the initial description, especially to make appear the main item in the change log that was not appearing before due to bad markdown formatting.

[feerrenrut]

Do you plan to add tests for ambiguous multi profiles situations?
If so, or it's already covered and I'm not seeing it, then it would be good to highlight the issue and explain how it can affect developers.

[CyrilleB79]

Do you plan to add tests for ambiguous multi profiles situations? If so, or it's already covered and I'm not seeing it, then it would be good to highlight the issue and explain how it can affect developers.

No I do not plan to add more tests unless you ask to. I have only added two unit tests for the non-obvious cases where the upgrade is done on profiles that are assumed not to be the default one given the content of the profile's config:

• test_ManualProfile_CellBordersColorAndStyle
• test_ManualProfile_TetherToReview

It seems to me that further tests highlighting the issue with arbitrary stacking order of profiles were out of the scope of this PR (according to #14233 (comment))

[feerrenrut]

It seems to me that further tests highlighting the issue with arbitrary stacking order of profiles were out of the scope of this PR (according to #14233 (comment))

In that comment I was trying to say that the more important test is the arbitrary stacking order behavior. Because it is the more surprising aspect. That said, if you don't want to write these tests, or they are difficult to write, then we can create a new github issue to track it as a problem that affects developers.

[CyrilleB79]

It seems to me that further tests highlighting the issue with arbitrary stacking order of profiles were out of the scope of this PR (according to #14233 (comment))

In that comment I was trying to say that the more important test is the arbitrary stacking order behavior. Because it is the more surprising aspect. That said, if you don't want to write these tests, or they are difficult to write, then we can create a new github issue to track it as a problem that affects developers.

Yes, I'd prefer have this PR merged as is, without these additional unit tests.
Note that when this PR is merged, we the situation with stacked profile will already be improved since there will no be options changing unexpectedly.

The confusion that can remain is when the same option is defined in two profiles (e.g. application profile and manual profile), the value of the option will depend on which profile is activated last.

If you don't mind, I'll let you create the new issue for this, so that you can express more clearly than I what you are expecting. Thanks.