Modify NVDA behavior with CLDR characters by Nardol · Pull Request #9707 · nvaccess/nvda

Nardol · 2019-06-10T11:32:47Z

Link to issue number:

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 · 2019-06-11T07:47:08Z

Is this all CLDR characters, or only Emojis? I understood this to be primarily about fixing Emoji handling, but my brief comprehension of CLDR is that it includes much more than Emojis. Although maybe I am mistaken about one of those things. Either way, I think the changelog should at least mention Emojis in this context, as the vast majority of people will have no idea what CLDR is, and will assume it doesn't apply to them. Maybe: (Section: New features) "Add a gesture, shift+NVDA+p by default, to toggle on and off CLDR (Emoji) character reporting". and (Section: Changes) "When CLDR (Emoji) character reporting is enabled, announce them on every punctuation level"

Nardol · 2019-06-11T08:15:31Z

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 · 2019-06-11T08:21:31Z

Thank you for the changes. Either way, nice work and will be appreciated by many I'm sure!

XLTechie · 2019-06-12T09:47:09Z

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.

Nardol · 2019-06-12T12:42:07Z

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

XLTechie · 2019-06-18T09:38:51Z

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 · 2019-06-18T09:46:01Z

Hi, I will review this code in a few moments, and no, I don’t think this will make the cut to 2019.2 as major features are frozen. Personally, when translatable string freeze is declared, I consider this a feature freeze. Thanks. From: Luke Davis <notifications@github.com> Sent: Tuesday, June 18, 2019 2:39 AM To: nvaccess/nvda <nvda@noreply.github.com> Cc: Subscribed <subscribed@noreply.github.com> Subject: Re: [nvaccess/nvda] Modify NVDA behavior with CLDR characters (#9707) CC: @michaelDCurran <https://github.com/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. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9707?email_source=notifications&email_token=AB4AXEDPQUA7TJGBX6X3FG3P3CUK7A5CNFSM4HWSWZA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX5Z2LQ#issuecomment-503029038> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4AXECIEX7EFY2X6SGH543P3CUK7ANCNFSM4HWSWZAQ> .

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

josephsl · 2019-06-18T09:55:24Z

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 · 2019-06-18T10:00:05Z

I wasn't thinking of how late in the game we were for 2019.2 when I wrote that, but of course you are right. That's what I get for posting on no sleep.

josephsl

Few general comments:

CLDR sconscript: can you write a short comment explaining why the punctuation level change is needed?
Keyboard command: I personally don't care about assigning a default shortcut, I think some may ask you to leave it unassigned.
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 · 2019-06-18T10:04:55Z

Hi Luke,

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

Thanks.

LeonarddeR · 2019-06-18T10:30:24Z

 		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(


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(

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.

LeonarddeR · 2019-06-18T10:32:04Z

 	# Translators: Describes a command.
 	script_recognizeWithUwpOcr.__doc__ = _("Recognizes the content of the current navigator object with Windows 10 OCR")

+	def script_toggleReportCLDR(self,gesture):


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

Nardol · 2019-06-18T13:06:59Z

@LeonarddeR: before I push all requested changes, are you on the same though as @josephsl about assigning no gesture by default? I ask because of your last comment on issue #8826 where you suggested the gesture. Thanks.

LeonarddeR · 2019-06-18T13:26:15Z

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

LeonarddeR · 2019-06-18T15:11:30Z

Ah, looks like I read things wrong there. Excuse me.

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

Nardol · 2019-07-03T14:13:47Z

@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 · 2019-07-03T15:36:20Z

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.

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 · 2019-08-01T18:13:01Z

PR introduces Flake8 errors 😲

See test results for Failed build of commit d66cc3b6b9

Nardol · 2019-08-02T11:56:18Z

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?

Nardol · 2019-08-17T18:30:55Z

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.

@josephsl

… be reviewed again by @josephsl or someone else

michaelDCurran

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.

Nardol · 2019-10-02T08:55:16Z

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.

josephsl

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).

…rom everywhere.

josephsl

Congratulations.

Modify NVDA behavior with CLDR characters to have them announced ever…

51584e6

…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

Nardol added 2 commits June 12, 2019 13:30

Merge remote-tracking branch 'origin/master' into toggleRepportCLDR

54d42e7

User guide: add gesture

471027c

Merge branch 'master' into toggleRepportCLDR

6ecb6fa

Merge branch 'master' into toggleRepportCLDR

286dbe7

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

josephsl suggested changes Jun 18, 2019

View reviewed changes

LeonarddeR suggested changes Jun 18, 2019

View reviewed changes

Nardol added 3 commits June 18, 2019 17:42

Changes after reviewS.

f86d49f

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

Merge branch 'master' into toggleRepportCLDR

d4c251a

Merge branch 'master' into toggleRepportCLDR

4db50f3

JulienCochuyt mentioned this pull request Jul 15, 2019

add reportActiveConfigurationProfile command #9325

Merged

Merge remote-tracking branch 'origin/master' into toggleRepportCLDR

e3c3eff

LeonarddeR approved these changes Jul 29, 2019

View reviewed changes

Nardol added 2 commits August 1, 2019 15:34

Merge branch 'master' into toggleRepportCLDR

797438c

Merge branch 'master' into toggleRepportCLDR

82b394e

Fix code style for Lint check

34ded1f

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

Code style fix

e5f1601

Nardol added 3 commits August 2, 2019 22:21

Merge branch 'master' into toggleRepportCLDR

3c38d3e

Reverted to original code style as codestyle checks now pass

e77187c

Merge branch 'master' into toggleRepportCLDR

7d38924

Nardol added 2 commits September 6, 2019 20:33

Merge branch 'master' into toggleRepportCLDR

702c6d9

Delete the commented gesture, will add it back if needed when PR will…

a3c6369

… be reviewed again by @josephsl or someone else

CoBC mentioned this pull request Sep 9, 2019

Not include emojis in punctuation traitment #9700

Closed

LeonarddeR approved these changes Oct 1, 2019

View reviewed changes

michaelDCurran requested changes Oct 2, 2019

View reviewed changes

Nardol added 2 commits October 2, 2019 10:46

Merge branch 'master' into toggleRepportCLDR

b7d8c2a

Remove keyboard shortcut from user guide

b9958f1

Nardol requested a review from michaelDCurran October 2, 2019 08:52

Nardol requested a review from josephsl October 2, 2019 08:56

josephsl suggested changes Oct 2, 2019

View reviewed changes

Nardol added 2 commits October 2, 2019 17:46

Merge branch 'master' into toggleRepportCLDR

7c03bb3

User guide: advise users to assign a command to toggle this feature f…

5402c95

…rom everywhere.

Nardol requested a review from josephsl October 2, 2019 15:53

josephsl approved these changes Oct 2, 2019

View reviewed changes

michaelDCurran approved these changes Oct 4, 2019

View reviewed changes

michaelDCurran merged commit 36e608d into nvaccess:master Oct 4, 2019

nvaccessAuto added this to the 2019.3 milestone Oct 4, 2019

michaelDCurran added a commit that referenced this pull request Oct 4, 2019

Update changes file for pr #9707

c5d9b9a

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

Uh oh!

Conversation

Nardol commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

XLTechie commented Jun 11, 2019 via email

Uh oh!

Nardol commented Jun 11, 2019

Uh oh!

XLTechie commented Jun 11, 2019 via email

Uh oh!

XLTechie commented Jun 12, 2019

Uh oh!

Nardol commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XLTechie commented Jun 18, 2019

Uh oh!

josephsl commented Jun 18, 2019 via email

Uh oh!

josephsl commented Jun 18, 2019

Uh oh!

XLTechie commented Jun 18, 2019 via email

Uh oh!

josephsl left a comment

Choose a reason for hiding this comment

Uh oh!

josephsl commented Jun 18, 2019

Uh oh!

LeonarddeR Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

Nardol Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

LeonarddeR Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

Nardol commented Jun 18, 2019 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LeonarddeR commented Jun 18, 2019

Uh oh!

LeonarddeR commented Jun 18, 2019 via email

Uh oh!

Nardol commented Jul 3, 2019

Uh oh!

feerrenrut commented Jul 3, 2019

Uh oh!

AppVeyorBot commented Aug 1, 2019

Uh oh!

Nardol commented Aug 2, 2019

Uh oh!

Nardol commented Aug 17, 2019

Uh oh!

michaelDCurran left a comment

Choose a reason for hiding this comment

Uh oh!

Nardol commented Oct 2, 2019

Uh oh!

josephsl left a comment

Choose a reason for hiding this comment

Uh oh!

josephsl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Nardol commented Jun 10, 2019 •

edited

Loading

Nardol commented Jun 12, 2019 •

edited

Loading

Nardol commented Jun 18, 2019 via email •

edited

Loading