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

Hide option to show braille messages indefinitely when messages timeout is set to 0 #11602

Conversation

dawidpieper
Copy link
Contributor

@dawidpieper dawidpieper commented Sep 14, 2020

Summary of the issue:

When user sets braille messages timeout to 0, option to show them indefinitely still appears. Its checking does not take any effect, though. I found it confusing for some people.

Description of how this pull request fixes the issue:

Now there is a combobox that enables users to set if braille messages should be displayed and for how long:

  • Disabled - disables showing of braille messages
  • Use timeout - Enables users to set custom timeout
  • Show indefinitely - Messages never disappear automatically.

Checkbox to show messages indefinitely has been removed and timeout field is now shown only if "Use timeout" option is selected.

Testing performed:

All tests passed.

Known issues with pull request:

None yet

Change log entry:

Changes
Rebuilt option to set if braille messages should be shown and when they should disappear

@feerrenrut
Copy link
Contributor

feerrenrut commented Sep 14, 2020

It may not be immediately obvious why this control is missing.

Instead, perhaps the following approach:

  • Use a comboBox "Show braille messages", with options: "Disable", "Use timeout", "Show indefinitely"
  • When "Use timeout" is selected, then the timeout control is enabled, otherwise it is disabled.

Also, please check the user guide and confirm that it describes these options well (or needs changes).

@dawidpieper
Copy link
Contributor Author

Thank you, I think it would be a good approach. I'll try to prepare it tomorrow.

@XLTechie

This comment has been minimized.

@dawidpieper

This comment has been minimized.

@feerrenrut

This comment has been minimized.

@dawidpieper dawidpieper force-pushed the hide_show_messages_indefinitely_checkbox_in_braille_settings_when_timeout_set_to_0 branch from c61c559 to d36e352 Compare September 16, 2020 17:36
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Generally looks good, thanks @dawidpieper

source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
@dawidpieper
Copy link
Contributor Author

dawidpieper commented Sep 22, 2020

I've harcoded 1 as a minimum value. I don't know what is the main policy for config specs - readability or backward compatibility. I can also change config specs and prepare upgrade as suggested, if it is the preferred way.

@feerrenrut feerrenrut self-assigned this Sep 23, 2020
@AppVeyorBot

This comment has been minimized.

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Thanks @dawidpieper

@feerrenrut feerrenrut merged commit dc9a654 into nvaccess:master Oct 6, 2020
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2020.4 milestone Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants