Fix regexp error message in dictionaries by CyrilleB79 · Pull Request #14557 · nvaccess/nvda

CyrilleB79 · 2023-01-17T11:28:22Z

Link to issue number:

Summary of the issue:

Dictionaries do not allow to use backslash character normally in the replacement field when regexp is not selected as entry type. When such a character is used, a message box complaining of a regexp error appears.

Description of user facing changes

The user can use backslash in replacement field normally when entry type is not regexp
Error messages are more precise and more specific when entering erroneous values in the fields for a regexp entry

Description of development approach

When the entry type is not regexp, escape the replacement string
When entries are created:
- filter exception on re.error instead of the too global Exception to catch only exceptions related to regexps
- separate the try/except statement in two in order to indicate in which field the regexp error lies
- In case of a regexp error, after validating "OK" in the message box, the field where the error lies is re-focused
- Reports the regexp error only in case of entry type = regexp; in the other cases,re-raise the exception, in order not to catch unrelated exceptions, e.g. code bug.

Testing strategy:

Manual tests:

Add entry with backslash in replacement field and entry type = everywhere
Add entry with error either in pattern or in replacement field with entry type = regexp.

Known issues with pull request:

None

Change log entries:

Bug fixes
It is now possible to use the backslash character in the replacement field of a dictionaries entry, when the type is not set to regular expression. (#14556)

Code Review Checklist:

[x] Pull Request description:

description is up to date
change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
Security precautions taken.

XLTechie · 2023-01-17T11:43:02Z

For changelog:

It is now again possible to use backslash character in the replacement field of dictionaries. (#14556)

Suggestion:

It is now possible to use the backslash character in the replacement field of dictionaries, when the pattern is not set to regular expression. (#14556)

CyrilleB79 · 2023-01-17T14:18:29Z

@XLTechie thanks for the review. I have addressed the raised points in the code and in the initial description.

seanbudd

Thanks @CyrilleB79

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

CyrilleB79 · 2023-01-18T07:47:03Z

@seanbudd, I have addressed review comments. However, it is not possible to use f-strings with gettext so I have turned back to % interpolation as it was before.

I have also added two more lines to focus the field causing the regexp error as it was already done in case of empty pattern field error.

XLTechie · 2023-01-18T07:57:08Z

However, it is not possible to use f-strings with gettext so I have turned back to % interpolation as it was before.

I meant only to suggest that for the log messages, not the user messages. Apologies if I commented on the wrong line.

XLTechie · 2023-01-18T08:20:20Z

However, it is not possible to use f-strings with gettext so I have turned back to % interpolation as it was before.

It is possible, however, to use .format() with gettext, if you want to go slightly more modern than % replacements in the user messages. See globalCommands.py for examples, among others.

CyrilleB79 · 2023-01-18T09:01:31Z

However, it is not possible to use f-strings with gettext so I have turned back to % interpolation as it was before.
I meant only to suggest that for the log messages, not the user messages. Apologies if I commented on the wrong line.

No worry, you commented on the correct line. But I have then added another extra f-string in an inappropriate place, i.e. where gettext is used.

Regarding % versus .format() syntax, is there a recommendation or a preference to use the latter? (NVDA/NVAccess recommendation and/or Python/PEP recommendation)
I'll let @seanbudd decide if a change should be done.

josephsl · 2023-01-18T09:22:46Z

Hi, regarding formatted string literals (F strings) and string formatting, the former is not used in translatable strings due to security (translated messages can introduce strings that could crash Python), and using str.format allows translators to see more readable (and understandable) source text for translations. Thanks.

seanbudd · 2023-01-18T23:59:54Z

Agreed with @josephsl, the label in the format string e.g. {settingsName} is more explicit and helpful than %s

CyrilleB79 · 2023-01-19T07:40:10Z

Agreed with @josephsl, the label in the format string e.g. {settingsName} is more explicit and helpful than %s

OK @seanbudd, I have fixed it.

seanbudd

Looks good to me, other than a minor syntax issue

AppVeyorBot · 2023-02-13T23:42:48Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/8btitr2wn58l9tw7/artifacts/output/nvda_snapshot_pr14557-27678,5ed1c6b8.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 23.4,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 20.2,
FINISH_END 0.2

See test results for failed build of commit 5ed1c6b852

…placement expression of dictionary

…t expression of dictionary (#14664) Fixes #14663 Fix-up of #14557 Summary of the issue: When reading text containing a non-regexp dictionary entry, if the replacement field contains some special characters (such as space, +, etc.), these characters are escaped with a backslash. Thus this backslash is heard in the text spoken by NVDA. Description of user facing changes No unappropriated backslash will be spoken by NVDA when the replacement field of a dictionary entry contains space or other special character. Description of development approach Use theString.replace('\\', '\\\\') instead of re.escape(theString). Indeed, it was well documented in the doc of re.escape: This function must not be used for the replacement string in sub() and subn(), only backslashes should be escaped.

Fix regexp error message in dictionaries.

760e062

XLTechie reviewed Jan 17, 2023

View reviewed changes

Comment thread source/gui/speechDict.py Outdated

Review actions: slight reformatting of messages.

936b97a

CyrilleB79 force-pushed the regexpError branch from dc90aa4 to 936b97a Compare January 17, 2023 14:20

CyrilleB79 marked this pull request as ready for review January 17, 2023 16:36

CyrilleB79 requested a review from a team as a code owner January 17, 2023 16:36

CyrilleB79 requested a review from seanbudd January 17, 2023 16:36

seanbudd reviewed Jan 17, 2023

View reviewed changes

Comment thread source/speechDictHandler/__init__.py Outdated

Comment thread source/gui/speechDict.py Outdated

CyrilleB79 marked this pull request as draft January 17, 2023 23:36

CyrilleB79 and others added 3 commits January 18, 2023 08:03

Update source/speechDictHandler/__init__.py

309b733

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Do not use f-strings with gettext

fbd9bac

Focus the field in which there is an error.

5c6e0fd

CyrilleB79 marked this pull request as ready for review January 18, 2023 07:47

CyrilleB79 requested a review from seanbudd January 18, 2023 07:47

XLTechie mentioned this pull request Jan 18, 2023

Request: unit tests for speech dictionaries #14566

Open

Reformat error message strings.

6f7b6c3

seanbudd reviewed Feb 12, 2023

View reviewed changes

Comment thread source/gui/speechDict.py Outdated

Comment thread source/gui/speechDict.py Outdated

Apply suggestions from code review

f678111

seanbudd approved these changes Feb 13, 2023

View reviewed changes

seanbudd reviewed Feb 14, 2023

View reviewed changes

Comment thread source/speechDictHandler/__init__.py Outdated

seanbudd and others added 3 commits February 15, 2023 10:52

add typing

84e084d

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

06bdf88

update changes

a2f1e61

seanbudd merged commit 8b1b943 into nvaccess:master Feb 14, 2023

nvaccessAuto added this to the 2023.2 milestone Feb 14, 2023

CyrilleB79 added a commit to CyrilleB79/nvda that referenced this pull request Feb 22, 2023

Fix-up of nvaccess#14557: remove unwanted backslashes appearing in re…

90dc7be

…placement expression of dictionary

This was referenced Feb 22, 2023

Strange behavior in dictionaries if replacement contains space #14663

Closed

Fix-up of #14557: remove unwanted backslashes appearing in replacement expression of dictionary #14664

Merged

Uh oh!

Conversation

CyrilleB79 commented Jan 17, 2023 • 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 user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

XLTechie commented Jan 17, 2023

Uh oh!

Uh oh!

CyrilleB79 commented Jan 17, 2023

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CyrilleB79 commented Jan 18, 2023

Uh oh!

XLTechie commented Jan 18, 2023 via email

Uh oh!

XLTechie commented Jan 18, 2023 via email

Uh oh!

CyrilleB79 commented Jan 18, 2023

Uh oh!

josephsl commented Jan 18, 2023 via email

Uh oh!

seanbudd commented Jan 18, 2023

Uh oh!

CyrilleB79 commented Jan 19, 2023

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Feb 13, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CyrilleB79 commented Jan 17, 2023 •

edited

Loading