Skip to content

Revert "In Hindi, NVDA will not read anymore punctuation symbols whatever the punctuation level"#14477

Merged
seanbudd merged 1 commit into
masterfrom
revert-14459-updateCLDR
Dec 27, 2022
Merged

Revert "In Hindi, NVDA will not read anymore punctuation symbols whatever the punctuation level"#14477
seanbudd merged 1 commit into
masterfrom
revert-14459-updateCLDR

Conversation

@seanbudd

@seanbudd seanbudd commented Dec 27, 2022

Copy link
Copy Markdown
Member

Reverts #14459 due to #14473

…ever the punctuation level (#14459)"

This reverts commit 8746c6f.
@seanbudd seanbudd requested a review from a team as a code owner December 27, 2022 22:26
@seanbudd seanbudd requested review from feerrenrut and removed request for feerrenrut December 27, 2022 22:26
@seanbudd seanbudd merged commit ff37b86 into master Dec 27, 2022
@seanbudd seanbudd deleted the revert-14459-updateCLDR branch December 27, 2022 22:27
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Dec 27, 2022
@seanbudd seanbudd linked an issue Dec 27, 2022 that may be closed by this pull request
@CyrilleB79

Copy link
Copy Markdown
Contributor

@seanbudd, I am a bit concerned by this quick revert.

The current PR's description does not follow the template. Even if it's a revert, this PR should have been tested IMO and a description of these tests should be written in the initial description. Since you have not tested it yourself (as written in #14473 (comment)), a confirmation of @OzancanKaratas that this PR fixes #14473 would have been helpful.

Now I hope to have an answer of @OzancanKaratas for #14473 (comment), so that I can reintroduce #14459 without introducing #14473. But it is badly tracked because I am commenting in a closed PR and a closed issue.

If I have no answer soon, I'd better reopening a new PR similar to #14459 so that it be tested and commented. What would you recommend to do?

Thanks.

Also, when reverting, do not forget to reopen the corresponding issues, i.e. #14417 here.
The revert is also more tricky since #14459 introduces changes from a submodule (nvda-cldr). It seems that it has not been tracked in the corresponding repo (or is it?) When reverting an update of a submodule, the issue coming from the submodule should be correctly tracked.

Could you please finalize this half-revert or clarify in some way and better track the current situation? Thanks.

@seanbudd

seanbudd commented Jan 2, 2023

Copy link
Copy Markdown
Member Author

@CyrilleB79, current practice is to revert PRs which cause a known regression ASAP.
While sometimes a full PR description has been provided, it generally is not useful when tracking a revert.
The original PR contains information related to that PR.
This revert PR is not introducing a new feature or change, it's merely a janitorial action.
I can list many recent revert PRs with no/minimal description.

It is not our responsibility to test features that we have not planned for the release.
However, we do have the responsibility for ensuring regressions or further issues are not introduced into a release.
I hope this explains why the current practice is in place.

I have re-opened the initial issue #14459, my mistake for forgetting. It's a shame GitHub doesn't do this automatically.

seanbudd pushed a commit that referenced this pull request Mar 23, 2023
… punctuation level (2nd attempt) (#14558)

A first PR (#14459) had been merged to fix #14417. Unfortunately an issue was found (see #14473) so it has been reverted in #14477.

This PR is a second attempt to fix #14417 without causing #14473. It will remain a draft until I can have more information on #14473 from @OzancanKaratas, as requested in #14473 (comment), or from anyone else able to reproduce.

Link to issue number:
Fixes #14417

Summary of the issue:
Preliminary note for review
Keep in mind the following: in NVDA with CLDR enabled and with no custom user symbol defined, symbol level for symbol X is defined as follows:

look at locale symbol file:
If X is defined in this file and a symbol level is defined for X, then this level applies for X. Else, look at next file.
look at locale CLDR file:
If X is defined in this file and a symbol level is defined for X, then this level applies for X. Else, look at next file.
look at English symbol file:
If X is defined in this file and a symbol level is defined for X, then this level applies for X. Else, look at next file.
look at English CLDR file:
If X is defined in this file and a symbol level is defined for X, then this level applies for X. Else, use default symbol level (don't remember if it is None or All).
Description of the issue
Hindi has no symbol defined in its symbol file, only copyright header; seems that the file was prepared for translation but no actual symbol translation took place. But there is a Hindi CLDR file.

Currently, CLDR files are generated with level "None" for all symbols.

Usually, in locales with a CLDR file and a normal symbol files, less common characters that are only in CLDR are reported at level None, i.e. whatever the punctuation level setting of the user. But common punctuation symbols (dot, question marke, etc.) are added by translators in the locale symbol file what allows to have these symbols reported at a higher punctuation level.

For Hindi (or any language with no current symbol translated), all the characters present in CLDR file are reported at "None" level and above (i.e. at any level), because the level is not redefined in the locale (Hindi) symbol file.

In such situation, using the level of the locale CLDR (None) is not a good strategy. It would be better to take advantage of the levels defined for the symbols in the English symbol file.

Description of user facing changes
CLDR data will be available for languages which had no symbol file (am, et, kk, ne, th, ur) or empty symbol file (hi). For these languages, since there are no locale symbol file definition, the level defined in the English symbol file will be honoured.

Description of development approach
Update nvda-cldr repository to get the changes implemented in nvaccess/nvda-cldr#4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NVDA cannot read CLDR characters after #14459

3 participants