Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix prevent display from turning off #17714

Merged
merged 4 commits into from
Feb 25, 2025
Merged

Conversation

LeonarddeR
Copy link
Collaborator

Link to issue number:

Fixes #17705 
Fixup of #17651

Summary of the issue:

Pr #17651 introduced a feature flag in the general section of the config, but this is not supported yet.

Description of user facing changes

General settings panel can be reopened after saving.

Description of development approach

Rename the flag and make it a boolean.

Testing strategy:

Tested str from #17705 

Known issues with pull request:

I had to rename the config setting because otherwise, configuration would get corrupted on Alpha.

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.

@coderabbitai summary

@LeonarddeR LeonarddeR requested review from a team as code owners February 20, 2025 09:20
@AppVeyorBot
Copy link

  • PASS: Translation comments check.
  • PASS: License check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 8dc7a35975

@CyrilleB79
Copy link
Collaborator

@LeonarddeR, you write:

Pr #17651 introduced a feature flag in the general section of the config, but this is not supported yet.

I guess that this limitation is not logged in a GitHub issue.
Could you open one describing the issue in details? As well as information that you may have already gathered while on this issue investigating #17705. More specifically I wonder, if you have figured it, if the limitation applies to the general section of the config or the General panel in the settings dialog.

@LeonarddeR
Copy link
Collaborator Author

@CyrilleB79 I'm not sure. While this is in fact a shortcoming in the current implementation of feature flags, until now there hasn't been a desire to have a feature flag in the general section, and neither we know whether that desire will ever come back. It feels a bit unnecessary to open an issue that describes a problem that only arises in a certain situation.

@CyrilleB79
Copy link
Collaborator

OK let's wait and see if NV Access is happy to have a checkbox instead of a feature flag for this feature.

Anyway, could you please write here the results of your investigation on the existing limitation with feature flags in General panel or general config section?

@LeonarddeR
Copy link
Collaborator Author

As far as I understand it correctly, all sections that are not part of config.conf.BASE_ONLY_SECTIONS will create an AggregatedSection object in the config manager. This class has logic to account for feature flag, i.e. when setting a value to a feature flag, the value is instantly validated. This doesn't apply to the base only sections, that are just configOBj.Section objects.

@SaschaCowley
Copy link
Member

@LeonarddeR can you please open an issue for the fact that any section in ConfigManager.BASE_ONLY_SECTIONS cannot have a config flag? The issue here is wider than just general settings

@LeonarddeR
Copy link
Collaborator Author

I opened #17723

@AppVeyorBot
Copy link

  • PASS: Translation comments check.
  • PASS: License check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit ead5c96a15

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Feb 25, 2025
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.

Good work

@seanbudd seanbudd merged commit 974fbd9 into nvaccess:master Feb 25, 2025
5 checks passed
@github-actions github-actions bot added this to the 2025.1 milestone Feb 25, 2025
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.

Cannot open parameters dialog twice - NVDA alpha-35316,1065b056
6 participants