ContextQMD
Back to Unigram

FormattedTextBlock review — to-do

notes/formatted-text-block-review.md

12.1035.3 KB
Original Source

FormattedTextBlock review — to-do

Status: landed on develop 2026-08-15, 24 of 27 items fixed. Fela tested the series in the app against the message set below and confirmed the behaviour, including the relative date and spoiler cases that were visibly broken beforehand.

Three items are still open, all of them decisions rather than patches — the non-pooled unload/reload teardown (P2), the WalkInlines enumerator (P3, wants a profiler) and HasLineEnding's commented-out InvalidateMeasure (P3, belongs with the layout-cycle audit). Each says under its own entry what it is waiting on.

formatted-text-block-test-plan.md and formatted-text-block-test-messages.py beside this file reproduce the test set; run the script immediately before testing, since the relative dates are anchored seconds old on purpose.

Read-through of Telegram/Controls/FormattedTextBlock.cs (2,704 lines) and Telegram/Controls/FormattedTextBlock.Selectable.cs (405), cross-checked against Telegram.Native/Controls/FormattedTextBlockBase.{h,cpp}, the host Telegram/Controls/Messages/MessageTextBlock.cs, Telegram/Common/TextStyleRun.cs (StyledText/StyledParagraph), the template in Telegram/Themes/Generic.xaml:2367 and the call sites in Controls/Cells/, Controls/Messages/ and Controls/Chats/.

Line numbers are as of 73b3ec369 (the three files are untouched since c23b13d6d), i.e. before the P0 commits below; anything still open has shifted by ~17 lines since.

Legend: [live] = confirmed reachable from current app code, with the call site named · [latent] = correct today only by convention or because no caller exercises it.

The three facts most of this rests on:

  • MessageTextBlock hands each child block a paragraph range of a shared StyledText (MessageTextBlock.cs:313-354). Every offset in StyledText.Text is absolute for the whole message; the rendered/TextHighlighter index space is per block and starts at 0. _indexMap is the only thing that reconciles the two.
  • Only MessageBubble.Message gets a RecyclePool (MessageBubble.xaml.cs:190/278). Every other FormattedTextBlock in the app — ChatCell.BriefText, WebPageContent, PollContent, MessageReply.Label, ChatPinnedMessage, … — runs with _pools == null, which changes what OnLoaded/OnUnloaded do.
  • The app runs several XAML threads (see the [ThreadStatic] RelativeDateService at :2543). Anything cached statically here must be a value type or [ThreadStatic] — Brush/FontFamily are DependencyObjects and thread-affine.

P0 — wrong text comes out

  • The fast path drops the index map for blocks that don't start at paragraph 0 — FormattedTextBlock.cs:1005-1007 [live] → fixed in the commit that checked this box (git log --follow formatted-text-block-review.md)

    ```csharp
// Plain single run: rendered index == styled offset, so the converter's
// 1:1 fallback is exact — no map needed.
_indexMap = null;
```

The identity only holds when `_first == 0`. The fast path is entered whenever
`rangeStart == rangeEnd` and that paragraph `IsPlain` (`:929`), and `MessageTextBlock`
emits exactly that shape for **every single normal paragraph sandwiched between typed
ones** (`MessageTextBlock.cs:230/234`) — e.g. a message of `code block / plain line /
code block` gives the middle block `_first == _last == 1`. A fresh block has
`_plain == true` and `HasCodeBlocks == false`, so the `_plain == prevPlain` guard does
not save it.

With a null map, `RenderedToStyled` and `StyledToRendered`
(`Selectable.cs:278/311`) return the rendered index unchanged, so everything built on
them is off by `styled.Paragraphs[_first].Offset`:

- `GetSelectedText` (`Selectable.cs:168`) — selecting that line and copying yields text
  from the **start of the message**, of the right length.
- `GetSourceOffset` (`:182`) — the cross-block range `TextSelectionManager` builds is
  anchored at the wrong paragraph.
- `GetSelectionBoundary` (`:199`) — double-click word / triple-click paragraph resolves
  against the wrong paragraph, so `ParagraphRange` returns another paragraph's `[lo, hi)`.
- `ApplyHighlighters` (`:505`) — `StyledToRendered(find)` places the search highlight past
  the end of the block's content.

Fixed with an `_origin` field — the styled offset that rendered index 0 maps to — set
next to `_first`/`_last` in `SetText`, so it is right before every early return and on
both paths, and consulted only by the no-map fallback in `RenderedToStyled` /
`StyledToRendered`. The fast path stays allocation-free.

Setting it in `SetText` rather than in the fast-path branch also covers the case the
original note missed: the slow path can leave `_indexMap` **empty** (every entity skipped
by the `entity.Length + entity.Offset > text.Length` guard at `:1141`), which takes the
same fallback.

  • ProcessCodeBlock's execution guard is an ABA — :941, :886, :1931 [live] → fixed in the commit that checked this box

    `var execution = ++_templateExecuted;` (`:941`) versus `_templateExecuted = 0;` in
`OnUnloaded` (`:886`). The counter restarts, so `execution == 1` is handed out again after
every unload. A tokenization started before the unload (`SyntaxToken.TokenizeAsync`, an
`await` that can outlive a scroll) resumes, finds `_templateExecuted == 1` and passes the
"still the same content" test for **different** content.

What it then does is worse than a stale repaint: `inlines` is the `Paragraph_Inlines`
collection of a paragraph that `Recycle` (`:807-812`) has already returned to the shared
`FormattedTextBlockRecyclePool`, so `direct.ClearCollection(inlines)` (`:1940`) wipes
whichever block dequeued it, and `ProcessCodeBlock` fills it with the old message's syntax
spans. Only pooled blocks (message bubbles) are affected, which is also the only place
code blocks scroll fast.

Fixed with a second, never-reset `_generation` counter for the async guard.
`_templateExecuted` kept only its other job — the "text already applied" flag `OnLoaded`
tests, which is exactly why it has to be cleared on unload — so it became
`bool _textApplied`.

P1 — allocation on the hottest text surface in the app

  • _light and _dark are instance fields — :2023 and :2054 [live] → fixed in the commit that checked this box

    Two `Dictionary<string, Color>` of 27–28 entries, built **per `FormattedTextBlock`**,
i.e. per message block, whether or not the message contains code. Colors are structs and
the tables are constant, so `static readonly` is a straight win (≈3 KB per instance) with
no thread affinity to worry about.

`_brushes` (`:2087`) must stay per-thread at least, because `SolidColorBrush` is
thread-affine, but it can be allocated lazily on the first `GetColor` call instead of in
the field initializer — only code blocks ever touch it.

Both done: the tables are `static readonly` (never mutated — `OnActualThemeChanged`
writes through to the *brushes*, not the tables), and `_brushes` is now null until
`GetColor` needs it, with the theme handler returning early when it is.

  • monospaceFontFamily is never assigned — :1060-1064 [live] → fixed in the commit that checked this box

    ```csharp
FontFamily monospaceFontFamily = null;
FontFamily GetMonospaceFontFamily()
{
    return monospaceFontFamily ?? new FontFamily("Cascadia Mono, Consolas, " + Theme.Current.XamlAutoFontFamily);
}
```

The local is only ever read, so the memoization does nothing: every inline-code entity in
the paragraph allocates a fresh `FontFamily` plus the concatenated string.

Same in the recursive `ProcessCodeBlock` (`:1956`) — `new FontFamily(...)` is built once
per **token node**, so a syntax-highlighted block allocates one per span in the tree.

Worth remembering that a font chain is not free at use either: see the
"packaged font fallback cost" note — RichEdit pays ~1 ms per run for a chain whose first
entry misses.

Fixed one level up, on Fela's call: the chain is now `Theme.MonospaceFontFamily`, built in
`UpdateEmojiSet` beside the other families — which is also the only place it can change —
and every site in the app reads it. That covers the eight other call sites, six of which
(`TextBlockHelper.cs:226/347`, `GameContent.xaml.cs:240/245`,
`ProfileHeader.xaml.cs:1735/1740`) were allocating one `FontFamily` **per code entity
inside a render loop**.

Those six built `"Cascadia Mono, Consolas"` with no fallback tail, so they now inherit
one: a character the monospace faces don't cover (an emoji in a code span in a bio) will
render instead of dropping to the system default. Deliberate, but it is a rendering
change — say so if you'd rather keep two chains.

  • OnPointerMoved re-sets the cursor on every sample — :322-331 [live] → fixed in the commit that checked this box

    ```csharp
if (hyperlink == null)
{
    _textSelectionIBeam = true;
    Window.Current.CoreWindow.PointerCursor = new Windows.UI.Core.CoreCursor(CoreCursorType.IBeam, 0);
}
```

`_textSelectionIBeam` is written but never tested, so moving over message text allocates a
`CoreCursor` and writes `CoreWindow.PointerCursor` at pointer sample rate.

Fixed by testing the flag — with the part the note missed: the flag also has to be
**cleared** over a hyperlink. The `Hyperlink` puts its own `Hand` cursor up, so ours is
no longer on screen, and gating on a flag that stayed `true` would skip restoring the
I-beam on the way back onto text and leave the `Hand` there.

The remaining `CoreCursor` allocations (`Arrow` at `:316`/`:340`/`:346`, `Hand` at
`:304`) are all edge-triggered already, so they're left alone.

  • MeasureOverride runs a native text measure per pass for expandable quotes — :256-277 [live] → fixed in the commit that checked this box

    `PlaceholderHelper.Foreground.MaxLines(...)` on every measure, with no memo on
(text, width, size). Quote blocks re-measure with the rest of the bubble on every window
resize and on every `InvalidateMeasure` from the panel.

Fixed by keeping those three inputs and skipping the call when they match. The text is
compared by reference, which is exact here rather than a heuristic: `GetParts` returns
the same string until a relative date rewrites the paragraph, and that is precisely when
the measurement can change. `TextBlock` is also null-checked now, since the `FontSize`
read on the `AutoFontSize == false` path had nothing guarding it.

Related: `IsTextTrimmableChanged` is raised **from inside** `MeasureOverride` (`:271`) and
`BlockQuote.OnIsTextTrimmableChanged` (`BlockQuote.cs:62`) reacts by changing state that
feeds `ComputedIsExpandable`, which `MessageTextBlock.ArrangeOverride` reads
(`MessageTextBlock.cs:445`). That is the shape the layout-cycle audit is about; worth
checking it can't re-enter.

  • Seven collections per instance, before any content — :97-99 and :555-558 → fixed in the commit that checked this box

    `_links`, `_dates`, `_spoilers` + four `HashSet`s for the active elements. The vast
majority of blocks have no hyperlink, no spoiler and no date. Lazy `??=` at the first
`Add` costs a null check on a path that is already doing XAML interop.

While there: the four `_active*` sets only need `Contains`/`Remove` for
`_activeRuns` (`:1934`, the async code path). `List<T>` is cheaper to fill and to
enumerate for the other three, and `_activeRuns`' single linear scan happens once per
tokenized code block.

Done, with the split the note implies: `_links`/`_dates`/`_spoilers` are lazy (they have
an `Add` site and a teardown site each, so the guards are few), while `_activeRuns` and
`_activeParagraphs` stay eager because every rendered block fills them — laziness there
would be six null checks for nothing. All four became `List<T>`.

The set → list swap rests on elements never being added twice, which holds: each is
either newly constructed or dequeued from the pool (so removed from it), `Recycle` clears
the lists, and the one place that hands an element back mid-render
(`ProcessCodeBlock`'s placeholder) removes it from the list before enqueuing it.

P2 — state that survives when it shouldn't

  • A block with no RecyclePool never rebuilds after unload/reload — :864 vs :887 [latent] — needs a repro before fixing

    `OnUnloaded` calls `ClearEntities()` unconditionally (`:887`) — tooltips detached, relative
dates unsubscribed, `_spoilers` emptied, `_effectiveViewportChanged` dropped and the
viewport registration revoked — and only *then* returns early for `_pools == null` (`:889`),
leaving the inline tree in place. `OnLoaded` then returns early for the same reason
(`:864`), so nothing re-runs `SetText`.

The visible tree still shows the right text, but: link tooltips are gone, relative dates
freeze, custom emoji stop being told about the viewport, and `_spoilers` is empty while
the transparent `_spoiler` highlighter is still applied — so the next `UpdateSpoilers`
(any inner size change reaches it through `HandleSizeChanged`,
`FormattedTextBlockBase.cpp:73-77`) removes the particle overlay and leaves the spoilered
text *invisible* rather than revealed.

Most non-pooled hosts re-`SetText` on reuse, which is why this hasn't shown up; the ones to
check are those that unload/reload without re-setting text (popups, pivots,
`ChatPinnedMessage`).

→ **left open deliberately.** Both fixes the note proposes cost something real, and
picking between them blind is worse than leaving the bug:

- *Move `ClearEntities()` behind the early-out* — non-pooled blocks would keep their
  tooltips, date subscriptions and viewport registration across an unload. But a block
  that unloads and never comes back then stays subscribed to the thread-static
  `RelativeDateService`, which pins the block and everything it references. Trading a
  stale-state bug for a leak.
- *Let `OnLoaded` re-apply regardless of `_pools`* (drop `|| _pools == null` from `:864`)
  — correct, but it puts a full `SetText` on every reload of every non-pooled block, and
  the biggest population of those is `ChatCell.BriefText`. The chat list already re-sets
  text on reuse, so that is a second full rebuild per recycled row on the app's hottest
  list.

What decides it is which hosts actually reload without re-setting text. That is a repro
away, not an argument away.

  • Relative dates can stall for good — :2600-2648 [live] → fixed in the commit that checked this box

    In `GetNextUpdateInterval`, an item that isn't due yet contributes
`remainingSeconds = (long)(item.NextUpdateAt - tickCount) / 1000` (`:2640`) and is skipped
when that truncates to 0. If every item is within a second of its deadline — the normal
case for the `< 60s` bucket, where the interval is 1 s and `DispatcherTimer` can fire a
few ms early — `minSeconds` stays `int.MaxValue` and the timer is rearmed
**68 years out**. Nothing else restarts it, so every relative timestamp on the thread
freezes until another `Subscribe` happens.

Fixed both ways the note suggests, because they cover different holes: the not-due branch
rounds up instead of truncating, and the return clamps `int.MaxValue` to a second so an
empty or fully-skipped set can't arm the timer past the heat death of the sun.

  • record TextDate — :2494 [live] → fixed in the commit that checked this box

    The project rule is that .NET Native has no records. It is also the wrong shape: it's a
mutable dictionary *value* (`NextUpdateAt { get; set; }`) that never needs value equality,
and the generated `Equals`/`GetHashCode`/`PrintMembers` walk five reference members. A
plain class is smaller and generates nothing.

Now a plain class. `EntityType` went with it: it was a positional member only because the
constructor needs it to compute `Date`, and nothing ever read it back.

  • Revealing a spoiler wipes the search highlight — :435-436 [live] → fixed in the commit that checked this box

    ```csharp
SetText(_clientService, _text, _first, _last, _fontSize);
SetQuery(string.Empty);
```

`SetText` already calls `ApplyHighlighters` (`:1584`), so the `SetQuery(string.Empty)` is
not needed to repaint — it only sets `_query = ""`, dropping the in-message search
highlight (`MessageBubble.xaml.cs:147`) when the user taps a spoiler. Delete the line.

  • ApplyHighlighters silently drops everything when the inner block isn't loaded — :482 [live, but pre-existing] → fixed in the commit that checked this box

    The `!TextBlock.IsLoaded` early-out predates the refactor (it was `SetQuery`'s), but it now
gates the spoiler/cached/marked highlighters too, and there is **no re-apply on load**:
`OnApplyTemplate` calls `SetText` (`:247`), which sets `_templateExecuted = 1`, and
`OnLoaded` returns early precisely on `_templateExecuted > 0` (`:864`). So if
`RichTextBlock.IsLoaded` is false during template application — which is what the guard
exists for — a first-render spoiler is never covered.

Spoilers do work today, so `IsLoaded` is presumably already true at that point and the
guard is dead weight; worth confirming, then either dropping the guard or setting a
`_highlightersPending` flag that `OnLoaded` honours. As written the behaviour depends on
an undocumented ordering.

Took the flag rather than dropping the guard, since that is the option that is right
either way: if `IsLoaded` is already true at template time nothing changes at all, and if
it isn't, the highlighters now arrive on load instead of never. The ordering question
itself stays open — this makes the answer stop mattering.

  • Date-driven spoiler fix-up loses deltas and never touches the ranges — :2508-2519 [latent] [live] → fixed on the formatted-text-block-dates branch

    `TextDate.Update` re-derives each spoiler from `OriginalOffset` plus *its own* delta, so
with two relative dates before the same spoiler the second update overwrites the first's
shift. And the highlighter ranges themselves are left alone (the commented-out block at
`:2521-2532`), so the transparent range drifts off the text whenever a date changes
length (`"59 seconds ago"` → `"1 minute ago"`).

**Marked [latent] on a wrong reading.** I traced where `TextEntityTypeDateTime` is
constructed, found only Unigram's own sites (`TryParseDateTime` on a `tg://date` link and
the `x-tl-field-tags` clipboard reader, both feeding the composer, plus `PageBlockHelper`
for Instant View) and concluded the read view only saw dates in IV. It is a **TDLib type**
— `td_api.tl:5759` — so it arrives in ordinary message entities, and this is live on the
message path.

That also promotes a third consumer nobody was tracking: `_indexMap`. A date's segment
records `FormattedText.Length` at build time, so once the date is rewritten every segment
after it is off — which is selection and copy in any message containing a relative date,
independent of spoilers.

The three consumers are fixed by two different mechanisms, because they want different
things:

- **Geometry** (`_spoilers`) is now *derived*, not stored. `TextStyleSpoiler` keeps source
  offsets and `DisplayedRange` computes the displayed range from the paragraph's runs at
  the point of use: dates ending before the spoiler push it along, a date inside it
  stretches it. Any number of dates, any order, no state to drift. The whole patch loop in
  `TextDate.Update` is gone, along with the second constructor and the `dates` accumulator
  in `SetText`.
- **Rendered space** (`_indexMap`, and the spoiler/marked/cached ranges) is *shifted*, by
  `ShiftRenderedSpace(segment, delta)`. Each date carries the index of its own map segment,
  captured when `SetText` mapped it, so a tick moves exactly what is downstream of it. The
  delta is measured **since the last tick** — the old code measured against `Entity.Length`,
  the source length, which is the total growth since first render, so applying it every
  tick and once per date compounded.

**A spoiler cannot actually contain a date**, found while building the test set: send one
that does and the server splits the spoiler around the date
(`spoiler("A ") date("X") spoiler(" B")`). So the "a date inside it stretches it" half of
`DisplayedRange`, and the stretch branch of `ShiftRanges`, are never taken for a date. They
stay live for the marked and cached highlighters, which the app builds itself and which can
span anything — but for spoilers the real case is the *split*, two covers either side of a
date, where the one in front must hold still while the one behind moves. That is what T10
in the test plan sends.

Two things fixed on the way, both from the same root:

- A spoiler's rendered range was `Length = entity.Length` — the *source* length. A spoiler
  wrapping a date renders longer than its source, so the cover was short from the first
  render. The range is now measured from what was actually emitted (`offset` before and
  after), which is right for emoji and math inside a spoiler too.
- The two branches of `UpdateSpoilers` disagreed about which space the offset was in: the
  block branch measures against the date-expanded `GetParts` text, the inline branch
  against raw `_text.Text`. The stored offset carried the date shift, so it was wrong in
  the inline branch by construction. Deriving per branch settles it.

`// TODO: get rid of _spoiler` (`:1580`) is still the better end state — none of the
rendered-space shifting would be needed if the cover came from the geometry — but that is
a design change, and this makes the current design correct.

  • Nothing guarantees a block ever releases its relative dates — MessageTextBlock.cs:294, FormattedTextBlock.cs:909 [live] — raised by Fela, not found by the review → fixed in the commit that checked this box

    The review checked that `ClearEntities` unsubscribes and stopped there. It never asked
what guarantees `ClearEntities` runs, and for the blocks inside a `MessageTextBlock`,
nothing does. `ClearBlocks` drops them with `Children.Clear()` and leaves teardown to the
`Unloaded` event — its own comment says so.

`FrameworkElementEx` only raises `OnUnloaded` for an element it saw `Loaded` on
(`FrameworkElementEx.h:37-49`). A block subscribes during `SetText`, which runs from
`OnApplyTemplate`, which runs on the first **measure** — before `Loaded`. A block measured
and then dropped before its `Loaded` arrives therefore never releases anything, and what
it holds is a registration in the `[ThreadStatic]` `RelativeDateService`: that pins the
block, its `StyledParagraph`, its `TextStyleRun` and the run object for the rest of the
session, and the timer goes on ticking it and calling `RegisterLayoutChanged` on a control
that is not in the tree. Its runs never return to the pool either.

**The half that needs no assumption about `Loaded`:** the service is keyed by the *run
object*, and runs come from the shared pool. `SubscribeImpl` returned early when the key
already existed, so a single registration that outlives its block makes that pooled run
**permanently unsubscribable** — every later block that dequeues it has its date silently
never update. One leak poisons a pooled object for the session.

Fixed at both ends: `ClearBlocks` calls `Clear()` on each block before dropping it, so
teardown is deterministic instead of event-dependent, and `SubscribeImpl` replaces the
entry rather than skipping, since a run being resubscribed always means the old
registration is dead.

Left alone: the runs still return to the pool via `OnUnloaded` only. A missed pool return
costs an allocation, not a pinned graph, and making `Clear` recycle as well would put two
paths on the same pool — worth doing only with the recycling audit, not blind.

P3 — cleanup, naming, dead code

  • The two GetOrCreateRun overloads are the same 80 lines twice — :618-699 and :701-782. The only difference is text.Substring(offset, length) vs text on the pooled path; the non-pooled path already forwards to two NativeUtils overloads. The range overload can call the other with the substring, or both can share a private ApplyRunProperties(direct, run, ...). → fixed in the commit that checked this box

    Took the shared helper, not the forwarding, because forwarding would have cost the range
overload its one real advantage: on the path that builds a new `Run`, `NativeUtils` takes
the offset and length and never materializes the substring. Each overload keeps its own
`Run_Text` line and its own native call, and the 60 lines of property resetting between
them is now written once. Net -59 lines, no behaviour change.

  • Clear() has no callers — Clear() doesn't clear — :386-396 [live] → fixed in the commit that checked this box

    **This entry was wrong.** `Clear()` is not dead: `MessageService.Recycle` calls it on the
`Text` block of every service message (`MessageService.cs:121-124`), and
`MessageGiftContent` calls it on `Subtitle`. The original grep looked for the control's
field names and missed a `FindName("Text") is FormattedTextBlock content` and a
`Subtitle.Clear()`.

That turns the second half of the note from a hypothetical into a live bug, and against
the contract written directly above the call site — *"whatever isn't reset is inherited by
it"*. `Clear` nulled `_query` and `_spoiler` but never touched
`TextBlock.TextHighlighters`, and left `_cached`/`_marked`/`_selection` entirely, so a
recycled service-message container could show the **previous** message's spoiler cover and
highlights over the new text.

Fixed by dropping all four sources and calling `ApplyHighlighters`, which is what takes
them off the control.

  • The SetQuery(string.Empty) calls in the cells are now no-ops — ChatCell.xaml.cs:1526, ForumTopicCell.xaml.cs:557, BusinessChatLinkCell.xaml.cs:37, ChatPinnedMessage.xaml.cs:422, MessageReply.xaml.cs:433, ProfileHeader.xaml.cs:1054/1391. With _query null, the guard at :466 returns immediately; they only existed to trigger the old "apply highlighters" pass. → closed as keep, no code change

    They are only no-ops because nothing gives those controls a query today — the one live
query in the app is `MessageBubble` → `MessageTextBlock` (`:147`). Each of these sits on
a recycle path, where the call is the reset that would matter the moment a query reached
a cell, and where it costs one string comparison that returns immediately. Deleting seven
lines across seven files to save that is the wrong trade.

  • SetFontSize only reaches the first paragraph — :451-459. It sets TextBlock.Blocks[0].FontSize, so a multi-paragraph block keeps the old size on paragraphs 2..n; and on the fast path the Run carries its own FontSize (:977), which wins over the paragraph's, making the call a no-op there. → closed as by design (Fela), no code change

    The method exists for the mockup bubbles only: its single caller is
`MessageBubble.UpdateMockup` (`MessageBubble.xaml.cs:3712`, the call at `:3770`), which
renders the fake conversation in appearance settings where the font-size slider has to
take effect without going back through `SetText`. Those messages are one plain paragraph
each, so "the first paragraph" is the whole text and there is nothing for a loop to reach.

Which also settles the quote half: `SetFontSize` never sees a quote, so it cannot clobber
the caption size, and the TODO at `:1127` stays a question about `SetText` alone.

  • Selectable.cs's header comment contradicts WalkInlines — Selectable.cs:38-40 says the highlighter space counts "1 per inline object (custom emoji, image, math)", but case InlineUIContainer: break; (:394-395) counts 0, which is what SetText's Map(..., 1, entity.Length) // emoji (container=0 rendered) + ZWNJ (:1406) assumes. The code is self-consistent; the comment is the thing that will mislead. → fixed in the commit that checked this box; the comment now says an inline object counts 0 and explains why (the ZWNJ beside it is the unit that stands in for it).

  • Inline mode misses textOffset on the query highlight — :505 vs :1280 [latent]. The spoiler branch adds _spanForInlines.ContentStart.OffsetToIndex(TextBlock) to its ranges; the query branch doesn't, so a search highlight in a ChatCell brief would land short by the length of the "Fela: " prefix. Latent only because no caller passes a non-empty query to an inline-mode block today (the only real query is MessageBubble → MessageTextBlock). → fixed in the commit that checked this box

    Fixed to match the spoiler branch. Worth knowing it is **unexercised**: no caller and
no test reaches it, so it is right by symmetry with code that works, not by
observation. If chat-list search highlighting is ever wired up, check that line first.

  • RenderedToStyled/StyledToRendered are linear — Selectable.cs:278/311. One segment per run, scanned per pointer move during a drag (GetSelectionBoundary, GetSelectedText). Segments are sorted and non-overlapping in both spaces, so a binary search is a two-line change; worth it for long code blocks, where the segment count is the token count. → closed as not worth it, no code change

    The last sentence was wrong, and it was the whole argument. `_indexMap` is built by
`SetText`'s entity loop only — `ProcessCodeBlock` adds spans and runs but never segments,
so a syntax-highlighted block contributes **one** segment, not one per token. Real maps run
to a handful of entries, where a linear scan beats a binary search. Left alone.

  • WalkInlines allocates an enumerator per level, per pointer move — Selectable.cs:367. foreach over InlineCollection goes through the projected IEnumerable<Inline>. Indexing is not obviously better (each [i] is its own interop call) — measure before changing, but note it sits on the same path _contentLength (:85-89) was cached for. → left open: the one item here that wants a profiler.

    Both shapes are interop-bound and which wins depends on how the projection caches its
enumerator, which is not something to settle by reading. The cheap experiment is a drag
across a syntax-highlighted code block — the deepest inline tree the app builds, and the
case `_contentLength` was already cached for.

  • OnHyperlinkForegroundChanged and OnCodeForegroundChanged are identical — :2252-2261 and :2281-2290. Both walk all hyperlinks and recolor those whose Foreground matches the old value, so if the link brush and the code brush are ever the same instance, one property change recolors both kinds. Tag the hyperlink kind, or keep the code links in their own list. → merged into one RecolorHyperlinks in the commit that checked this box

    Only the duplication is fixed. Tagging the kind would mean a second list on a hot path to
close a case that needs the two brushes to be the *same instance*, which no style in the
app does — so the comment says it instead of the code paying for it.

  • Naming, while in here — var hyperlink = GetOrCreateSpan(direct) for spoilers (:1201, :1268), foreach (var hyperlink in _spoilers) over TextStyleSpoiler structs (:1732, :1796), and TextStyleRun Yolo in the RelativeDateService signatures (:2559, :2565). → renamed to span, spoiler and run in the commit that checked this box

    `GetNextUpdateInterval` still updates the run texts as a side effect of a `Get`, which is
more than a rename — that call is the only thing driving the updates at all, so moving it
means restructuring the timer, not renaming a method.

  • Blocks + re-templating — :208-212 / :231-240. _blocks is never cleared after its paragraphs move into TextBlock.Blocks, and the loop casts with as Paragraph without a null check, so a second OnApplyTemplate (theme/style change) would either re-parent the same Paragraphs or add null. → fixed in the commit that checked this box

    `_blocks` is dropped once handed over, which is safe because `Blocks` reads through to
`TextBlock.Blocks` from that point on, and a non-`Paragraph` entry is skipped rather than
added as null.

  • TextBlock is assumed non-null outside SetText — MeasureOverride:263, UpdateSpoilers:1713, InvalidateSkeleton:2426. All three read TextBlock.FontSize only when AutoFontSize is false, which is why it hasn't fired; SetText guards with _templateApplied but these don't. → guarded in MeasureOverride only (with the measure-memo commit); the other two closed as unreachable

    `UpdateSpoilers` and `InvalidateSkeleton` run only from `OnLayoutUpdated`, which fires from
a `LayoutUpdated`/`SizeChanged` registration made on `m_textBlock` itself
(`FormattedTextBlockBase.cpp:37-43`). With no template child, `RegisterLayoutChanged` would
have dereferenced null in C++ long before either could run. `MeasureOverride` is the one
XAML can reach on its own, so it is the one that got a check.

  • HasLineEnding's InvalidateMeasure is commented out — :173-184, read by MessageBubblePanel.cs:253. Fine while SetText always precedes measure; a SetText after layout (spoiler reveal, relative-date rebuild) won't re-measure the bubble. → left open. Uncommenting it is one line, but it puts an InvalidateMeasure on a property that SetText writes on every render, and the panel that reads it is the message bubble's own layout. Whoever commented it out was most likely avoiding exactly that, and the layout-cycle audit is the place that question belongs.