Topics

delayed character descriptions. #addonrequestreview

DaVid
 

Hi there.

This week I wrote this add-on and I decided to send it for review.
This add-on announces the character description after some time if the
last read text was a character. Default time is 1 second, but you can
change it on the add-on settings.
This feature was copied from talkback on android, and a friend told me
that voice over on iOs has it also.

Get the add-on here:
https://github.com/david-acm/delayedCharacterDescriptions/releases/download/0.2a2/delayedCharacterDescriptions-0.2a2.nvda-addon

and the github repo here:
https://github.com/david-acm/delayedCharacterDescriptions

Regards,
David.

Noelia Ruiz
 

Hi, I was not sure regarding to post or not basic review results for
this add-on, since the documentation found when open add-on help for
the binary to be installed seems to correspond to other add-on. I
understand that we all make mistakes, and this is perfectly accepted.
But recently I mentioned another bug in documentation, just lack about
important scripts, and we need to install binaries or check them
before requesting a review at least in these basic things, imo. This
is not urgent and we can perform a basic test before requesting a
review, not for mistakes difficult to detect, of course. And
especially if several add-ons have the same issue, reviewers and users
in general can decide to ignore them.
But these are my results:

- License and copyright: pass. This is released under GPL 3, but NVDA
accepts GPL 2 or later, as recommended in copyright headers article:

https://github.com/nvaccess/nvda/wiki/Copyright-headers
- Security: pass

- Documentation: fail.

- User experience: fail.

I haven't reviewed the repo, since a binary was provided for
installation and firstly I have installed it for review.

Examples of failures (maybe others):

1. Users can enter values like alphanumeric characters in the edit box
and this can produce mistakes when trying to press OK (tested now on
Windows 7).
2. If for any reasons plugins are reloaded, the panel appears multiple
times in settings and this can be confusing.
3. Milliseconds are mentioned in the add-on description and in code,
but values are expressed in seconds (for instance, 1.0).
4. The add-on description seems to mean that descriptions are read for
the last read character, not just when users run a command used for
this. This is a small difference, but though documentation cannot
include all features and use cases, I think that this should be
clarified, since I have been confused thinking that if I press down
arrow and the line contains an unique character, the description could
be read too.
I will wait at least one or two weeks before reviewing this add-on if
you request this for a different version. I will do this for add-ons
when documentation or similar things seem to show that they need more
testing before request a review. Other reviewers can do it before.
This is to perform reviews better without burning out if too many
add-ons are sent too fast (smile).

Regards


2019-09-03 15:25 GMT+02:00, DaVid <dhf360@...>:
Hi there.

This week I wrote this add-on and I decided to send it for review.
This add-on announces the character description after some time if the
last read text was a character. Default time is 1 second, but you can
change it on the add-on settings.
This feature was copied from talkback on android, and a friend told me
that voice over on iOs has it also.

Get the add-on here:
https://github.com/david-acm/delayedCharacterDescriptions/releases/download/0.2a2/delayedCharacterDescriptions-0.2a2.nvda-addon

and the github repo here:
https://github.com/david-acm/delayedCharacterDescriptions

Regards,
David.



DaVid
 

Thanks. I should've sent it to testing, not reviewing. Sorry.

About points 2 and 3 they were solved in the latest commit, not
available in the latest release. I need to integrate it with appveyor
but I prefer to determine if this add-on is useful for the community
to mantain it.

point 1: what is the correct way to deal with this?
I extended the isValid method. I send an error message if the settings
are invalid and return False. Else True.
I set the kill focus to notify the user if the field has a wrong
value,as another check. But NVDA doesn't speak the message, so I send
a low frequency beep also.

Point 4: I don't know how to clarify it.
I based on textInfos.UNIT_CHARACTER, but I don't know all the cases
when NVDA sends this argument. I can track NVDA's code to know those
cases, but It could change in future NVDA versions.
Add-on developers can send it also, then the event will be activated.
I could write "based on textInfos.UNIT_CHARACTER argument", but this
is a very technical info so I don't know if is a good idea...

Regards,
DaVid.

2019-09-04 9:56 GMT-06:00, Noelia Ruiz <nrm1977@...>:

Hi, I was not sure regarding to post or not basic review results for
this add-on, since the documentation found when open add-on help for
the binary to be installed seems to correspond to other add-on. I
understand that we all make mistakes, and this is perfectly accepted.
But recently I mentioned another bug in documentation, just lack about
important scripts, and we need to install binaries or check them
before requesting a review at least in these basic things, imo. This
is not urgent and we can perform a basic test before requesting a
review, not for mistakes difficult to detect, of course. And
especially if several add-ons have the same issue, reviewers and users
in general can decide to ignore them.
But these are my results:

- License and copyright: pass. This is released under GPL 3, but NVDA
accepts GPL 2 or later, as recommended in copyright headers article:

https://github.com/nvaccess/nvda/wiki/Copyright-headers
- Security: pass

- Documentation: fail.

- User experience: fail.

I haven't reviewed the repo, since a binary was provided for
installation and firstly I have installed it for review.

Examples of failures (maybe others):

1. Users can enter values like alphanumeric characters in the edit box
and this can produce mistakes when trying to press OK (tested now on
Windows 7).
2. If for any reasons plugins are reloaded, the panel appears multiple
times in settings and this can be confusing.
3. Milliseconds are mentioned in the add-on description and in code,
but values are expressed in seconds (for instance, 1.0).
4. The add-on description seems to mean that descriptions are read for
the last read character, not just when users run a command used for
this. This is a small difference, but though documentation cannot
include all features and use cases, I think that this should be
clarified, since I have been confused thinking that if I press down
arrow and the line contains an unique character, the description could
be read too.
I will wait at least one or two weeks before reviewing this add-on if
you request this for a different version. I will do this for add-ons
when documentation or similar things seem to show that they need more
testing before request a review. Other reviewers can do it before.
This is to perform reviews better without burning out if too many
add-ons are sent too fast (smile).

Regards


2019-09-03 15:25 GMT+02:00, DaVid <dhf360@...>:
Hi there.

This week I wrote this add-on and I decided to send it for review.
This add-on announces the character description after some time if the
last read text was a character. Default time is 1 second, but you can
change it on the add-on settings.
This feature was copied from talkback on android, and a friend told me
that voice over on iOs has it also.

Get the add-on here:
https://github.com/david-acm/delayedCharacterDescriptions/releases/download/0.2a2/delayedCharacterDescriptions-0.2a2.nvda-addon

and the github repo here:
https://github.com/david-acm/delayedCharacterDescriptions

Regards,
David.





Noelia Ruiz
 

Hi, for the dialog, you can use a spin control setting min, max and initial values. You have examples in NVDA or, for example, in clipContentsDesigner add-on. This could be good, since you may not want that someone sets a value of 1000000 seconds :)

For determining the use cases in which the add-on could work, I don't know. It deppends on what can be useful. You may specify that this would work when right or left arrows are pressed, or when the character under the review cursor is reviewed, etc.
Also, in case this is useful for the community, you may want to describe also symbols, not just alphanumeric characters. Just an idea.
And for profiles, you may want to use code from other add-ons or contained in NVDA's core, for ease of maintainance, though for now your code works well. See characterProcessing.py for an example of NVDA's core.

Regards

El 04/09/2019 a las 19:39, DaVid escribió:
Thanks. I should've sent it to testing, not reviewing. Sorry.
About points 2 and 3 they were solved in the latest commit, not
available in the latest release. I need to integrate it with appveyor
but I prefer to determine if this add-on is useful for the community
to mantain it.
point 1: what is the correct way to deal with this?
I extended the isValid method. I send an error message if the settings
are invalid and return False. Else True.
I set the kill focus to notify the user if the field has a wrong
value,as another check. But NVDA doesn't speak the message, so I send
a low frequency beep also.
Point 4: I don't know how to clarify it.
I based on textInfos.UNIT_CHARACTER, but I don't know all the cases
when NVDA sends this argument. I can track NVDA's code to know those
cases, but It could change in future NVDA versions.
Add-on developers can send it also, then the event will be activated.
I could write "based on textInfos.UNIT_CHARACTER argument", but this
is a very technical info so I don't know if is a good idea...
Regards,
DaVid.
2019-09-04 9:56 GMT-06:00, Noelia Ruiz <nrm1977@...>:
Hi, I was not sure regarding to post or not basic review results for
this add-on, since the documentation found when open add-on help for
the binary to be installed seems to correspond to other add-on. I
understand that we all make mistakes, and this is perfectly accepted.
But recently I mentioned another bug in documentation, just lack about
important scripts, and we need to install binaries or check them
before requesting a review at least in these basic things, imo. This
is not urgent and we can perform a basic test before requesting a
review, not for mistakes difficult to detect, of course. And
especially if several add-ons have the same issue, reviewers and users
in general can decide to ignore them.
But these are my results:

- License and copyright: pass. This is released under GPL 3, but NVDA
accepts GPL 2 or later, as recommended in copyright headers article:

https://github.com/nvaccess/nvda/wiki/Copyright-headers
- Security: pass

- Documentation: fail.

- User experience: fail.

I haven't reviewed the repo, since a binary was provided for
installation and firstly I have installed it for review.

Examples of failures (maybe others):

1. Users can enter values like alphanumeric characters in the edit box
and this can produce mistakes when trying to press OK (tested now on
Windows 7).
2. If for any reasons plugins are reloaded, the panel appears multiple
times in settings and this can be confusing.
3. Milliseconds are mentioned in the add-on description and in code,
but values are expressed in seconds (for instance, 1.0).
4. The add-on description seems to mean that descriptions are read for
the last read character, not just when users run a command used for
this. This is a small difference, but though documentation cannot
include all features and use cases, I think that this should be
clarified, since I have been confused thinking that if I press down
arrow and the line contains an unique character, the description could
be read too.
I will wait at least one or two weeks before reviewing this add-on if
you request this for a different version. I will do this for add-ons
when documentation or similar things seem to show that they need more
testing before request a review. Other reviewers can do it before.
This is to perform reviews better without burning out if too many
add-ons are sent too fast (smile).

Regards


2019-09-03 15:25 GMT+02:00, DaVid <dhf360@...>:
Hi there.

This week I wrote this add-on and I decided to send it for review.
This add-on announces the character description after some time if the
last read text was a character. Default time is 1 second, but you can
change it on the add-on settings.
This feature was copied from talkback on android, and a friend told me
that voice over on iOs has it also.

Get the add-on here:
https://github.com/david-acm/delayedCharacterDescriptions/releases/download/0.2a2/delayedCharacterDescriptions-0.2a2.nvda-addon

and the github repo here:
https://github.com/david-acm/delayedCharacterDescriptions

Regards,
David.





DaVid
 

Thanks. I solved the integer edit control on just one line! :)

About describing characters, I delegate it to NVDA's functionality. In
the current version if the character description doesn't exist for the
current language, then will be ignored.
I updated the documentation also and added a readme.
The latest release (0.3a1) is available here:
https://github.com/david-acm/delayedCharacterDescriptions/releases/download/0.3a1/delayedCharacterDescriptions-0.3a1.nvda-addon

Regards,
DaVid.

2019-09-04 11:49 GMT-06:00, Noelia Ruiz <nrm1977@...>:

Hi, for the dialog, you can use a spin control setting min, max and
initial values. You have examples in NVDA or, for example, in
clipContentsDesigner add-on. This could be good, since you may not want
that someone sets a value of 1000000 seconds :)

For determining the use cases in which the add-on could work, I don't
know. It deppends on what can be useful. You may specify that this would
work when right or left arrows are pressed, or when the character under
the review cursor is reviewed, etc.
Also, in case this is useful for the community, you may want to describe
also symbols, not just alphanumeric characters. Just an idea.
And for profiles, you may want to use code from other add-ons or
contained in NVDA's core, for ease of maintainance, though for now your
code works well. See characterProcessing.py for an example of NVDA's core.

Regards


El 04/09/2019 a las 19:39, DaVid escribió:
Thanks. I should've sent it to testing, not reviewing. Sorry.

About points 2 and 3 they were solved in the latest commit, not
available in the latest release. I need to integrate it with appveyor
but I prefer to determine if this add-on is useful for the community
to mantain it.

point 1: what is the correct way to deal with this?
I extended the isValid method. I send an error message if the settings
are invalid and return False. Else True.
I set the kill focus to notify the user if the field has a wrong
value,as another check. But NVDA doesn't speak the message, so I send
a low frequency beep also.

Point 4: I don't know how to clarify it.
I based on textInfos.UNIT_CHARACTER, but I don't know all the cases
when NVDA sends this argument. I can track NVDA's code to know those
cases, but It could change in future NVDA versions.
Add-on developers can send it also, then the event will be activated.
I could write "based on textInfos.UNIT_CHARACTER argument", but this
is a very technical info so I don't know if is a good idea...

Regards,
DaVid.

2019-09-04 9:56 GMT-06:00, Noelia Ruiz <nrm1977@...>:
Hi, I was not sure regarding to post or not basic review results for
this add-on, since the documentation found when open add-on help for
the binary to be installed seems to correspond to other add-on. I
understand that we all make mistakes, and this is perfectly accepted.
But recently I mentioned another bug in documentation, just lack about
important scripts, and we need to install binaries or check them
before requesting a review at least in these basic things, imo. This
is not urgent and we can perform a basic test before requesting a
review, not for mistakes difficult to detect, of course. And
especially if several add-ons have the same issue, reviewers and users
in general can decide to ignore them.
But these are my results:

- License and copyright: pass. This is released under GPL 3, but NVDA
accepts GPL 2 or later, as recommended in copyright headers article:

https://github.com/nvaccess/nvda/wiki/Copyright-headers
- Security: pass

- Documentation: fail.

- User experience: fail.

I haven't reviewed the repo, since a binary was provided for
installation and firstly I have installed it for review.

Examples of failures (maybe others):

1. Users can enter values like alphanumeric characters in the edit box
and this can produce mistakes when trying to press OK (tested now on
Windows 7).
2. If for any reasons plugins are reloaded, the panel appears multiple
times in settings and this can be confusing.
3. Milliseconds are mentioned in the add-on description and in code,
but values are expressed in seconds (for instance, 1.0).
4. The add-on description seems to mean that descriptions are read for
the last read character, not just when users run a command used for
this. This is a small difference, but though documentation cannot
include all features and use cases, I think that this should be
clarified, since I have been confused thinking that if I press down
arrow and the line contains an unique character, the description could
be read too.
I will wait at least one or two weeks before reviewing this add-on if
you request this for a different version. I will do this for add-ons
when documentation or similar things seem to show that they need more
testing before request a review. Other reviewers can do it before.
This is to perform reviews better without burning out if too many
add-ons are sent too fast (smile).

Regards


2019-09-03 15:25 GMT+02:00, DaVid <dhf360@...>:
Hi there.

This week I wrote this add-on and I decided to send it for review.
This add-on announces the character description after some time if the
last read text was a character. Default time is 1 second, but you can
change it on the add-on settings.
This feature was copied from talkback on android, and a friend told me
that voice over on iOs has it also.

Get the add-on here:
https://github.com/david-acm/delayedCharacterDescriptions/releases/download/0.2a2/delayedCharacterDescriptions-0.2a2.nvda-addon

and the github repo here:
https://github.com/david-acm/delayedCharacterDescriptions

Regards,
David.








DaVid
 

Hi. I updated the documentation and added a script to enable or
disable the delayed descriptions.

Get the latest release here:
https://davidacm.github.io/getlatest/gh/davidacm/delayedCharacterDescriptions

Regards,
DaVid.

Cyrille
 

Hello

The issue is still present on last version of the add-on on a fresh portable NVDA 2019.2. §Even if it does not happen so often.
Here is the log (INFO level unfortunately):

