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

Allow setting braille display specific settings in the GUI #8214

Merged
merged 39 commits into from May 16, 2019

Conversation

leonardder
Copy link
Collaborator

@leonardder leonardder commented Apr 30, 2018

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:

  1. 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.
  2. 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.
  3. 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.
  4. 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. (#7452)

@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Apr 30, 2018

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

@dkager dkager commented Jun 15, 2018

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
Copy link
Collaborator Author

@leonardder leonardder commented Jun 16, 2018

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

@dkager dkager commented Jun 16, 2018

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 leonardder closed this Jun 20, 2018
@leonardder leonardder deleted the i7452 branch Jun 20, 2018
@leonardder leonardder restored the i7452 branch Jun 20, 2018
@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Jun 20, 2018

I closed this by accidentally removing the branch. Reopening.

@leonardder leonardder reopened this Jun 20, 2018
@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Aug 1, 2018

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
Copy link
Collaborator Author

@leonardder leonardder commented Aug 1, 2018

@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
Copy link
Member

@feerrenrut feerrenrut commented Aug 1, 2018

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
Copy link
Collaborator Author

@leonardder leonardder commented Aug 1, 2018

@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
Copy link
Member

@feerrenrut feerrenrut commented Aug 1, 2018

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.

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

@feerrenrut feerrenrut May 8, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@leonardder leonardder May 8, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Member

@feerrenrut feerrenrut May 9, 2019

Choose a reason for hiding this comment

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

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

Yes, that is fine with me.

continue

@classmethod
def _paramToPercent(cls, current, min, max):
Copy link
Member

@feerrenrut feerrenrut May 8, 2019

Choose a reason for hiding this comment

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

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

source/driverHandler.py Show resolved Hide resolved
@@ -1622,14 +1623,14 @@ def setDisplayByName(self, name, isFallback=False, detected=None):
elif not isFallback and not detected:
self._disableDetection()

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

@feerrenrut feerrenrut May 8, 2019

Choose a reason for hiding this comment

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

Is this resolved?

@@ -37,3 +38,41 @@ def OnSetFocus(self, evt):
self.SetSelection(0, numChars)
evt.Skip()

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

@feerrenrut feerrenrut May 8, 2019

Choose a reason for hiding this comment

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

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

source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
if self.lastControl:
checkbox.MoveAfterInTabOrder(self.lastControl)
self.lastControl=checkbox
return checkbox

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

@feerrenrut feerrenrut May 8, 2019

Choose a reason for hiding this comment

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

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
Copy link
Collaborator Author

@leonardder leonardder commented May 8, 2019

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?

@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented May 9, 2019

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

  1. 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.
  2. 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.

@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented May 9, 2019

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

@feerrenrut feerrenrut commented May 9, 2019

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

Could you first explain what these issues were?

@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented May 9, 2019

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

@feerrenrut feerrenrut commented May 13, 2019

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

@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented May 13, 2019

@feerrenrut feerrenrut merged commit 4832a8e into nvaccess:master May 16, 2019
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2019.2 milestone May 16, 2019
feerrenrut added a commit that referenced this issue May 16, 2019
feerrenrut pushed a commit that referenced this issue May 20, 2019
…(PR #9587)

* Fix overlooked rename of ID to id in synthDriverHandler.LanguageInfo
* Original PR #8214
@leonardder leonardder added the BabbageWork label Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BabbageWork component/NVDA-GUI component/speech-synth-drivers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants