[pmeems]

Phase 5 adds ActiveX navigation on top of the programmatic Core work in PR #38.

The control exposes stable layer IDs, layer visibility/removal, zoom-to-map/layer, and center zoom through a new IMapWinGISControl4, preserving the published interfaces. Mouse-wheel zoom uses cursor anchoring; drag-to-pan uses the windowless site’s capture lifecycle. New dataset loads preserve the current view, with ZoomToMap available to include them.

The C# host exercises COM navigation, layer operations, wheel zoom, drag capture/release, and resizing. No dependencies were added. Deep zoom clipping and layer reordering remain follow-up limitations.

[Julian-MW]

Source review of Phase 5 (ActiveX navigation). The overall design checks out: the IMapWinGISControl4 derivation preserves the published-interface policy, dispatch IDs are contiguous (1–16, no collisions), the pan sign convention and wheel anchoring match Core's Viewport math, and the first-load fit/pending-fit semantics in Core's MapView::AddLayer line up with the documented "first layer fits, later loads preserve" behavior.

Five inline comments, all minor/consistency-level — nothing blocking:

1. SetLayerVisible/get_LayerCount/GetLayerId lack the exception-to-IErrorInfo translation used by the other new navigation methods.
2. Mouse-wheel/pan failures are silently swallowed (OutputDebugStringA only) while the message is still consumed.
3. RemoveLayer decrements the cached _featureCount with the layer's current feature count rather than the count captured at load, which can leave the total overstated if the source changed in between (the guard only catches the opposite direction).
4. WaitForRepaintAsync relies on a fixed 150 ms sleep + DoEvents — timing-dependent.
5. VerifyImagesDiffer's 100-pixel floor can false-fail on a resolution-limit clamp (correct changed = false → identical frame) and compares the whole window including chrome.

Happy to re-check anything after follow-ups.

[Julian-MW]

The fixed 150 ms delay plus Application.DoEvents makes the repaint wait timing-dependent: on a slow or heavily loaded desktop session the repaint may not be complete when the capture runs, and DoEvents pumps unrelated messages while waiting. Consider polling the captured frame until it is stable (or waiting on an explicit rendered-frame signal) instead of a hard-coded sleep.

[Julian-MW]

If ZoomAt throws here, the failure is only written to the debug output and the message is still consumed (handled = TRUE), so the host has no way to observe that the wheel zoom failed — unlike the COM surface, which reports errors through IErrorInfo. The same applies to the pan path in OnMouseMove. Not a blocker, but consider routing these through a shared trace/error helper (or at least documenting that mouse-navigation failures are intentionally silent).

[Julian-MW]

The five navigation methods above (ZoomToMap, ZoomToLayer, ZoomIn, ZoomOut, RemoveLayer) all translate Core exceptions into HRESULTs via SetNavigationErrorInfo, but SetLayerVisible, get_LayerCount, and GetLayerId call Core directly without a guard. Today those Core paths are noexcept or bounds-checked so nothing escapes, but the pattern is now inconsistent — if any of these Core calls ever gains validation that throws (e.g. MapView::SetLayerVisible), a C++ exception would cross the COM boundary. Could you wrap these three the same way for consistency?

[Julian-MW]

_featureCount is incremented at load time with the feature count read then (via the render scene), but here it is decremented with the layer's current dataset feature count. If the source layer's feature count changed between load and removal (Core allows the same source layer to be added more than once and edits to source geometry), the decrement can under-subtract and leave FeatureCount permanently overstated — the guard above only catches the over-subtract case (featureCount > _featureCount). Consider storing the per-layer count captured at load alongside the layer, or recomputing the total from the remaining layers after removal.

[Julian-MW]

Two robustness concerns with this check: (1) if a wheel step lands exactly at the resolution limit, Core's ZoomAt correctly returns changed = false and the frame is identical — changedPixels will be 0 and this throws, turning a legitimate clamp into a test failure; (2) the comparison covers the entire captured window, including any non-map chrome, so an unrelated pixel change could also satisfy the threshold. Consider gating on the operation having reported a change (or comparing only the map client area) rather than a raw 100-pixel floor.