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

PR for mp3DirectCut-20.12 #1

Merged
merged 12 commits into from
Dec 12, 2020
Merged

PR for mp3DirectCut-20.12 #1

merged 12 commits into from
Dec 12, 2020

Conversation

abdel792
Copy link
Collaborator

@abdel792 abdel792 commented Dec 6, 2020

This Pull request is for the 20.12 version including the following fixes introduced in 20.11-dev:

  • Stop speech during recording and reading for the latest versions of mp3directcut;
  • Fixed reading remaining time for new versions of NVDA using Python 3.

NVDA translation automation and others added 10 commits January 17, 2020 13:01
* Stop speech during recording and reading for the latest versions of mp3directcut;
* Fixed reading remaining time for new versions of NVDA using Python 3.
… 20.11-dev:

* Stop speech during recording and reading for the latest versions of mp3directcut;
* Fixed reading remaining time for new versions of NVDA using Python 3.
Copy link

@nvdaes nvdaes left a comment

Choose a reason for hiding this comment

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

@abdel792, though it's not strictly needed, you may want to accept my suggestion before we merge this PR, since when an add-on declares to be compatible with NVDA 2019.3, in fact its compatibility is extended to higher versions, since there aren't api changes with break compatibility with later versions. Anyway, I think this is understood and if you want we can merge this like that.
Also, I would suggest to use () before methods without a space before the first symbol (, since it's difficult to fi searched for screen reader users who review the add-on.

Copy link

@nvdaes nvdaes left a comment

Choose a reason for hiding this comment

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

My suggestion is change compatibility line: instead of using "until 2020.3", you may use "until 2020.3 or higher", since when 2020.4 is released, users will find that the add-on is still compatible, due to the fact that api changes breaking compatibility aren't expected until NVDA 2021.1. Hope this helps.

readme.md Outdated
@@ -134,7 +134,12 @@ This addon offers the following commands:

## Compatibility ##

* This add-on is compatible with the versions of NVDA ranging from 2016.4 until 2019.4.
* This add-on is compatible with the versions of NVDA ranging from 2016.4 until 2020.3.
Copy link

Choose a reason for hiding this comment

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

Suggested change
* This add-on is compatible with the versions of NVDA ranging from 2016.4 until 2020.3.
* This add-on is compatible with the versions of NVDA ranging from 2016.4 until 2020.3 or higher.

@abdel792
Copy link
Collaborator Author

Hi @nvdaes,

Thank you.

I just created a new commit following your suggestion regarding compatible versions in readme.md.

@nvdaes
Copy link

nvdaes commented Dec 12, 2020

I'll approve this and will merge into stable. In case it's useful, when someone requests changes sending a review on GitHub with suggestions produced adding comments for specific lines and pressing control+g, the author of the PR should have the ability to apply all suggestions searching an accessible button on GitHub, without needing to do an extra commit. This may especially useful when multiple changes are required.

@nvdaes nvdaes merged commit 6800668 into nvdaaddons:master Dec 12, 2020
@abdel792
Copy link
Collaborator Author

Hi @nvdaes,

Thank you very much for your approval.

@nvdaes
Copy link

nvdaes commented Dec 12, 2020

Thank you @abdel792: I have merged this into stable branch too for svn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants