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

Synth handler/settings ring: look up normalized names of common voice settings to help with translation contexts #5185

Closed
nvaccessAuto opened this Issue Jun 27, 2015 · 10 comments

Comments

Projects
None yet
1 participant
@nvaccessAuto

nvaccessAuto commented Jun 27, 2015

Reported by nvdakor on 2015-06-27 03:20
Hi,
Spin-off from #5100:
Consider the following scenario: a user uses NvDA in a language other than English, such as Japanese, and uses synthe settings ring commands to manipulate voice settings. Normally, the user would hear only the translated portion of the message, but for some languages, the accelerator used in voice settings dialog is also spoken.

Problem: This is because commonly manipulated settings are stored in synth driver handler, and the translatable strings in there are also used in voice settings dialog.

Proposed solution: Modify i18nName function as follows:

  1. Look up a dictionary which maps currently used translatable string to normalized string suitable for output when changing the setting via synth settings ring.
  2. If possible, for synthesizers which exposes additional configuration options via voice settings, let them return a normalized string for synth settings ring.

Impact and use cases:

  • Translators can translate a given setting differently - one for voice setting and synth settings rint.
  • Allows Gettext contexts to be applied so the translators can understand where the message will be used.
  • Allows synth driver developers to provide different messages for use in GUI and synth settings ring.

Additional comments are appreciated. Thanks.

Blocking #5100

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jun 27, 2015

Comment 1 by nvdakor on 2015-06-27 04:18
Hi,
An alternate proposal, which may affect existing synthesizers: What if we add a new attribute in synthDriverHandler.SynthSetting that'll store label used in voice settings dialog? Besides, i18nName sounds misleading (at least to me).
Thanks.

nvaccessAuto commented Jun 27, 2015

Comment 1 by nvdakor on 2015-06-27 04:18
Hi,
An alternate proposal, which may affect existing synthesizers: What if we add a new attribute in synthDriverHandler.SynthSetting that'll store label used in voice settings dialog? Besides, i18nName sounds misleading (at least to me).
Thanks.

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jun 29, 2015

Comment 2 by jteh on 2015-06-29 00:59
I assume this is necessary because some languages include the accelerator after the actual string rather than as part of the string; e.g. "Pitch (&x)"?

As you suggest, the best solution is to add a new attribute to SynthSetting. I guess we can have displayName and displayNameWithAccelerator, where the latter is used for the GUI and contains the accelerator. (The former shouldn't specifically mention the ring, as it might be used for other things in future where accelerators aren't wanted.) i18nName could be mapped to displayNameWithAccelerator for backwards compatibility. Unfortunately, this will require most translators to do a lot of unnecessary translation, but I don't see a way around this.

nvaccessAuto commented Jun 29, 2015

Comment 2 by jteh on 2015-06-29 00:59
I assume this is necessary because some languages include the accelerator after the actual string rather than as part of the string; e.g. "Pitch (&x)"?

As you suggest, the best solution is to add a new attribute to SynthSetting. I guess we can have displayName and displayNameWithAccelerator, where the latter is used for the GUI and contains the accelerator. (The former shouldn't specifically mention the ring, as it might be used for other things in future where accelerators aren't wanted.) i18nName could be mapped to displayNameWithAccelerator for backwards compatibility. Unfortunately, this will require most translators to do a lot of unnecessary translation, but I don't see a way around this.

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jun 29, 2015

Comment 3 by nvdakor on 2015-06-29 01:20
Hi,
Sounds fine with me (although we may need to ask synth driver developers if they are willing to modify their drivers). And yes, this is in response to the case where a language includes accelerators (mostly Chinese, Japanese and Korean, but I'm sure there are other languages with this problem).
In terms of looking up the synth settings ring label, the procedure I'm thinking of is:

  1. First, see if displayName is available.
  2. Fall back to current method used (stripping ampersand) if displayName is not found (displayName itself will be None unless specified by the synthesizer when synth setting is constructed; for Espeak, this can be done easily, as it uses only common settings such as volume and pitch).
    Thanks.

nvaccessAuto commented Jun 29, 2015

Comment 3 by nvdakor on 2015-06-29 01:20
Hi,
Sounds fine with me (although we may need to ask synth driver developers if they are willing to modify their drivers). And yes, this is in response to the case where a language includes accelerators (mostly Chinese, Japanese and Korean, but I'm sure there are other languages with this problem).
In terms of looking up the synth settings ring label, the procedure I'm thinking of is:

  1. First, see if displayName is available.
  2. Fall back to current method used (stripping ampersand) if displayName is not found (displayName itself will be None unless specified by the synthesizer when synth setting is constructed; for Espeak, this can be done easily, as it uses only common settings such as volume and pitch).
    Thanks.
@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jun 29, 2015

Comment 4 by Joseph Lee <joseph.lee22590@... on 2015-06-29 02:15
In [f1aa79e]:

Synth driver andler and settings ring: Provide correct label in specific situations to help translators and to add Gettext contexts. re #5185

Synth Driver Handler's settings factory function gains a new attribute. This is really i18nName split into two attributes: displayNameWithAccelerator, used in voice settings dialog and other GUI prompts, and displayName for use in synth settings ring. This is needed to let translators translate settings labels using appropriate wording in specific contexts and to add Gettext contexts.
In synth settings ring, when returing the label for a synth setting, first consult displayName before resorting to removing ampersand from i18nName. i18nName will be kept for backward compatibility (setting.i18nName is synomous with setting.displayName).

nvaccessAuto commented Jun 29, 2015

Comment 4 by Joseph Lee <joseph.lee22590@... on 2015-06-29 02:15
In [f1aa79e]:

Synth driver andler and settings ring: Provide correct label in specific situations to help translators and to add Gettext contexts. re #5185

Synth Driver Handler's settings factory function gains a new attribute. This is really i18nName split into two attributes: displayNameWithAccelerator, used in voice settings dialog and other GUI prompts, and displayName for use in synth settings ring. This is needed to let translators translate settings labels using appropriate wording in specific contexts and to add Gettext contexts.
In synth settings ring, when returing the label for a synth setting, first consult displayName before resorting to removing ampersand from i18nName. i18nName will be kept for backward compatibility (setting.i18nName is synomous with setting.displayName).

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jul 3, 2015

Comment 6 by jteh on 2015-07-03 07:35
Thanks! Some code review:

In SynthSetting:

+ def __init__(self,name,displayNameWithAccelerator,displayName=None,availableInSynthSettingsRing=True):
For public APIs, new keyword arguments should generally be added after existing ones because of the Python 2 behaviour of allowing positional arguments to be used for keyword arguments. For example, if an old driver called SynthSetting("blah", _("blah"), True), True would end up being displayName instead of availableInSynthSettingsRing. This also applies to NumericSynthSetting etc.

+ self.i18nName=displayNameWithAccelerator

Please put a code doc comment on the line above this stating that it's deprecated. Something like this will do:
#: @deprecated: Use L{displaynameWithAccelerator} and L{displayName} instead.

In NumericSynthSetting.__init__:

+ super(NumericSynthSetting,self).__init__(name,displayNameWithAccelerator,displayName,availableInSynthSettingsRing)

It's best to only pass keyword arguments as keyword arguments; e.g. displayName=displayName. This avoids the confusion as to which positional argument will get mapped to what keyword arguments (see above). I realise this wasn't done for availableInSynthSettingsRing before, but it's probably worth doing it since you're changing the code. This also applies to other subclasses.

You use "synth settings ring" as the gettext message context. While I think it's okay to have the translators comments say that (since it's only currently used for the ring), it could also be conceivably used in other places where we want to refer to synth settings without the accelerator. Therefore, I'd prefer this be "synth setting" or "synth setting without accelerator" or similar.

If displayName is None, you check for this in the settings ring code and do the replacement there. I wonder whether we should instead do this in the SynthSetting constructor. Thoughts?

Thanks again.
Changes:
Milestone changed from None to 2015.3

nvaccessAuto commented Jul 3, 2015

Comment 6 by jteh on 2015-07-03 07:35
Thanks! Some code review:

In SynthSetting:

+ def __init__(self,name,displayNameWithAccelerator,displayName=None,availableInSynthSettingsRing=True):
For public APIs, new keyword arguments should generally be added after existing ones because of the Python 2 behaviour of allowing positional arguments to be used for keyword arguments. For example, if an old driver called SynthSetting("blah", _("blah"), True), True would end up being displayName instead of availableInSynthSettingsRing. This also applies to NumericSynthSetting etc.

+ self.i18nName=displayNameWithAccelerator

Please put a code doc comment on the line above this stating that it's deprecated. Something like this will do:
#: @deprecated: Use L{displaynameWithAccelerator} and L{displayName} instead.

In NumericSynthSetting.__init__:

+ super(NumericSynthSetting,self).__init__(name,displayNameWithAccelerator,displayName,availableInSynthSettingsRing)

It's best to only pass keyword arguments as keyword arguments; e.g. displayName=displayName. This avoids the confusion as to which positional argument will get mapped to what keyword arguments (see above). I realise this wasn't done for availableInSynthSettingsRing before, but it's probably worth doing it since you're changing the code. This also applies to other subclasses.

You use "synth settings ring" as the gettext message context. While I think it's okay to have the translators comments say that (since it's only currently used for the ring), it could also be conceivably used in other places where we want to refer to synth settings without the accelerator. Therefore, I'd prefer this be "synth setting" or "synth setting without accelerator" or similar.

If displayName is None, you check for this in the settings ring code and do the replacement there. I wonder whether we should instead do this in the SynthSetting constructor. Thoughts?

Thanks again.
Changes:
Milestone changed from None to 2015.3

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jul 3, 2015

Comment 7 by nvdakor (in reply to comment 6) on 2015-07-03 07:45
Replying to jteh:

In SynthSetting:

+ def __init__(self,name,displayNameWithAccelerator,displayName=None,availableInSynthSettingsRing=True):
For public APIs, new keyword arguments should generally be added after existing ones because of the Python 2 behaviour of allowing positional arguments to be used for keyword arguments. For example, if an old driver called SynthSetting("blah", _("blah"), True), True would end up being displayName instead of availableInSynthSettingsRing. This also applies to NumericSynthSetting etc.
Didn't know this before. Thanks for this info (does Python 3 retain this behavior?).
+ self.i18nName=displayNameWithAccelerator

Please put a code doc comment on the line above this stating that it's deprecated. Something like this will do:

#: @deprecated: Use L{displaynameWithAccelerator} and L{displayName} instead.
Will do so in the next commit.
In NumericSynthSetting.__init__:

+ super(NumericSynthSetting,self).__init__(name,displayNameWithAccelerator,displayName,availableInSynthSettingsRing)

It's best to only pass keyword arguments as keyword arguments; e.g. displayName=displayName. This avoids the confusion as to which positional argument will get mapped to what keyword arguments (see above). I realise this wasn't done for availableInSynthSettingsRing before, but it's probably worth doing it since you're changing the code. This also applies to other subclasses.

Sure.

You use "synth settings ring" as the gettext message context. While I think it's okay to have the translators comments say that (since it's only currently used for the ring), it could also be conceivably used in other places where we want to refer to synth settings without the accelerator. Therefore, I'd prefer this be "synth setting" or "synth setting without accelerator" or similar.

I wrote it like that because of the scope of this ticket, but I do understand that this could be used in other situations, hence will change it.

If displayName is None, you check for this in the settings ring code and do the replacement there. I wonder whether we should instead do this in the SynthSetting constructor. Thoughts?

I don't know about that (you know more about this than I do).
Thanks for the review.

nvaccessAuto commented Jul 3, 2015

Comment 7 by nvdakor (in reply to comment 6) on 2015-07-03 07:45
Replying to jteh:

In SynthSetting:

+ def __init__(self,name,displayNameWithAccelerator,displayName=None,availableInSynthSettingsRing=True):
For public APIs, new keyword arguments should generally be added after existing ones because of the Python 2 behaviour of allowing positional arguments to be used for keyword arguments. For example, if an old driver called SynthSetting("blah", _("blah"), True), True would end up being displayName instead of availableInSynthSettingsRing. This also applies to NumericSynthSetting etc.
Didn't know this before. Thanks for this info (does Python 3 retain this behavior?).
+ self.i18nName=displayNameWithAccelerator

Please put a code doc comment on the line above this stating that it's deprecated. Something like this will do:

#: @deprecated: Use L{displaynameWithAccelerator} and L{displayName} instead.
Will do so in the next commit.
In NumericSynthSetting.__init__:

+ super(NumericSynthSetting,self).__init__(name,displayNameWithAccelerator,displayName,availableInSynthSettingsRing)

It's best to only pass keyword arguments as keyword arguments; e.g. displayName=displayName. This avoids the confusion as to which positional argument will get mapped to what keyword arguments (see above). I realise this wasn't done for availableInSynthSettingsRing before, but it's probably worth doing it since you're changing the code. This also applies to other subclasses.

Sure.

You use "synth settings ring" as the gettext message context. While I think it's okay to have the translators comments say that (since it's only currently used for the ring), it could also be conceivably used in other places where we want to refer to synth settings without the accelerator. Therefore, I'd prefer this be "synth setting" or "synth setting without accelerator" or similar.

I wrote it like that because of the scope of this ticket, but I do understand that this could be used in other situations, hence will change it.

If displayName is None, you check for this in the settings ring code and do the replacement there. I wonder whether we should instead do this in the SynthSetting constructor. Thoughts?

I don't know about that (you know more about this than I do).
Thanks for the review.

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jul 3, 2015

Comment 8 by Joseph Lee <joseph.lee22590@... on 2015-07-03 08:01
In [6544943]:

Synth driver handler: reordered keyword arguments and clarified Gettext context. re #5185

Code review by: James Teh (jamie@nvaccess.org)

nvaccessAuto commented Jul 3, 2015

Comment 8 by Joseph Lee <joseph.lee22590@... on 2015-07-03 08:01
In [6544943]:

Synth driver handler: reordered keyword arguments and clarified Gettext context. re #5185

Code review by: James Teh (jamie@nvaccess.org)

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jul 9, 2015

Comment 9 by James Teh <jamie@... on 2015-07-09 07:46
In [ebee25a]:

Merge branch 't5185' into next

Incubates #5185.

Changes:
Added labels: incubating

nvaccessAuto commented Jul 9, 2015

Comment 9 by James Teh <jamie@... on 2015-07-09 07:46
In [ebee25a]:

Merge branch 't5185' into next

Incubates #5185.

Changes:
Added labels: incubating

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jul 9, 2015

Comment 10 by jteh on 2015-07-09 07:48
Thanks. I made one minor change before incubating.

Note for merge to master: Squash this to one commit. Add entry in Changes for Developers.

nvaccessAuto commented Jul 9, 2015

Comment 10 by jteh on 2015-07-09 07:48
Thanks. I made one minor change before incubating.

Note for merge to master: Squash this to one commit. Add entry in Changes for Developers.

@nvaccessAuto

This comment has been minimized.

Show comment
Hide comment
@nvaccessAuto

nvaccessAuto Jul 24, 2015

Comment 11 by James Teh <jamie@... on 2015-07-24 06:43
In [27f96bd]:

Rather than a single i18nName attribute, synthDriverHandler.SynthSetting now has separate displayNameWithAccelerator and displayName attributes to avoid reporting of the accelerator in the synth settings ring in some languages.

Fixes #5185.

Changes:
Removed labels: incubating
State: closed

nvaccessAuto commented Jul 24, 2015

Comment 11 by James Teh <jamie@... on 2015-07-24 06:43
In [27f96bd]:

Rather than a single i18nName attribute, synthDriverHandler.SynthSetting now has separate displayNameWithAccelerator and displayName attributes to avoid reporting of the accelerator in the synth settings ring in some languages.

Fixes #5185.

Changes:
Removed labels: incubating
State: closed

@nvaccessAuto nvaccessAuto added this to the 2015.3 milestone Nov 10, 2015

josephsl added a commit that referenced this issue Nov 23, 2015

Rather than a single i18nName attribute, synthDriverHandler.SynthSett…
…ing now has separate displayNameWithAccelerator and displayName attributes to avoid reporting of the accelerator in the synth settings ring in some languages.

Fixes #5185.

feerrenrut added a commit that referenced this issue Apr 4, 2017

Synth driver handler: remove deprecated i18n name attribute. (PR #6877)
partial fix for #6846, and #5185

Removed deprecated `i18nName` for synth settings, replaced by `displayName` and `displayNameWithAccelerator`.

feerrenrut added a commit that referenced this issue Apr 4, 2017

Update changes file for PR: #6878, #6875, #6877, #6876, #6914, #6987
- Deprecated code removed:
 - PR #6878: `speech.REASON_*` constants, `controlTypes.REASON_*` should be used
   instead. (issue #6846)
 - PR #6877: `i18nName` for synth settings, `displayName` and
   `displayNameWithAccelerator` should be used instead. (issues #6846, #5185)
 - PR #6876: `config.validateConfig`. (issues #6846, #667)
 - PR #6875: `config.save`. (issue #6846)

- PR #6914: Support Windows Calculator on Windows 10 Enterprise LTSB (Long-Term
  Servicing Branch) and Server. (issue #6914)
- PR #6987: Reduced the chance of configuration file being corrupted when
  Windows shuts down. Configuration files are now written to a temporary
  file before replacing the actual configuration file. (issue #3165)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment