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

Modify NVDA behavior with CLDR characters #9707

Merged
merged 22 commits into from Oct 4, 2019

Conversation

@pzajda
Copy link
Contributor

commented Jun 10, 2019

Link to issue number:

#8826

Summary of the issue:

Emojis are not announced on every punctuation level.
Emojis are not properly punctuations, there are characters which provide more precision about a written message missing them might cause confusion.
As emojis are handled with CLDR characters database, we have to change NVDA behavior for all these characters.

Description of how this pull request fixes the issue:

  • In the CLDR dictionaries generation, change the punctuation level to none
  • Add a global gesture (Shift+NVDA+p) to toggle them.

Testing performed:

  • Write a text with emoji
  • Read the text by line, word and character
  • Press Shift+NVDA+p
  • Repeat the two first steps
  • Tested with all punctuation levels, from none to all.

Known issues with pull request:

None found

Change log entry:

Section: New features

  • Add a gesture, shift+NVDA+p by default, to toggle on and off CLDR characters (including emojis) reporting

Section: Changes

  • When CLDR characters (including emojis) reporting is enabled, announce them on every punctuation level
…ytimes and quicker configurable.

* In the cldr.dic files, set the punctuation level to none
* Add shift+NVDA+p to toggle on and off CLDR characters anouncement
Fix #8826
@XLTechie

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@pzajda

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Thanks @XLTechie for your comment.
I've updated the pull request description to reflect the changes you mentioned.

I also added a test I did but forgot to mention.

@XLTechie

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@XLTechie

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Someone like @feerrenrut will need to review this, but before that, you might want to include with this an update to the user guide mentioning this new key.

@pzajda

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

I've just updated the user guide to mention the new gesture.
Thanks @XLTechie and sorry for this oversight.

@XLTechie

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

CC: @michaelDCurran
I know you're busy with Python 3 work, but this seemed like a small enough issue to make it into 2019.2 if someone could take a look at it.

@josephsl

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

Doesn't change anything but everything will be automatically tested again to be sure.
@josephsl

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

Hi,

Argument for postponing this after 2019.2: translatable strings are present, which will require explanation and translation by translators. A while ago string freeze was extended a bit but it was to allow really last minute translations to be included.

Thanks.

@XLTechie

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Copy link
Collaborator

left a comment

Few general comments:

  1. CLDR sconscript: can you write a short comment explaining why the punctuation level change is needed?
  2. Keyboard command: I personally don't care about assigning a default shortcut, I think some may ask you to leave it unassigned.
  3. Input help mode fails: I think there is a mismatch between the name of the new script and the name of the script used for obtaining the docstring (input help mode message). I can tell how this happened - when doing a copy/paste, I think it would help if you check the names defined for script and the docstring to really make sure input help works.

@leonardder, what do you think?

@josephsl

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

Hi Luke,

Don't worry about making that comment - I myself made terrible mistakes and assumed bad things after an insomnia.

Thanks.

@@ -30,7 +30,7 @@ def createCLDRAnnotationsDict(sources, dest):
with codecs.open(dest, "w", "utf_8_sig", errors="replace") as dictFile:
dictFile.write(u"symbols:\r\n")
for pattern, description in cldrDict.iteritems():
dictFile.write(u"{pattern}\t{description}\tsome\r\n".format(
dictFile.write(u"{pattern}\t{description}\tnone\r\n".format(

This comment has been minimized.

Copy link
@leonardder

leonardder Jun 18, 2019

Collaborator

Please remove None here, as it is the implicit default. It will save us a lot of space.

Suggested change
dictFile.write(u"{pattern}\t{description}\tnone\r\n".format(
dictFile.write(u"{pattern}\t{description}\r\n".format(

This comment has been minimized.

Copy link
@pzajda

pzajda Jun 18, 2019

Author Contributor

Please remove None here, as it is the implicit default. It will save us a lot of space.

After some test, it looks like the default level is all when I remove all "none", emojis were not pronounced except if I was on "all" level.

@@ -2286,6 +2286,21 @@ def script_recognizeWithUwpOcr(self, gesture):
# Translators: Describes a command.
script_recognizeWithUwpOcr.__doc__ = _("Recognizes the content of the current navigator object with Windows 10 OCR")

def script_toggleReportCLDR(self,gesture):

This comment has been minimized.

Copy link
@leonardder

leonardder Jun 18, 2019

Collaborator

Please use the script decorator to set script metadata. script_review_currentSymbol is a good example here.

@pzajda

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

@leonardder

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

I do not have a particular preference. Would be good to know what either @michaelDCurran or @feerrenrut prefers.

@leonardder

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

pzajda added 3 commits Jun 18, 2019
cldrDict_sconscript:
* Add a comment to specify why punctuation level is set to none
source/globalCommands.py:
* Use script decorator instead of old-style coding
* Fix translators comments
* commented gesture assignment until final advice
@pzajda

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@leonardder @josephsl @feerrenrut anything else to change on the code to be merged?
And another question: after the changelog push back, is there a chance for this feature to be included in NVDA 2019.2?

@feerrenrut

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

is there a chance for this feature to be included in NVDA 2019.2

No, only high priority bug fixes will be considered for merging for now.

@pzajda pzajda force-pushed the pzajda:toggleRepportCLDR branch from 75e224d to 4db50f3 Jul 28, 2019
pzajda added 2 commits Aug 1, 2019
One error left: continuation line under-indented for hanging indent
I don't understand how to fix this when I took it from another part of this file
@AppVeyorBot

This comment has been minimized.

Copy link

commented Aug 1, 2019

PR introduces Flake8 errors 😲

See test results for Failed build of commit d66cc3b6b9

@pzajda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

In commit e5f1601 I fixed a strange coding style thing:
When using the script decorator, the last ")" had to be at the same line than the script category instead of putting it on a new line indented by -1.
IMHO it is not realy good as it make two levels of indentation between two lines.
@leonardder @feerrenrut what do you think? Are you OK with the actual code style?

@pzajda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

All looks good with tests :) thanks for the fixes @feerrenrut and @leonardder if I don't make a mistake.
@josephsl what is your advice with the actual code?
I ask because there is still a waiting approval about oyour review.
Thanks.

pzajda added 2 commits Sep 6, 2019
… be reviewed again by @josephsl or someone else
Copy link
Contributor

left a comment

Is the script supposed to be bound to NVDA+shift+p by default or not? It looks like it isn't, yet the userGuide says it is.
I'm thinking that in the end it was decided not to bind the script by default. So if true, please remove the changes to the userGuide.

@pzajda pzajda requested a review from michaelDCurran Oct 2, 2019
@pzajda

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2019

I finally decided to let user decide to add a shortcut or not as suggested by @josephsl and it is now removed from the user documentation.

@pzajda pzajda requested a review from josephsl Oct 2, 2019
Copy link
Collaborator

left a comment

The only thing missing is the user guide edit advising users to assign a command to toggle this feature from everywhere (you can use the wording from simple review mode entry).

@pzajda pzajda requested a review from josephsl Oct 2, 2019
Copy link
Collaborator

left a comment

Congratulations.

@michaelDCurran michaelDCurran merged commit 36e608d into nvaccess:master Oct 4, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Oct 4, 2019
michaelDCurran added a commit that referenced this pull request Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.