Skip to content

Docking: OnFloatingWindowClosed can't be distinguished from cross-window dock-back #417

@codemonkeychris

Description

@codemonkeychris

Summary

DockManager.OnFloatingWindowClosed fires on every Window.Closed event of a docking floating window — including the synthetic close that reactor itself triggers immediately after a cross-window dock-back. A consumer that uses this event to drive per-document state cleanup (dispose per-doc state, drop from an open-keys set, clear "active document" trackers) cannot tell whether:

  1. The user closed the float — content is gone, app should release resources tied to that document, or
  2. The pane was just dock-backed into another host — content is alive and visible in the new dock position, app must not release anything.

Both surface as a single event with the same payload shape:

public sealed class DockFloatingWindowClosedEventArgs
{
    public DockableContent? Content { get; init; }
}

The doc comment on reactor's own raise site even calls this out (DockFloatingWindow.cs:147-152):

Spec 045 §2.6 — paired OnFloatingWindowClosed event. Fires for both user-initiated close and host-unmount close (…); the pane content reference may be stale after a cross-window dock-back already migrated it to another host.

…but the public surface gives the consumer no way to act on that warning.

Trigger sequence

Cross-window dock-back, single-pane floating window, source pane is a Document:

  1. User opens doc D in main host (center group). OnContentDocked fires; app records open-keys + per-doc state in e.g. a Dictionary<string, TFooState> keyed by tag.
  2. User drags D's tab outside the source TabView → wasOutside=true → reactor tears D into a DockFloatingWindow.
  3. User drops the floating window onto the main host's south overlay. The overlay's OnConfirm (DockHostNativeComponent.cs:778-832) runs the cross-window branch (isLocalSource == false):
    • DockLayoutMutator.InsertPaneRelativeToGroup(...) produces a new layout with D in the south position
    • StoreOverride(newLayout) queues the state update
    • manager.OnContentDocked fires with Content = D, Target = south
    • manager.OnLiveLayoutChanged fires
    • DockDragSession.MarkConsumed(), then session.End()
  4. Floating window's onTabDragCompleted callback (DockFloatingWindow.cs:179-186) observes DockDragSession.Consumed == true and calls windowHolder[0]?.Close().
  5. window.Closed fires synchronously → reactor invokes manager.OnFloatingWindowClosed with Content = D — the same DockableContent reference that is now alive in the south group.

From the consumer's vantage point at step 5, the event is indistinguishable from "user clicked X on a floating window holding D."

Consumer-visible failure (real app)

In a real consumer's OnFloatingWindowClosed:

OnFloatingWindowClosed = e =>
{
    if (e.Content?.Key is string key && key.StartsWith("doc:"))
    {
        var closedTag = key.Substring("doc:".Length);
        CloseDocKey(closedTag);   // removes from openKeys,
                                  // disposes per-doc state dict entry,
                                  // clears activeTag if it matched
    }
    ...
};

After cross-window dock-back, CloseDocKey("timeline-rendering") fires. The per-document state dictionary entry is removed. The user clicks the freshly-redocked tab; OnActiveContentChanged runs with the live pane; the shell sets activeTag = "timeline-rendering". The configuration panel does TimelineStates.TryGetValue("timeline-rendering", out _) — miss — and renders "Timeline Rendering has no settings" despite the document being alive and visible.

Worse failure modes are easy to construct: if the per-doc state owns a SwapChainPanel / Win2D resources / a file handle, the dock-back would dispose live resources out from under a still-mounted page.

Why "walk the live layout" isn't a clean workaround

The obvious test — "is the pane still in DockHostModel.Root?" — doesn't work at event time. StoreOverride (DockHostNativeComponent.cs:175) calls a state setter; SyncModelFromElement runs at the top of the next render. By the time OnFloatingWindowClosed fires inside window.Closed inside OnConfirm, the model's Root may still reflect the pre-drop layout.

DockDragSession.Consumed is the right signal but it's internal, and even if it were public, "the most recent drag was consumed somewhere" doesn't pin down which pane was consumed (a multi-pane float that loses one tab to a dock-back leaves Consumed=true even though its close — if it later happens — is a genuine user close).

Current workaround

Track the most recently dock-confirmed key and short-circuit OnFloatingWindowClosed when the keys match:

var recentDockKeyRef = UseRef<string?>(null);

OnContentDocked = e =>
{
    recentDockKeyRef.Current = e.Content.Key as string;
},
OnFloatingWindowClosed = e =>
{
    if (e.Content?.Key is string key && key.StartsWith("doc:"))
    {
        if (recentDockKeyRef.Current == key)
        {
            recentDockKeyRef.Current = null;
            return;     // dock-back, not a real close
        }
        var closedTag = key.Substring("doc:".Length);
        CloseDocKey(closedTag);
    }
    ...
};

Relies on the documented ordering inside OnConfirm: OnContentDockedOnLiveLayoutChangedMarkConsumedEnd → (floating window's TabDragCompleted fires synchronously on stack unwind) → window.Close()OnFloatingWindowClosed. Brittle to any future reordering, and it requires consumers to infer "no dock-back happened recently" rather than reading a clean signal.

Suggested fixes (any one would close the gap)

In rough order of preference:

1. Add a reason discriminator to the event args (preferred)

public enum DockFloatingCloseReason
{
    UserClosed,         // X button, Alt+F4, app-driven Close(),
                        // host-unmount, ...
    MigratedToHost,     // cross-window dock-back into another host
    MigratedToFloat,    // future: re-tear-out into a different float
}

public sealed class DockFloatingWindowClosedEventArgs
{
    public DockableContent? Content { get; init; }
    public required DockFloatingCloseReason Reason { get; init; }
}

Cheap to thread through: DockFloatingWindow.Open's window.Closed handler already knows whether the last drag-completed branch consumed the pane (the DockDragSession.Consumed check that triggers the auto-close at DockFloatingWindow.cs:181-184). Stash the reason on the window holder right before Close() and read it in the Closed handler. Apps then write if (e.Reason == UserClosed) ... and the ambiguity is gone.

2. Split the event in two

OnFloatingWindowClosedByUser and OnContentMigrated (or fold the migration case into the existing OnContentDocked and suppress OnFloatingWindowClosed for that path). Slightly more API churn but removes the chance of consumers ignoring a new Reason field.

3. Public read of last-consumed pane (least preferred)

Document the contract loudly and provide a public DockDragSession.LastConsumedPane snapshot consumers can compare in their handler. Smallest change but pushes ordering knowledge onto every app.

Related observations

  • OnDocumentClosed is not fired for documents that leave the layout via tear-out or cross-window dock-back. Combined with OnFloatingWindowClosed-fires-on-migration, the consumer ends up with one event that always fires on migration plus zero events that only fire on real close — i.e. there is no clean "this document is going away" signal anywhere in the public surface for the float→migrate path. Even if (1) is rejected, it would be worth ensuring that some event uniquely identifies a true close.
  • Similar shape probably applies to ToolWindows that get re-pinned to a side strip via drag rather than via PinToSide. An app that disposed ToolWindow state on close would hit the same class of bug.

Repro

Single-pane floating window + cross-window dock-back, as described above. Reproduces in any consumer that disposes per-doc state in OnFloatingWindowClosed; concretely observed in a WinUI 3 gallery app where Win2D CanvasControl configuration state was wiped on dock-back, leaving the redocked tab visibly alive but its configuration panel reading "has no settings".

Metadata

Metadata

Labels

bugSomething isn't working

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions