Attachment patch-settings-pitch.patch added by ragb on 2012-03-23 17:55
Description:
Patch to present capPitchChange setting on the Voice settings dialog only when the synth supports the pitch setting.
Update:
Patch added from Trac
patch-settings-pitch.patch.txt

Comment 1 by briang1 on 2012-03-23 20:06
Hi, I've seen this sort of issue in other software though, the most common one is the realspeak voices. Maybe in these cases they do not set the pitch variable to off or something, who knows.

Changes:
Changed title from "The CapPitchChannge setting on the Voice Settings Dialog should only be presented if the synth supports the pitch setting" to "The CapPitchChange setting on the Voice Settings Dialog should only be presented if the synth supports the pitch setting"

Comment 2 by jteh on 2012-03-26 00:02
Some synths (e.g. sapi4) may support different settings for different voices, so we need to make sure this option gets hidden/displayed accordingly.

Comment 3 by ragb (in reply to comment 2) on 2012-03-26 03:21
Replying to jteh:

Some synths (e.g. sapi4) may support different settings for different voices, so we need to make sure this option gets hidden/displayed accordingly.

Sorry, Didn't know about that (synthDriverHandler.SynthDriver has no clear indication that this was possible and I haven't read all drivers...).

Shall I update the interface in gui.VoiceSettingsDialog.updateVoiceSettings instead of the approach present in my patch? It's an exceptional case and the less hugly approach would be an extra if there, I think.

Comment 4 by jteh (in reply to comment 3) on 2012-03-26 03:32
Replying to ragb:

Shall I update the interface in gui.VoiceSettingsDialog.updateVoiceSettings

That's what needs to be done, yes. It's a bit painful, though.

instead of the approach present in my patch?

Err, I somehow missed your patch. :) I've read it now and it was the same approach I thought of before I realised it wouldn't quite work.

It's an exceptional case and the less hugly approach would be an extra if there, I think.

I don't quite follow. It's pretty ugly, but it probably does need to be handled correctly.

Comment 5 by ragb (in reply to comment 4) on 2012-03-26 03:54
Replying to jteh:

That's what needs to be done, yes. It's a bit painful, though.

instead of the approach present in my patch?

Err, I somehow missed your patch. :) I've read it now and it was the same approach I thought of before I realised it wouldn't quite work.

Yes, it doesn't work at all.

It's an exceptional case and the less hugly approach would be an extra if there, I think.

I don't quite follow. It's pretty ugly, but it probably does need to be handled correctly.

I was refering to adding and extra if statement to the end of the updateVoiceSettings method to Show or Hide the capPitchChangeLabel and capPitchChangeEdit controls, if the synth supports pitch setting or not. But this is not as simple as it seems:
when the dialog is created, updateVoiceSettings is called before these controls are constructed so doesn't work. This can be done, with some repeated checks but is sort of hugly---- Will think abuot it.

@jcsteh Although the original author of this ticket cannot be contacted anymore, since you were interacting earlier, could you still please respond to #2195 (comment)?

There's not really anything to respond to. Some potential problems were noted, which I agree are certainly valid, but it's just a matter of refactoring things to get around that.