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

Code cleanup: officially remove deprecated code paths #6846

Open
josephsl opened this Issue Feb 5, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@josephsl
Collaborator

josephsl commented Feb 5, 2017

Hi,

This was brought up on NvDA development and add-ons lists:

There are code paths/fragments that are marked as deprecated and to be removed at a future date. I believe this year would be a good time to do it.

The deprecated code paths include:

  • synthSettingsRing.i18nName (#5185)
  • config.validate (#5932)
  • speech.reason constants (imported directly from control types now)
  • synthDriverHandler.SynthDriver: speakText and speakCharacter methods.
  • config.save
  • Others.

Thanks.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Feb 5, 2017

Contributor

Happy enough for this to be done, but you'd need to:

  1. Check that this stuff isn't accidentally used anywhere in core (and explain how you did it).
  2. Test (and document how you tested) that this doesn't have undesirable effects on affected code paths. For example, if you remove config.save, you need to be sure that saving configuration still works.
  3. Check commonly used synth drivers (including Pico and Festival) to see if deprecated functions aren't used there. We have no interest in updating these ourselves, but I believe they're still used quite a bit. If they do use deprecated functions, you'd need to be willing to update these.
  4. Be willing to deal with any regressions that arise. If a regression occurs and it doesn't get fixed within a few days, we will most likely back out the change and close the PR, since that's the simplest path to resolving the issue.

Thanks.

Contributor

jcsteh commented Feb 5, 2017

Happy enough for this to be done, but you'd need to:

  1. Check that this stuff isn't accidentally used anywhere in core (and explain how you did it).
  2. Test (and document how you tested) that this doesn't have undesirable effects on affected code paths. For example, if you remove config.save, you need to be sure that saving configuration still works.
  3. Check commonly used synth drivers (including Pico and Festival) to see if deprecated functions aren't used there. We have no interest in updating these ourselves, but I believe they're still used quite a bit. If they do use deprecated functions, you'd need to be willing to update these.
  4. Be willing to deal with any regressions that arise. If a regression occurs and it doesn't get fixed within a few days, we will most likely back out the change and close the PR, since that's the simplest path to resolving the issue.

Thanks.

@jcsteh jcsteh added the p4 label Feb 5, 2017

@josephsl

This comment has been minimized.

Show comment
Hide comment
@josephsl

josephsl Feb 6, 2017

Collaborator
Collaborator

josephsl commented Feb 6, 2017

josephsl added a commit to josephsl/nvda that referenced this issue Feb 6, 2017

Removed config.validateConfig, config.conf.validator now uses Validat…
…or constructor. re nvaccess#6846, nvaccess#667.

As there's no need to store a module-level validator, validator class instanciation will be done from config.ConfigManager constructor.
@josephsl

This comment has been minimized.

Show comment
Hide comment
@josephsl

josephsl Feb 6, 2017

Collaborator

Hi,

The following describes the current situation with various deprecated code paths. For each one, the following procedure was used:

  1. Switch to the NVDA Core source directory.
  2. Run grep with the following parameters: grep -lr expression
  3. For ease of reading, the output was piped to a text file for examination later.
  4. For each code path found, the context was looked at (function level, surrounding lines, etc.). For some, git log was used to see the history of the file in question and where the line came from.
  5. Ran NVDA from source code prior to and after removing the deprecated code paths to make sure regressions didn't occur (so far, none).

Status of removed code paths:

config.save:

  • Not used by anything in the NVDA Core source.
  • Already calls config.conf.save, and likewise other modules call config.conf.save instead of config.save.
  • Add-ons that weren't updated to deal with config.conf may need to be updated.
  • Core regression: none.

config.validateConfig:

  • config.ConfigManager does not call this function anymore, nor does other modules.
  • config.val was also removed, as config.conf.validator will now fetch validator directly from Validator constructor.
  • Add-ons that uses config.validateConfig will need to be updated to provide their own implementations.
  • Regressions: none.

SynthSetting.i18nName:

  • Not used by NVDA Core modules.
  • displayNameWithaccelerator replaces this attribute.
  • Speech synthesizer drivers that uses i18n must provide displayNameWithAccelerator.
  • Regressions: none.

speech.reason constants:

  • No module in NVDA Core uses this.
  • At least one add-on still relies on this, and fixing it is trivial.
  • Replaced by reason constants from control types.
  • Regressions: none.

SynthDriver.speakText/speakCharacter:

  • Should be handled by SynthDriver.speak.
  • NVDA Core falls back to speakText and speakCharacter.
  • speakText raises not implemented error, while speakCharacter is still implemented.
  • Audiologic driver still implements speakText.
  • Regressions: potentially major.

In case of the last one, I'd like to hear what Audiologic developers would have to say before proceeding, and for speech.reason constants, add-on authors will be notified.

Thanks.

Collaborator

josephsl commented Feb 6, 2017

Hi,

The following describes the current situation with various deprecated code paths. For each one, the following procedure was used:

  1. Switch to the NVDA Core source directory.
  2. Run grep with the following parameters: grep -lr expression
  3. For ease of reading, the output was piped to a text file for examination later.
  4. For each code path found, the context was looked at (function level, surrounding lines, etc.). For some, git log was used to see the history of the file in question and where the line came from.
  5. Ran NVDA from source code prior to and after removing the deprecated code paths to make sure regressions didn't occur (so far, none).

Status of removed code paths:

config.save:

  • Not used by anything in the NVDA Core source.
  • Already calls config.conf.save, and likewise other modules call config.conf.save instead of config.save.
  • Add-ons that weren't updated to deal with config.conf may need to be updated.
  • Core regression: none.

config.validateConfig:

  • config.ConfigManager does not call this function anymore, nor does other modules.
  • config.val was also removed, as config.conf.validator will now fetch validator directly from Validator constructor.
  • Add-ons that uses config.validateConfig will need to be updated to provide their own implementations.
  • Regressions: none.

SynthSetting.i18nName:

  • Not used by NVDA Core modules.
  • displayNameWithaccelerator replaces this attribute.
  • Speech synthesizer drivers that uses i18n must provide displayNameWithAccelerator.
  • Regressions: none.

speech.reason constants:

  • No module in NVDA Core uses this.
  • At least one add-on still relies on this, and fixing it is trivial.
  • Replaced by reason constants from control types.
  • Regressions: none.

SynthDriver.speakText/speakCharacter:

  • Should be handled by SynthDriver.speak.
  • NVDA Core falls back to speakText and speakCharacter.
  • speakText raises not implemented error, while speakCharacter is still implemented.
  • Audiologic driver still implements speakText.
  • Regressions: potentially major.

In case of the last one, I'd like to hear what Audiologic developers would have to say before proceeding, and for speech.reason constants, add-on authors will be notified.

Thanks.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Feb 6, 2017

Contributor
Contributor

jcsteh commented Feb 6, 2017

@josephsl

This comment has been minimized.

Show comment
Hide comment
@josephsl

josephsl Feb 6, 2017

Collaborator

Hi,

Results:

  • Svox Pico: ready to move on (no need to modify).
  • Festival: speakText still implemented.
  • Newfon: speakText still implemented.

For Newfon and Festival: if SynthDriver.speak and SynthDriver.speakText are identical in terms of what they do, then it'd be best to alert the authors so they can change it.

Thanks.

Collaborator

josephsl commented Feb 6, 2017

Hi,

Results:

  • Svox Pico: ready to move on (no need to modify).
  • Festival: speakText still implemented.
  • Newfon: speakText still implemented.

For Newfon and Festival: if SynthDriver.speak and SynthDriver.speakText are identical in terms of what they do, then it'd be best to alert the authors so they can change it.

Thanks.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Feb 6, 2017

Contributor
Contributor

jcsteh commented Feb 6, 2017

josephsl added a commit to josephsl/nvda that referenced this issue Feb 12, 2017

Removed config.validateConfig, config.conf.validator now uses Validat…
…or constructor. re nvaccess#6846, nvaccess#667.

As there's no need to store a module-level validator, validator class instanciation will be done from config.ConfigManager constructor.

josephsl added a commit to josephsl/nvda that referenced this issue Feb 12, 2017

josephsl added a commit to josephsl/nvda that referenced this issue Feb 12, 2017

feerrenrut added a commit that referenced this issue Mar 18, 2017

Incubates #6878
Re Issue #6846
Merge branch 'josephsl-i6846-removeSpeechReasonConstants' into next

feerrenrut added a commit that referenced this issue Mar 18, 2017

Incubates #6876
Re Issue #6846
Merge branch 'josephsl-i6846-removeConfigValidateConfig' into next

feerrenrut added a commit that referenced this issue Mar 18, 2017

Incubates #6877
Re Issue #6846
Merge branch 'josephsl-i6846-removeSynthSettingsI18NName' into next

feerrenrut added a commit that referenced this issue Mar 18, 2017

Incubates #6875
Re Issue #6846
Merge branch 'josephsl-i6846-removeConfigSave' into next

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

Speech module: remove reason_* constants. (PR #6878)
Partial fix for #6846

Remove deprecated `speech.REASON_*` constants, `controlTypes.REASON_*` should be used instead.

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

Removed config.validateConfig, config.conf.validator now uses Validat…
…or constructor. (PR #6876)

Partial fix for #6846, and #667

Remove deprecated `config.validateConfig`. 
As there's no need to store a module-level `validator`, `validator` class instantiation will be done from `config.ConfigManager` constructor.

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

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