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

Add Keyboard layout setting to the Welcome dialog #6868

Merged
merged 3 commits into from May 30, 2017

Conversation

dkager
Copy link
Collaborator

@dkager dkager commented Feb 10, 2017

For this addition I didn't rewrite the dialog to use guiHelper. As a consequence I'm not sure if it works out visually.
Fixes #6863.

Also:
* Remove empty line at the end of the detail message.
* Clean up translator comments.
* Add mnemonics to the settings controls.
Todo:
* Use guiHelper.
* Bring Keyboard settings dialog in line, i.e. add mnemonics there too.
@feerrenrut
Copy link
Contributor

I had a look at this to check the visual layout. Currently there is no gap (horizontally) between the label and wx.Choice. You can add a spacer for this. See guiHelper.py line 118. Also the vertical alignment of the choice control is wrong. It needs to be centred vertically. I think that this would be a good opportunity to convert this dialog to use the guiHelper.

@dkager
Copy link
Collaborator Author

dkager commented Feb 10, 2017

Thank you. I'll move to guiHelper as part of this PR then.

@dkager
Copy link
Collaborator Author

dkager commented Feb 10, 2017

Just sprinkled some guiHelper on this dialog.

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.

Looks good. I have built this locally to visually inspect and ensure the spacing and alignment is ok.

index = self.kbdNames.index(config.conf["keyboard"]["keyboardLayout"])
self.kbdList.SetSelection(index)
except:
log.debugWarning("Could not set Keyboard layout list to current layout",exc_info=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

log.debugWarning does not produce a sound in NVDA. If this is hit I would suggest that it is a serious enough error that users should be notified. I would propose changing this to log.error

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated my comment, debugWarning never produces a sound. error sounds are purposefully omitted from the release.

@dkager
Copy link
Collaborator Author

dkager commented Apr 25, 2017

I copied the keyboard code from elsewhere. Arguably the welcome experience should be errorless from the user's perspective, even if under the hood everything's on fire. :)

@feerrenrut
Copy link
Contributor

Understood, this will be the case in a release. However pre-release we want to know about problems as early as possible to have a chance to fix them prior to the release and RC versions.

@dkager
Copy link
Collaborator Author

dkager commented Apr 26, 2017

Do you suppose this also goes for the same log message when setting the layout from the settings dialog? That's where the code came from.

@dkager
Copy link
Collaborator Author

dkager commented Apr 26, 2017

Made it an error for the welcome dialog. Didn't touch the settings dialog though.

feerrenrut added a commit that referenced this pull request Apr 27, 2017
Re issue: #6863
Merge branch 'dkager-i6863' into next
@feerrenrut feerrenrut merged commit 8737fa3 into nvaccess:master May 30, 2017
@nvaccessAuto nvaccessAuto added this to the 2017.3 milestone May 30, 2017
feerrenrut added a commit that referenced this pull request May 30, 2017
- PR #7169 : Editable div elements in Chrome are no longer have their label reported as their value while in browse mode. (Issue #7153)
- PR #6396 : An unbound gesture (script_restart) has been added to allow NVDA to be restarted quickly. (PR #6396)
- PR #6777 : A Braille setting has been added to "show messages indefinitely". (Issue #6669)
- PR #7133 : Pressing end while in browse mode of an empty Microsoft Word document no longer causes a runtime error. (Issue #7009)
- PR #6868 : The keyboard layout can now be set from the NVDA Welcome dialog. (Issue #6863)
- PR #6813 : The names of "landmarks" are abbreviated in Braille (Issue #3975)
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

3 participants