Some cleanup, fixing, and enhancements to TextFormatter

[dodexahedron]

While doing a cleanup and performance pass on TextFormatter, the loops in the FindHotKey method made go hmmm.

They're basically Rune-ified IndexOf operations, looking for the hotkey specifier Rune and then the following Rune as the hotkey, so long as it isn't FFFD.

But the state machine doesn't account for the character following the hotkey specifier being FFFD.

When that happens, the state machine keeps running until it finds the next non-specifier and non-FFFD Rune and calls that the hotkey. That's of course not expected, but what makes it even more dangerous is that the index provided in the hotPos out parameter will be the index of the specifier, as with normal cases, but the following index is not the hotkey that it chose.

The naive fix is simple: Just have it reset curHotPos to -1 each time it finds Rune.ReplacementChar.

I've added several test cases that cover various sequences containing the hotkey specifier in at least one position and FFFD in at least one position.

I also see some simple ways to squeeze almost free performance out of that method, so I'm doing that as well (this was in the middle of performance work, after all). Mostly some span stuff and avoiding as many loop iterations as I can figure out without spending too much time on just that method.

[dodexahedron]

So...

As I'm analyzing this, I'm noticing there are some invalid test cases (which I think was probably already known, based on comments around those tests).

In particular, the cases that include codepoint 0xE0B0 are incorrect.

Those shouldn't be returning as symbols, because that codepoint is in a private use area. If you feed that value to Rune.IsSymbol(thatRune), you'll get false, and if you feed it to Rune.GetUnicodeCategory(thatRune), you'll get UnicodeCategory.PrivateUse.

Those test cases should expect a false result, and we should probably have tests that include codepoints from each category, as well as combinations, both for scalars and for surrogate pairs, to ensure these methods are actually behaving properly. I'll at least be sure to fix the cases or the code, as appropriate. For that situation, it's both code and expected test result that need fixing.

I have what appears, so far, to be at least a more "correct" implementation (MUCH faster, too, for the majority of likely typical cases). I'll describe it in a follow-up comment here.

[dodexahedron]

The new implementation of the FindHotKey method that I have is partially complete, in that it works seemingly correctly for BMP values, but still needs me to make the fixes to non-BMP cases.

The only loop that has been addressed, so far, is the first one in the method.

The first thing it does is checks if it can simply do an ordinal char scan of the string or if it has to resort to using the Rune enumerator, by checking if the hotkey specifier Rune is in the BMP. If it is, we can do an ordinal scan for that character, even if the string also contains surrogate pairs (that particular point is made in the Microsoft Learn Supplementary API Remarks docs for working with strings and makes sense based on how it's all defined anyway.

Anyway...

1. If specifier is in the BMP, proceed to 1a, performing the loop beginning at index 0 of the string and ending one character before the end of the string (since it has to be a pair of chars to be valid):
   1a. If at end of the loop, proceed with 2. Otherwise, find IndexOf the specifier, beginning at index, and set index to the resulting value. Proceed to 1b.
   1b. If index was < 0 (not found) or we would go out of bounds, proceed to 1b1. Else proceed to 1c.
   1b1. Set curHotPos to index. Terminate loop and proceed with 2.
   1c. Get the rune that starts at index + 1 as potentialHotKeyRune. Proceed to 1d.
   1d. If all of the following are true of potentialHotKeyRune, proceed to 1e. Else increment index and proceed from 1a.
   • Is not FFFD
   • Is not same as specifier
   • Is in the BMP
   • Is a letter, digit, symbol, or punctuation
     1e. Set curHotKey to that rune and curHotPos to index. Terminate loop and proceed with 2.
2. If either of the following are true, perform the currently existing full unicode loop. Otherwise, skip that and just proceed with the rest of the method, as we've done what needs to be done up to that point:

• specifier is not in the BMP
• curHotPos > 0 and the rune that starts at curHotPos is not in the BMP (meaning either the previous loop didn't run (specifier not in BMP) or it did run but found a non-BMP rune after the specifier and bailed to enable fallback to this method).

That last step, for handling cases of either non-BMP specifier or when the specifier is BMP but the character following it is not, is currently still the original implementation, which I need to fix as described earlier. Just haven't gotten to that point yet.

When the first loop can run, this is dozens of times faster, in debug builds, and almost 400x faster in release builds, even before addressing the other loops that may need to run.

[dodexahedron]

Ugh. Sorry the formatting didn't come out like I wanted with that. Some things didn't get indented which should have and some got indented which shouldn't have.

Here is the code for the first loop, which is the main new part. Note most of the ifs are inverted to avoid nesting:

ReadOnlySpan<char> stringSpan = text.AsSpan ();
if (hotKeySpecifier.IsBmp)
{
  for(int index = 0;index < stringSpan.Length; index++)
  {
    index = text.IndexOf ((char)hotKeySpecifier.Value, index);

    if (index < 0 || index + 1 >= text.Length)
    {
      curHotPos = index;

      break;
    }

    if (!Rune.TryGetRuneAt (text, index + 1, out Rune potentialHotKeyRune))
    {
      continue;
    }

    if (potentialHotKeyRune == Rune.ReplacementChar
      || potentialHotKeyRune == hotKeySpecifier
      || !potentialHotKeyRune.IsBmp
      || (!Rune.IsLetterOrDigit (potentialHotKeyRune)
        && !Rune.IsPunctuation (potentialHotKeyRune)
        && !Rune.IsSymbol (potentialHotKeyRune)))
    {
      continue;
    }

    curHotKey = Rune.GetRuneAt (text, index + 1);
    curHotPos = index;

    break;
  }
}

[dodexahedron]

And here is the slightly modified second part, which is the fallback to full unicode, with the index reset fix applied:

if (!hotKeySpecifier.IsBmp || (curHotPos >= 0 && !Rune.GetRuneAt (text,curHotPos).IsBmp))
{
  int index = 0;
  foreach (Rune c in stringSpan.EnumerateRunes ())
  {
    if (c == Rune.ReplacementChar)
    {
      curHotPos = -1;
    }
    else if (c == hotKeySpecifier)
    {
      curHotPos = index;
    }
    else if (curHotPos > -1)
    {
      curHotKey = c;

      break;
    }

    index++;
  }
}

I'm not done with that, yet, so it may still have issues.

Note that using either or both of these with the current tests will fail on a few of them, so it's not worth bothering with that unless you're just curious. This is just so y'all can see what I'm thinking with this in case any of it raises any red flags.

[dodexahedron]

I'm totally going to benchmark this against the TextElementEnumerator, too, now that we have a cross-platform guarantee of consistent and correct behavior with it (which we didn't have when supporting .net Framework or .net < 5.0, making it pretty much useless before).

I would almost put money on it outperforming the Rune implementation and being more correct, implicitly, since it handles all code points, including surrogates, in multiple encodings, out of the box. And with the way there was SIMD love given to a lot of text-related stuff in .net 8, I even bet it might give my little BMP semi-optimization a run for its money, too.

[dodexahedron]

Also, about TextElementEnumerator and the StringInfo class...

It uses ICU, now, which means it uses the current "gold standard" unicode implementation, and will evolve with Unicode, unlike certain methods and properties of char and Rune which may be incorrect, incomplete, non-standard, or not agree with each other, even for the same codepoints (yes, even still in .net 8).

[BDisp]

That sounds interesting and I'm curious.

[tig]

TL;DR.

However, i scanned it and <3.

That said, it's dorky that HotKeySpecifier is a Rune. The only valid values are those typable on a keyboard with a single key press.

IMO, HotKeySpecifier should be Key.