Allow setting braille display specific settings in the GUI by LeonarddeR · Pull Request #8214 · nvaccess/nvda

LeonarddeR · 2018-04-30T13:08:43Z

Though this pr is a fully working implementation, I"m open for any feedback and am willing to change things drastically if desired.

Link to issue number:

Closes #7452

Summary of the issue:

Speech synthesizers support changing synth specific properties (such as volume, inflection, rate boost) in the graphical user interface. Braille display drivers currently don't have this ability. This means that braille display specific settings, such as dot firmness and whether to enable braille input, can't be changed from the GUI.

Description of how this pull request fixes the issue:

Introduce a new driverHandler with a Driver class that contains all the functionality that has to be available both for synthesizer and braille display drivers. In the future, vision enhancement providers (such as magnifier, screen curtain and focus highlight) will also inherit from this class. Support for driver specific settings is now part of this class.
Introduce a DriverSettingsMixin class for settings panels to support driver specific GUI settings. Most if not all of the synthesizer specific settings logic has been moved to this mixin.
Added braille input and ATC toggles to the Handy Tech display driver as an example implementation. The eurobraille and ALVA drivers are not yet changed for this.
Renamed the synthSettingsRing module to settingsRing. The global settings ring will still be a synthesizer settings ring. However, global plugins can now implement their own ring for custom drivers, such as future magnifiers.

Testing performed:

Tested changing handy tech driver parameters through the gui. Also changed several synth parameters through the synth settings ring and gui, and checked that the gui successfully updated itself accordingly.

Known issues with pull request:

None

Change log entry:

New features
- For braille display drivers that support it, driver settings can now be changed from the braille settings category in NVDA's settings dialog. (Allow braille display specific settings in the GUI #7452)

LeonarddeR · 2018-04-30T13:12:01Z

Just to be clear, I don't at all expect this to be reviewed before 2018.2. Furthermore, I don't expect this to conflict with braille display auto detection in a major way.

@bramd: As I recall that you were particularly interested in this idea, I'd also welcome a quick review from you, if you have the time.

dkager · 2018-06-15T15:20:12Z

I think a braille input toggle should be added to all drivers that potentially support this (and then I wonder if it should actually be driver-specific). You could also allow setting the braille input table to "No input".

LeonarddeR · 2018-06-16T07:35:52Z

The problem with your idea is that it requires some significant changes in all InputGesture derivatives, and I'm afraid it is currently not possible to test them all. I can do this for Hims, but than, we need to agree on what key names to use.

dkager · 2018-06-16T10:49:47Z

So we can't receive input and ignore it just before sending it to liblouis? I agree that this would be ugly, but it would be a universal setting that way.

The other solution for the HumanWare and ALVA displays is to just disable the 8 braille keys.

LeonarddeR · 2018-06-20T06:01:02Z

I closed this by accidentally removing the branch. Reopening.

…ttings in the GUI

… conflicts caused by wx python phoenix.

feerrenrut · 2018-08-01T01:39:32Z

The PR comment mentions that this is not complete, could you elaborate on what is remaining?
Also, can you please update the changes.t2t file.

LeonarddeR · 2018-08-01T05:04:23Z

@feerrenrut commented on 1 aug. 2018 03:39 CEST:

The PR comment mentions that this is not complete, could you elaborate on what is remaining?

THis is actually no longer true, I updated the comment accordingly.

Also, can you please update the changes.t2t file.

I don't think that we should aim to have this in 2018.3. Therefore if you agree, I'd rather wait until there's some more clarity about when this is ready to merge. Note that, though the amount of changes is really huge from a line count perspective, it is basically mainly moving things around, renaming synth to driver, etc.

feerrenrut · 2018-08-01T06:00:32Z

Note that, though the amount of changes is really huge from a line count perspective, it is basically mainly moving things around.

Would you have time to rebase the changes so that moves and renames can be reviewed separately from the rest of the changes?

I don't think that we should aim to have this in 2018.3

I have started reviewing this, and will leave my comments. I'll probably need to review it again.

LeonarddeR · 2018-08-01T06:08:20Z

@feerrenrut commented on 1 aug. 2018 08:00 CEST:

Would you have time to rebase the changes so that moves and renames can be reviewed separately from the rest of the changes?

I just rebased on master.

feerrenrut · 2018-08-01T07:46:54Z

I just rebased on master.

