Skip to content

Feature/plan viewer improvments 202604 p1#162

Open
rferraton wants to merge 2 commits intoerikdarlingdata:devfrom
rferraton:feature/plan-viewer-improvments-202604-P1
Open

Feature/plan viewer improvments 202604 p1#162
rferraton wants to merge 2 commits intoerikdarlingdata:devfrom
rferraton:feature/plan-viewer-improvments-202604-P1

Conversation

@rferraton
Copy link
Copy Markdown
Contributor

What does this PR do?

  • Allow to zoom in/out in humanadvices using mousewheel (my eyesight is not what it used to be)
  • Make the zoom on planviewer focused/centered on the mouse pointer
  • Make any node quoted in the human advices window clickable and navigate to the node. The plan become centered and focus (zoomed) on the selected node

Which component(s) does this affect?

  • Desktop App (PlanViewer.App)
  • Core Library (PlanViewer.Core)
  • CLI Tool (PlanViewer.Cli)
  • SSMS Extension (PlanViewer.Ssms)
  • Tests
  • Documentation

How was this tested?

2026-04-04_23h55_41

Describe the testing you've done. Include:

  • Plan files tested : both estimated and actual
  • Platforms tested Windows only

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (dotnet build -c Debug)
  • All tests pass (dotnet test)
  • I have not introduced any hardcoded credentials or server names

- Make the zoom on planviewer focused/centered on the mouse pointer
…te to the node. The plan become centered and focus (zoomed) on the selected node
Copy link
Copy Markdown
Owner

@erikdarlingdata erikdarlingdata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseded

Copy link
Copy Markdown
Owner

@erikdarlingdata erikdarlingdata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice features — mouse-centered zoom, advice zoom, and clickable node refs are all solid UX improvements. A few things to address before merging:

1. Mouse-centered zoom bypasses SetZoom

The new wheel handler in PlanScrollViewer_PointerWheelChanged sets _zoomLevel, _zoomTransform.ScaleX/Y, and ZoomLevelText.Text directly instead of calling SetZoom(). This means any future changes to SetZoom won't apply to the wheel zoom path.

Suggestion: refactor SetZoom to accept an optional mouse position (or add a SetZoomAtPoint overload) and call it from the wheel handler.

2. ShowAdviceWindow is now duplicated with even more code

Both MainWindow.cs and QuerySessionControl.cs had identical ShowAdviceWindow methods before this PR, and now both get the same ~20 lines of zoom handling and onNodeClick wiring copy-pasted. This makes future changes risky (easy to fix one and forget the other).

Would you consider extracting ShowAdviceWindow to a shared helper (e.g., AdviceWindowFactory.Show(...)) as part of this PR? If not, at least add a comment in both copies pointing to the other.

3. Cursor allocation on every PointerMoved

In WireNodeClickHandler, new Cursor(StandardCursorType.Hand) is called on every mouse move event when hovering over a link. This creates a new object per frame. Cache it as a static field alongside LinkBrush:

private static readonly Cursor HandCursor = new(StandardCursorType.Hand);

4. MakeNodeRefsClickable complexity

The recursive tree walker is ~250 lines handling Panel, Border, Expander, SelectableTextBlock (with inlines), and SelectableTextBlock (with plain text). This works but is fragile — any new container type in AdviceContentBuilder.Build would need to be added here too.

An alternative: build the clickable Runs inline during Build() where you already know the text content, instead of post-processing. You already have the onNodeClick callback at that point. This would eliminate the tree walk entirely. Up to you whether to tackle that now or in a follow-up.

Minor

  • NavigateToNode looks clean, no issues there.
  • The advice window zoom range (0.5x-3.0x) and step (0.1) feel good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants