Fix/text changed event args changed range sample

[codingdave]

See PavelTorgashov#215

[Hexman768]

@codingdave Thanks so much, I will be doing some testing and if all goes well, I should have this merged relatively soon!

[Hexman768]

@codingdave I see you have updated Start to SetStartAndEnd all over the place. Have you just done that everywhere Start was accessed or have you only replaced the calls the had to be replaced to resolve the issue?

[codingdave]

honestly, this code is 5 years ago. I hardly remember all the details. But you can check the original MR which has a very good description of my observation, the bugs, and the steps to getting them fixed - so please have another look.

As I can read I worked the problem systematically:
I observed that "The setter of Range.Start modifies Range.End. Start and End are equal.". So I checked all the places where Start is modified (as it changes End indirectly) and replaced it by the function SetStartAndEnd to be explicit.

That fixed the properties Start (and End).

By differentiating between Start, SetStartAndEnd, and End calls I was able to fix the ChangedRange, as you can see in the samples attached.

[Hexman768]

Here is an AI analysis of the code change, please take some of the potential issues into consideration, we definitely want to fix the potential for unbounded memory consumption.

What Changed

• Added CharSizeCache class:
  • Caches character size measurements for a given font and character, aiming to avoid repeated calls to TextRenderer.MeasureText (which are expensive).
  • Uses a Dictionary<string, SizeF> where the key encodes font properties and the character.
• Refactored GetCharSize:
  • Replaces direct measure logic with a call to CharSizeCache.GetCharSize.

---

Potential Issues & Review

1. Thread Safety

• Issue: The cache (Dictionary<string, SizeF>) is static and not thread-safe. If GetCharSize is called from multiple threads (e.g., in a multi-threaded UI or background rendering), this could lead to race conditions or exceptions.
• Recommendation: Use a concurrent dictionary (ConcurrentDictionary) or add lock protection around cache access.

2. Key Uniqueness and Font Identification

• Issue: The cache key is constructed from font properties and the character. This should be robust, but font comparison can be tricky:
  • If two fonts have identical properties but are different instances, this approach works.
  • If the font changes after being used as a key (unlikely, but possible if mutable), cached sizes may be inaccurate.
• Recommendation: Document this assumption or consider if it's possible for Font instances to share the same properties but behave differently.

3. Memory Usage

• Issue: The cache can grow unbounded if many fonts and characters are used, which may impact memory usage over time.
• Recommendation: Consider using a cache eviction strategy (e.g., LRU) or periodically clearing the cache if memory usage becomes problematic.

4. Measurement Accuracy

• Issue: The original algorithm for measuring the width of a character is:

  Size sz2 = TextRenderer.MeasureText("<" + c.ToString() + ">", font);
  Size sz3 = TextRenderer.MeasureText("<>", font);
  return new SizeF(sz2.Width - sz3.Width + 1, font.Height);

  • This method subtracts the width of "<>" from "", presumably to get just the width of c.
  • This works for monospaced fonts but may have edge cases for proportional fonts, ligatures, or font rendering quirks.
• Review: The caching does not change this approach, but any inaccuracies will now be reused multiple times. If the measurement algorithm is not correct for all fonts, this could propagate errors.

5. Unicode Characters / Surrogates

• Issue: Characters outside the BMP (Basic Multilingual Plane) may be represented as surrogate pairs (two chars). The current method only accepts single char values.
• Recommendation: If non-BMP characters are expected, consider accepting string or handling surrogate pairs.

6. Font Disposal

• Issue: The code correctly avoids holding onto Font instances, just using their properties as the cache key.
  • This is good and prevents leaking GDI resources.

7. Performance

• Positive: The cache should greatly improve performance if GetCharSize is called frequently with the same font/char combinations.

---

Summary

• Main Issues:
  • Thread safety of the cache.
  • Potential unbounded growth of the cache.
  • Measurement algorithm may not be accurate for all fonts or characters.
• Minor Issues:
  • Handling of surrogate pairs for non-BMP Unicode characters.

---

Actionable Recommendations

1. Make the cache thread-safe (e.g., use ConcurrentDictionary).
2. Consider cache size management if memory usage is a concern.
3. Review the measurement algorithm for edge cases.
4. If non-BMP Unicode characters are needed, update the function signature.

---

[codingdave]

Looking at the code it seems for me to be a big hallucination. I do not see any new class for caching (CharSizeCache) nor did the ai understand the pull request at all.

[Hexman768]

That may have actually been for a different pull request, and I just pasted it into the wrong Chrome tab.

My apologies there, I will get that cleaned up and try to get a new review completed as soon as I am able.