ERROR - queueHandler.flushQueue (22:50:45.960):
Error in func cancelSpeech from eventQueue
Traceback (most recent call last):
File "queueHandler.pyo", line 53, in flushQueue
File "D:\Cyrille\Dev\NVDATestVersions\Test\userConfig\addons\delayedCharacterDescriptions\globalPlugins\delayedCharacterDescriptions.py", line 35, in cancelSpeech
File "speech.pyo", line 100, in cancelSpeech
ValueError: generator already executing

Cheers,

Cyrille

-----Message d'origine-----
De : nvda-addons@nvda-addons.groups.io <nvda-addons@nvda-addons.groups.io> De la part de DaVid
Envoyé : samedi 7 septembre 2019 21:13
À : nvda-addons@nvda-addons.groups.io
Objet : Re: [nvda-addons] delayed character descriptions. #addonrequestreview

Hi. I updated the documentation and added a script to enable or disable the delayed descriptions.

Get the latest release here:
https://davidacm.github.io/getlatest/gh/davidacm/delayedCharacterDescriptions

Regards,
DaVid.

Luke Davis
 

My review and comments.

License: fail

In the source file and readme you have GPL 3 listed, but the license file you provide in the repo is for GPL 2. Since GPL 2 anticipates GPL 3 this is permitted, but it seems like a bad idea to use both. Plus, GPL 3 does require you to provide a copy of it.

Also, both of them say that you should include a header section at the top of your source files, including a pointer to the license and warranty, which you don't really do. You are in good company here, as many senior devs don't, but they should. I have attached an example one for GPL 2, which i took from GPL 2 and modified as a Python comment.

Security: pass

Documentation: pass with suggestions

In your readme:
Line 2: you have "miliseconds" instead of "milliseconds".
Line 4: I think "consider" should be "considers".
Line 5: Spanish should be capitalized

In your source file:
Line 2: you have "miliseconds" instead of "milliseconds" again.
Line 107: "input mode" should be "input help mode".

In buildVars.py:
Line 27: you have "miliseconds" again.
I don't know if you intend this as a dev or stable version, but if dev, be aware that your release channel is set to stable (None).

Also, very small point, but this is the buildVars.py file from the old template. Be aware that the template on github has been updated.

User experience: pass

Other comments:

You did not ask for a code review, but I will point something out anyway.

You have:

import characterProcessing, config, controlTypes, globalPluginHandler, gui, addonHandler, six, speech, textInfos, threading, wx

While that is perfectly permissible, I have recently learned that PEP 8 doesn't like it. See https://www.python.org/dev/peps/pep-0008/#imports

Line 13: why are you doing this thing with the fake timer? You say so as not to set it to None, but are you sure None wouldn't work there?
(I haven't tested, I am just asking because it seems strange)

It is unfortunate that you have to use milliseconds at all here.
Maybe it would be better if you used two spin controls, one for the number of whole seconds, and one for the fractional portion.
The first one could range 0-20, and the second could range 0-9.
It's really ashame we can't use wx.lib.agw.floatspin, it would be perfect for this.

Regards,

--
Luke Davis
Moderator: the new NVDA Help mailing list! (https://groups.io/g/NVDAHelp)
Author: Debug Helper NVDA add-on (https://addons.nvda-project.org/addons/debugHelper.en.html)

derek riemer
 

Why not integrate this into core?

On Sun, Sep 15, 2019 at 3:17 AM Luke Davis <luke@...> wrote:
My review and comments.

License: fail

In the source file and readme you have GPL 3 listed, but the license file you
provide in the repo is for GPL 2. Since GPL 2 anticipates GPL 3 this is
permitted, but it seems like a bad idea to use both. Plus, GPL 3 does require
you to provide a copy of it.

Also, both of them say that you should include a header section at the top of
your source files, including a pointer to the license and warranty, which you
don't really do. You are in good company here, as many senior devs don't, but
they should. I have attached an example one for GPL 2, which i took from GPL 2
and modified as a Python comment.

Security: pass

  Documentation: pass with suggestions

In your readme:
Line 2: you have "miliseconds" instead of "milliseconds".
Line 4: I think "consider" should be "considers".
Line 5: Spanish should be capitalized

In your source file:
Line 2: you have "miliseconds" instead of "milliseconds" again.
Line 107: "input mode" should be "input help mode".

In buildVars.py:
Line 27: you have "miliseconds" again.
I don't know if you intend this as a dev or stable version, but if dev, be aware
that your release channel is set to stable (None).

Also, very small point, but this is the buildVars.py file from the old template.
Be aware that the template on github has been updated.

User experience: pass

Other comments:

You did not ask for a code review, but I will point something out anyway.

You have:

import characterProcessing, config, controlTypes, globalPluginHandler, gui, addonHandler, six, speech, textInfos, threading, wx

While that is perfectly permissible, I have recently learned that PEP 8 doesn't
like it. See https://www.python.org/dev/peps/pep-0008/#imports

Line 13: why are you doing this thing with the fake timer? You say so as not to
set it to None, but are you sure None wouldn't work there?
(I haven't tested, I am just asking because it seems strange)

It is unfortunate that you have to use milliseconds at all here.
Maybe it would be better if you used two spin controls, one for the number of
whole seconds, and one for the fractional portion.
The first one could range 0-20, and the second could range 0-9.
It's really ashame we can't use wx.lib.agw.floatspin, it would be perfect for
this.

Regards,

--
Luke Davis
Moderator: the new NVDA Help mailing list! (https://groups.io/g/NVDAHelp)
Author: Debug Helper NVDA add-on (https://addons.nvda-project.org/addons/debugHelper.en.html)





--
Derek Riemer
Improving the world one byte at a time!        ⠠⠊⠍⠏⠗⠕⠧⠬ ⠮ ⠸⠺ ⠐⠕ ⠃⠽⠞⠑ ⠁⠞ ⠁ ⠐⠞⠖
•    Accessibility enthusiast.
•    Proud user of the NVDA screen reader.
•    Open source enthusiast.
•    Skier.

•    Personal website: https://derekriemer.com




Luke Davis
 

On Sun, 15 Sep 2019, derek riemer wrote:

Why not integrate this into core?
Seconded.

Luke

DaVid
 

For derek and Luke: I agree with you. I really don't use this feature,
but is useful for many users. Some synths don't have good character
pronunciations, some people have hearing loss.
I feel some users have considered it a waste of time. I'm just saying,
we should consider all use cases.
Another case I discovered today, I'm learning Portuguese. For testing
purposes I always have this add-on enabled, and I realized that it is
very useful when starting with a new language.

For Cyrille: Can you test it with another add-ons disabled? maybe
another add-ons patched the speech methods also, and generated those
issues.
I can't reproduce it on my computers. We should use thread locks, but
NVDA's speech methods seem to be thread unsafe and I don't want to
patch more NVDA's code.
I want to keep this add-on as simpler as possible.

For luke: I changed it to GPL2. When I created the repo I selected 3.0
on github,but at some point an older licence file from another add-on
replaced it. I used a template that I had prepared for another add-on
and forgot it.
I don't like to distribute licence files on add-ons, is good for a
complete software as NVDA, but in plug-ins... I don't want the user
ends with many licence files about the same thing.
I don't care about headers in these small code pieces, but even NVDA
code omits it in the files I have seen.
About pep 8, I follow those standards in a company professional
project. But not in personal projects. E.G. they recommend to use
spaces and I don't like to use spaces on my projects.

I updated the mistakes in the documentation, and changed the channel to dev.

About the fake timer. That timer is not executed. Its just to avoid
checking the existence of a timer if the timer need to be cancelled.
That condition is False for the first time only. So, set a fake timer
doesn't affect the code functionality.

Milliseconds field: I think that the user can deal with it. For
personal use, I prefer to edit only one field and not two.

Regards,
DaVid.

Rui Fontes
 

Yes, I also want that!

Rui Fontes
NVDA portuguese team


Às 23:09 de 15/09/2019, Luke Davis escreveu:

On Sun, 15 Sep 2019, derek riemer wrote:

Why not integrate this into core?
Seconded.
Luke