Ah woops, not quite what I meant. I should have been more clear. I was hoping that when I do the final review of this, I can do it one commit at a time. First look at the moved code all in one commit, then a commit with the rename, then all other changes in final (or several other commits). They do not have to be in that order, but being able to isolate each of those groups really makes the process easier.

feerrenrut

Why is the settings ring limited to synth / braille driver settings? It might be a good moment to consider this, there have been issues in the past requesting that the options in the settings ring are configurable. I'm tempted to suggest that the settings ring changes be made separately from this PR. Being able to adjust braille driver settings from the NVDA braille settings dialog is a great start!

feerrenrut · 2018-08-01T02:29:56Z

 		_BgThread.queueApc(_BgThread.executor)

+	@classmethod
+	def DotFirmnessSetting(cls,defaultVal,minVal,maxVal):


I'd like to see this actually used (and tested) somewhere in this PR.

I added and tested this functionality for the handy tech driver.

Í'm not sure what to do with models that don't support this functionality. Settings are defined on the driver class, and we don't know whether a model supports a setting when at the class level.

feerrenrut · 2018-08-01T05:15:56Z

+				continue
+
+	@classmethod
+	def _paramToPercent(cls, current, min, max):


While it seems that these were not introduced here, I question whether this is where they belong. They are quite a bit more specific than the Driver class, perhaps in the NumericDriverSetting class instead?

The problem is that they are used within many synthesizer classes, moving them out of the SynthDriver or one of its parent classes would cause backwards incompatibility. Having said that, I can move them back to the SynthDriver class if you desire.

Why can't you do the same as you already have done, import it for backwards compatibility?

feerrenrut · 2018-08-01T06:38:17Z


+class EnhancedInputSlider(wx.Slider):
+	"""
+	A slider that supports additional key events, such as pageup/pageDown.


Can you explain why pageup/pagedown support is desirable?

It turns out that my assumptions where wrong. Page up and page down support is to move the slider in bigger steps, but this support was already there in wx.Slider. Honestly, I really don't know now what's the purpose of this class. 😖

This comment seems to have been removed, but the class is still there and still supports pageup/pagedown?

feerrenrut · 2018-08-01T06:39:10Z

 		super(SynthesizerSelectionDialog, self).onOk(evt)

-class SynthSettingChanger(object):
+class DriverSettingChanger(object):


How hard would it be to pull out the DriverSettingChanger class and related sub-classes into a separate or submodule?

feerrenrut · 2018-08-01T06:39:38Z

+		self.sizerDict={}
+		self.lastControl=None		
+		super(DriverSettingsMixin,self).__init__(*args,**kwargs)
+		self._curDriverRef = 	weakref.ref(self.driver)


Too many spaces after the equals sign.

feerrenrut · 2018-08-01T07:32:40Z

-		self.step = setting.normalStep if isinstance(setting,synthDriverHandler.NumericSynthSetting) else 1
+class DriverSetting(baseObject.AutoPropertyObject):
+	"""a numeric driver setting. Has functions to set, get, increase and decrease its value """
+	def __init__(self,driver,setting,min=0,max=100):


min, max only really make sense for numerical derivatives of this class. I'd prefer it was moved to the specialisation.

LeonarddeR · 2018-08-01T09:24:52Z

@feerrenrut commented on 1 aug. 2018 09:46 CEST:

I was hoping that when I do the final review of this, I can do it one commit at a time. First look at the moved code all in one commit, then a commit with the rename, then all other changes in final (or several other commits). They do not have to be in that order, but being able to isolate each of those groups really makes the process easier.

I agree that this should have been handy, however I'm afraid that it will be very hard to accomplish this.

I have a backup of the old branch without the rebase on master, would it be helpful for you if I'd restore that? Probably not as it will create outdated review comments.

feerrenrut · 2019-05-08T15:07:06Z

+		@rtype: l{bool}
+		"""
+		for s in self.supportedSettings:
+			if s.name==settingName: return True


How does this work with translations? We shouldn't be comparing translated strings.
Oh, I see there is also displayName. I would have prefered name be ID I think.
But then StringParameterInfo has ID and name where name is the displayable string.

Mm, yes, this is confusing. I'd be happy to change name to id, that's not a big deal. however, I want to use self.id, not self.ID. So I would like to propose making things uniform here. How about self.id and self.displayName for both settings and parameters?

How about self.id and self.displayName for both settings and parameters?

Yes, that is fine with me.

feerrenrut · 2019-05-08T15:08:38Z

+				continue
+
+	@classmethod
+	def _paramToPercent(cls, current, min, max):


Why can't you do the same as you already have done, import it for backwards compatibility?

feerrenrut · 2019-05-08T15:17:47Z


+		firstLoad = not config.conf["braille"].isSet(name)
+		if firstLoad:
+			config.conf["braille"][name] = {}


Is this resolved?

feerrenrut · 2019-05-08T15:29:07Z


+class EnhancedInputSlider(wx.Slider):
+	"""
+	A slider that supports additional key events, such as pageup/pageDown.


This comment seems to have been removed, but the class is still there and still supports pageup/pagedown?

feerrenrut · 2019-05-08T16:08:34Z

 		return checkbox

+	def updateDriverSettings(self, changedSetting=None):
+		"""Creates, hides or updates existing GUI controls for all of supported settings."""


I think this should be pulled out, right now it's hard to read. What about the driver provides a factory for constructing these. The common ones can be pulled out of here and provided for re-use.

For now, leave this as it is, in the interests of getting this merged.

LeonarddeR · 2019-05-08T16:38:13Z

I seem to be unable to reply on some of your inline comments, e.g. #8214 (comment). Do you have any idea why that could be the case?

As for #8214 (comment), I'm afraid I don't understand. What do you want me to import?

…re saving settings

LeonarddeR · 2019-05-09T08:03:50Z

@feerrenrut: I've had another look at this project ans stumbled upon some issues, which I've tried to solve. I'm sorry for the extra review work that causes this:

A driver now saves its configuration when terminating. When exiting NVDA, the config is saved before terminating braille and speech, so that's where the pre_configSave action is used for. We actually would need a pre_configProfileSwitch action as well to make sure that settings are saved when profile switching. For speech, this has never been a problem because either the settings dialog or the settings ring automatically saves its configuration, but for braille drivers, this is currently not the case. I've not yet added such an action.
The braille and synth handler loaded/saved their configuration based on whether the device was loaded for the first time. This is now still the case, but I abstracted this in a new initSettings method on drivers,. Now, the respective handler only has to call initSettings after the initialization has succeeded. Note that we can't call initSettings as part of Driver.init, as the settings system requires that the driver is fully initialized before loading/saving. initSetting also ensures that every driver has appropriate attributes for the settings system to work.
A major drawback of the current code is that SynthDriver still needs overrides for initSettings and loadSettings. I really want to abstract this better, but I have no clue as how to do this most effectively.

… horribly

feerrenrut · 2019-05-09T11:10:50Z

seem to be unable to reply on some of your inline comments,

I think this is because they are on "outdated" lines. That's kind of annoying.

I'm afraid I don't understand. What do you want me to import?

I was trying to continue a thread that started in #8214 (comment) . These methods were moved from synthDriverHandler . Rather than put them in driverHandler it actually looks like they don't need to be class variables. I think I got confused when I referred to the import, ignore that part. Again, splitting this up might be work that could happen another time. Overall, I'm trying to reduce the size and complexity of modules in NVDA.

feerrenrut · 2019-05-09T11:54:25Z

I've had another look at this project ans stumbled upon some issues

Could you first explain what these issues were?

LeonarddeR · 2019-05-09T12:43:18Z

I've had another look at this project ans stumbled upon some issues

Could you first explain what these issues were?

Sure.

As for point 1, the issue popped up when using a handy tech display which supports the braille input setting. This driver contains a script to toggle braille input. However, when changing the state of the parameter using the script, the new state was never saved in the configuration. This is now solved by saving settings on terminating and on pre-saving configuration.

I also mentioned the issue related to config profile switches above. My second point wasn't strictly spoken because of an issue, but to avoid duplicated code, especially when the vision framework is also going to use the driverHandler.

feerrenrut · 2019-05-13T16:03:06Z

If you are happy with the testing on this, can you merge in master? I'll look to merge this tomorrow.

… conflict

LeonarddeR · 2019-05-13T17:13:39Z

Done. Thanks for the reminder!

…(PR #9587) * Fix overlooked rename of ID to id in synthDriverHandler.LanguageInfo * Original PR #8214

LeonarddeR requested a review from feerrenrut April 30, 2018 13:09

LeonarddeR added component/speech-synth-drivers component/NVDA-GUI component/braille labels Apr 30, 2018

LeonarddeR closed this Jun 20, 2018

LeonarddeR deleted the i7452 branch June 20, 2018 05:00

LeonarddeR restored the i7452 branch June 20, 2018 05:00

LeonarddeR reopened this Jun 20, 2018

Leonard de Ruijter added 9 commits June 28, 2018 15:36

Create abstract driver class and allow the use of braille specific se…

bb4346d

…ttings in the GUI

Add HID input setting to ALVA and eurobraille drivers

8e8ce01

Synthesiser>synthesizer

bd486ea

Fix translator comment

ba5dbfd

Merge remote-tracking branch 'origin/master' into i7452, fixing merge…

94cb69a

… conflicts caused by wx python phoenix.

Fix NVDAControls change for WXPy4

d71629e

Gui improvements

fc75265

Merge remote-tracking branch 'origin/master' into i7452

4a120fc

Abstract the settings ring

c1e766d

feerrenrut reviewed Aug 1, 2018

View reviewed changes

feerrenrut self-requested a review August 6, 2018 08:56

feerrenrut reviewed May 8, 2019

View reviewed changes

feerrenrut suggested changes May 8, 2019

View reviewed changes

Leonard de Ruijter added 2 commits May 8, 2019 18:18

Update doc strings

459ecb2

Review action, also split some other long lines

ac945b6

Leonard de Ruijter added 4 commits May 8, 2019 19:14

braille, make sure that settings are always reloaded on re-init

7eb4d0e

Add logging when loading and saving settings

6824e02

Make sure that a driver writes its settings to the configuration befo…

edd2ee5

…re saving settings

Abstract settings initialisation

0f7565f

Leonard de Ruijter added 3 commits May 9, 2019 10:31

Update synthDriverHandler to set attributes when they are not available

bd139ed

Allow setting default values for string settings

1e9fc3b

Make sure that on first load, the config spec population doesn't fail…

95576a1

… horribly

Rename .ID to .id, .name to .id, .name to .displayName

3fe5f33

Merge remote-tracking branch 'origin/master' into i7452, fixing merge…

a430ed7

… conflict

feerrenrut approved these changes May 16, 2019

View reviewed changes

feerrenrut merged commit 4832a8e into nvaccess:master May 16, 2019

nvaccessAuto added this to the 2019.2 milestone May 16, 2019

feerrenrut added a commit that referenced this pull request May 16, 2019

Update changes file for PR #8214

7e26d87

feerrenrut pushed a commit that referenced this pull request May 20, 2019

Fix overlooked rename of ID to id in synthDriverHandler.LanguageInfo …

ba1c46c

…(PR #9587) * Fix overlooked rename of ID to id in synthDriverHandler.LanguageInfo * Original PR #8214

LeonarddeR mentioned this pull request May 21, 2019

Remove functionality marked as deprecated #9603

Merged

michaelDCurran mentioned this pull request Jun 27, 2019

Removal of VoiceSettingsSlider breaks AudioThemes3d add-on #9824

Closed

LeonarddeR mentioned this pull request Jun 27, 2019

Restore VoiceSettingsSlider to ensure we don't break compatibility with Audio Themes add-on #9826

Merged

LeonarddeR mentioned this pull request Jul 10, 2019

Threshold: Revert "Restore VoiceSettingsSlider to ensure we don't break compatibility with Audio Themes add-on (#9826)" #9907

Merged

LeonarddeR added the BabbageWork Pull requests filed on behalf of Babbage B.V. label Oct 11, 2019

Uh oh!

Conversation

LeonarddeR commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

LeonarddeR commented Apr 30, 2018

Uh oh!

dkager commented Jun 15, 2018

Uh oh!

LeonarddeR commented Jun 16, 2018

Uh oh!

dkager commented Jun 16, 2018

Uh oh!

LeonarddeR commented Jun 20, 2018

Uh oh!

feerrenrut commented Aug 1, 2018

Uh oh!

LeonarddeR commented Aug 1, 2018

Uh oh!

feerrenrut commented Aug 1, 2018

Uh oh!

LeonarddeR commented Aug 1, 2018

Uh oh!

feerrenrut commented Aug 1, 2018

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR commented May 8, 2019

Uh oh!

LeonarddeR commented May 9, 2019

Uh oh!

feerrenrut commented May 9, 2019

Uh oh!

LeonarddeR commented Apr 30, 2018 •

edited

Loading