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.

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.

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.

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

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

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.

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)

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

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.

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