Skip to content

Fix prevent display from turning off#17714

Merged
seanbudd merged 4 commits into
nvaccess:masterfrom
LeonarddeR:idleFix
Feb 25, 2025
Merged

Fix prevent display from turning off#17714
seanbudd merged 4 commits into
nvaccess:masterfrom
LeonarddeR:idleFix

Conversation

@LeonarddeR

@LeonarddeR LeonarddeR commented Feb 20, 2025

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

AppVeyorBot commented Feb 20, 2025

Copy link
Copy Markdown
  • 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

CyrilleB79 commented Feb 20, 2025

Copy link
Copy Markdown
Contributor

@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

LeonarddeR commented Feb 20, 2025

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

CyrilleB79 commented Feb 20, 2025

Copy link
Copy Markdown
Contributor

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

LeonarddeR commented Feb 20, 2025

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

SaschaCowley commented Feb 21, 2025

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

LeonarddeR commented Feb 21, 2025

Copy link
Copy Markdown
Collaborator Author

I opened #17723

@AppVeyorBot

AppVeyorBot commented Feb 21, 2025

Copy link
Copy Markdown
  • 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
seanbudd
seanbudd approved these changes Feb 25, 2025
View reviewed changes
Comment thread source/gui/settingsDialogs.py Outdated
Qchristensen
Qchristensen approved these changes Feb 25, 2025
View reviewed changes

@Qchristensen Qchristensen left a comment

Copy link
Copy Markdown
Member

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
@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

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@Qchristensen Qchristensen Qchristensen approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

2025.1

Development

Successfully merging this pull request may close these issues.

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

6 participants

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