Hi @LeonarddeR - just noting that #16616 hasn't been triaged yet.
We're waiting on further discussion and testing in 2024.3 before considering changing the default for this in 2024.4

Thanks for pointing that out. Note that I was aware of the standpoint to delay this pr to 2024.4, but others might not have been.

@seanbudd this seems quite stable, which points do you think need still clarification? If you need more community feedback, this needs to be merged into alpha. I don‘t think there is anythink open regarding speech in #16616. you can also keep this behavior in alpha until you think it is stable enough to bring it into Beta. At least this is how we dealt with new features in the past as well, see e.g. cancellable speech which was enabled only in alpha for a longer period of time until it finally reached the stable version.

@seanbudd would you accept milestone 2024.3 for this PR if we change default value to disabled (see #16624 (comment))? This would at least avoid to ship 2024.3 with a buggy feature, closing #16624.

A subsequent PR for 2024.4 could then switch the default value to enabled.

@CyrilleB79 - yes as requested in #16624 (comment), however @LeonarddeR seems to suggest in #16624 (comment) that this is not possible for whatever reason

@seanbudd I must have misunderstood you then. I thought you were asking in #16624 (comment) whether it would be possible to fix #16624 without changing character navigation behavior. That would be very difficult. If you'd agree changing this pr to disable normalization again by default but yet change character navigation to also normalize, that is certainly possibly and I can change the pr as requested by @CyrilleB79.

That would be great yes. The normalization fix can go into 2024.3, but we don't want to turn it on by default until it has had some wider testing in a release

• FAIL: Lint check. See test results for more information.
• PASS: Translation comments check.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/xpx9om5592xwryp7/artifacts/output/nvda_snapshot_pr16622-32209,8e0d3c7f.exe
• PASS: System tests (tags: installer NVDA).
• PASS: Unit tests.
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.1,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 12.0,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.4,
  TEST_START 0.0,
  TEST_END 2.4,
  FINISH_END 0.2

See test results for failed build of commit 8e0d3c7fdb

• 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/b78s5cpjpirs2eyj/artifacts/output/nvda_snapshot_pr16622-32210,de6705d0.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.1,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 11.4,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.4,
  TEST_START 0.0,
  TEST_END 2.3,
  FINISH_END 0.2

See test results for failed build of commit de6705d08d

@LeonarddeR, after reviewing the implementations of calculateCharacterBoundaries and _calculateUniscribeOffsets in textUtils.cpp, here are some observations and a proposed refactoring strategy to reduce code duplication:

Observations:

1. Common Functionality: Both functions utilize the ScriptItemize and ScriptBreak functions from the Uniscribe API to analyze text. The main difference lies in how they handle the results from these functions.
2. Error Handling and Resource Management: Both functions handle errors similarly and have similar resource management patterns, especially with dynamic memory allocation for SCRIPT_ITEM and SCRIPT_LOGATTR.

Proposed Refactoring:

To reduce duplication, you can abstract the common parts of these functions into helper functions. Here’s a proposed refactoring:

1. Common Script Itemization:
   Create a function to handle the common itemization logic.

   bool itemizeScript(const wchar_t* text, int textLength, SCRIPT_ITEM** items, int* numItems) {
       *items = new SCRIPT_ITEM[textLength + 1];
       return ScriptItemize(text, textLength, textLength, nullptr, nullptr, *items, numItems) == S_OK && *numItems > 0;
   }

2. Common Script Breaking:
   Create a function to handle the common breaking logic.

   bool breakScript(const wchar_t* text, int textLength, SCRIPT_ITEM* items, int numItems, SCRIPT_LOGATTR** logAttrArray) {
       *logAttrArray = new SCRIPT_LOGATTR[textLength];
       int nextICharPos = textLength;
       for (int itemIndex = numItems - 1; itemIndex >= 0; --itemIndex) {
           int iCharPos = items[itemIndex].iCharPos;
           int iCharLength = nextICharPos - iCharPos;
           if (ScriptBreak(text + iCharPos, iCharLength, &(items[itemIndex].a), *logAttrArray + iCharPos) != S_OK) {
               return false;
           }
       }
       return true;
   }

3. Refactor Existing Functions:
   Utilize these helper functions in calculateCharacterBoundaries and _calculateUniscribeOffsets to reduce duplication and centralize the common logic.

This approach not only reduces duplication but also enhances maintainability by centralizing the error handling and memory management, making the code cleaner and easier to manage.

Let me know if this refactoring aligns with your expectations or if you need further modifications!

#!/bin/bash
# Extract the implementations of calculateCharacterBoundaries and _calculateUniscribeOffsets from textUtils.cpp
ast-grep --lang cpp --pattern $'$_ $_(calculateCharacterBoundaries, _calculateUniscribeOffsets)($$$) {
  $$$
}' --file textUtils.cpp

#!/bin/bash
# Correctly extract the implementations of calculateCharacterBoundaries and _calculateUniscribeOffsets from textUtils.cpp
ast-grep --lang cpp --pattern $'$_ $_(calculateCharacterBoundaries, _calculateUniscribeOffsets)($$$) {
  $$$
}'

#!/bin/bash
# Fetch the entire content of textUtils.cpp
cat textUtils.cpp

#!/bin/bash
# Search for the textUtils.cpp file in the repository to find the correct path
fd textUtils.cpp

#!/bin/bash
# Fetch the entire content of textUtils.cpp from the correct path
cat nvdaHelper/local/textUtils.cpp

@coderabbitai I didn't like your suggestions much. I refactored it in the last commit.

@LeonarddeR, I've reviewed the latest changes in textUtils.cpp from your last commit. It looks like you've made significant improvements by abstracting the common logic into the _getLogAttrArray function, which simplifies the handling of script itemization and breaking. This refactoring enhances the modularity and readability of the code.

Using std::vector for managing SCRIPT_ITEM and SCRIPT_LOGATTR arrays is a robust choice as it automatically manages memory and makes the code cleaner and safer by avoiding manual memory management.

If there's anything specific you'd like feedback on or further improvements you're considering, feel free to let me know! Great work on these refinements.

#!/bin/bash
# Fetch the latest changes made to the textUtils.cpp file in the last commit
git diff HEAD~1 -- nvdaHelper/local/textUtils.cpp

@LeonarddeR -is this ready for re-review?

I'd encourage any change of policy/usage of feature flags to be moved to a separate discussion.
Right now they are encouraged for general settings where appropriate, and are used throughout the braille settings, as well as in paragraph nav and other general settings.

@seanbudd Yes, I think the unit tests, including some edge cases, seem to prove that this works as advertised.

@seanbudd

I'd encourage any change of policy/usage of feature flags to be moved to a separate discussion.
Right now they are encouraged for general settings where appropriate, and are used throughout the braille settings, as well as in paragraph nav and other general settings.

This is because people didn't pay attention to the actual purpose of feature flags when reviewing the coresponding PRs. I think it makes sense to clearly not encourage using these feature flags outside of the advanced settings panel anymore, and state the actuual purpose in the policy docs.
Many users find this feature flag approach outside of the advanced settings as quite bad UX.

is this ready for re-review?

Yes, sorry. Forgot to mark it as ready