Skip to content

Toggle braille show selection indicator also with input gesture#14959

Merged
seanbudd merged 5 commits into
nvaccess:masterfrom
burmancomp:brailleToggleShowingSelection
Jun 9, 2023
Merged

Toggle braille show selection indicator also with input gesture#14959
seanbudd merged 5 commits into
nvaccess:masterfrom
burmancomp:brailleToggleShowingSelection

Conversation

@burmancomp

@burmancomp burmancomp commented Jun 4, 2023

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #14948

Summary of the issue:

Selection is indicated with dots 7 and 8 which may cause difficulties for reading.

Description of user facing changes

There is feature flag setting "Show selection" in Braille category which default behavior is Enabled. There is also possibility to use input gesture to cycle setting.

Description of development approach

Feature flag (BoolFlag) showSelection which default behavior is Enabled.

Testing strategy:

Tested with input gesture and from settings dialog.

Known issues with pull request:

Change log entries:

New features
Possibility to toggle braille show selection indicator from braille settings and with input gesture.
Changes
Bug fixes
For Developers

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.

@burmancomp burmancomp marked this pull request as ready for review June 4, 2023 21:50
@burmancomp burmancomp requested review from a team as code owners June 4, 2023 21:50
Comment thread source/braille.py
Comment thread user_docs/en/userGuide.t2t
Comment thread source/globalCommands.py Outdated
Comment thread source/globalCommands.py Outdated
Comment on lines +3309 to +3310
msg = _("Braille show selection")
msg += f" {self._featureFlagDefaultBehaviorDisplayString(displayString)}"

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.

This should be a single string so translators can re-arrange the parts.

Suggested change
msg = _("Braille show selection")
msg += f" {self._featureFlagDefaultBehaviorDisplayString(displayString)}"
msg = _("Braille show selection default (%s)") % featureFlag.behaviorOfDefault.displayString

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why not a format string?

Suggested change
msg = _("Braille show selection")
msg += f" {self._featureFlagDefaultBehaviorDisplayString(displayString)}"
msg = _("Braille show selection default ({})").format(featureFlag.behaviorOfDefault.displayString)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think that would be also possible. But why not "f style" with one msg assignment (asking this just for curiosity).

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.

gettext (the translation library) doesn't support f strings

Comment thread source/globalCommands.py Outdated
Comment thread source/globalCommands.py Outdated
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 60cfb82c35

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 57c64c59cb

@seanbudd seanbudd marked this pull request as draft June 5, 2023 23:50
@XLTechie

XLTechie commented Jun 6, 2023 via email

Copy link
Copy Markdown
Collaborator

@burmancomp burmancomp force-pushed the brailleToggleShowingSelection branch from 08f726a to c45a01b Compare June 6, 2023 14:26
@burmancomp

Copy link
Copy Markdown
Contributor Author

@burmancomp It appears that you may have pushed a bunch of unrelated commits from the master branch. Your last push had 15 commits, your total commits is now 18 I believe, and most of them are other people's work that was already merged.

This just shows that my understanding how different git commands work is very limited.

My solution to commit problem is not elegant one but hopefully it works at least this time.

@burmancomp burmancomp marked this pull request as ready for review June 7, 2023 07:53

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

Thanks @burmancomp

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

UserGuide reads well

@seanbudd seanbudd merged commit 9676495 into nvaccess:master Jun 9, 2023
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Jun 9, 2023
@burmancomp burmancomp deleted the brailleToggleShowingSelection branch June 9, 2023 07:13
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.

Braille: switch showing selection with dots 7 and 8 on/off

7 participants