Fix StackViewer: "Set Time Range" resets "Goto Items by callees" #2208

Copilot · 2025-05-22T23:34:46Z

Issue Description

When users perform the following steps in PerfView Stack Viewer:

Open CPU stacks
Go to some node in "call tree"
Press "SHIFT F10" (Goto Items in Callees)
Press "ALT R" (Set Time Range) on some time range

The focused node disappears and the view resets to the root node. This worked correctly in version 3.1.19 but was a regression in 3.1.20.

Root Cause

The regression was caused by the Update() method called during DoSetTimeRange, which then calls SetStackSource. This in turn calls SetFocus(m_callTree.Root), resetting the focus to the root node and losing the previously focused node.

Fix

The fix preserves the focused node when setting a time range:

Before calling Update() in the DoSetTimeRange method, we save the current focus node name
After Update() completes, we restore the focus to the previously saved node name

This is a minimal, targeted fix that only affects the time range setting operation without changing behavior elsewhere.

Testing

Enhanced existing tests to verify that focus is preserved when setting a time range
Added a new test specifically for the "Goto Items in Callees" scenario to prevent regressions

Fixes #2179.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

crl3.digicert.com
- Triggering command: dotnet build (dns block)
crl4.digicert.com
- Triggering command: dotnet build (dns block)
ocsp.digicert.com
- Triggering command: dotnet build (dns block)
s.symcb.com
- Triggering command: dotnet build (dns block)
s.symcd.com
- Triggering command: dotnet build (dns block)
- Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node /home/REDACTED/work/_temp/copilot-developer-action-main/dist/index.js (dns block)
ts-crl.ws.symantec.com
- Triggering command: dotnet build (dns block)
ts-ocsp.ws.symantec.com
- Triggering command: dotnet build (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to my firewall allow list

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>

brianrob · 2025-05-23T00:25:30Z

src/PerfView.Tests/StackViewer/StackWindowTests.cs

+                // Now execute Set Time Range command
+                var byNameView = stackWindow.m_byNameView;
+                stackWindow.StartTextBox.Text = "0";
+                stackWindow.EndTextBox.Text = "100";


It would be nice if these values were the minimum and maximum values from the stacksource. Can you please update this?

I've updated the test to use the min and max time values from the stack source rather than hardcoded values. The test now gets the minimum time from the first sample and calculates the maximum time by iterating through all samples in 2687b73.

brianrob · 2025-05-23T00:26:16Z

src/PerfView/StackViewer/StackWindow.xaml.cs


        private void DoSetTimeRange(object sender, ExecutedRoutedEventArgs e)
        {
+            string currentFocusName;


I don't believe that we actually need the currentFocusName change. Can you please undo this part of the change.

I've reverted the currentFocusName variable declaration to its original scope in 2687b73. Each code block now has its own local declaration instead of declaring it at the method level.

…ame scope change Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>

brianrob · 2025-05-23T00:33:15Z

src/PerfView/StackViewer/StackWindow.xaml.cs

                        var bucketEndIndex = (HistogramCharacterIndex)(bucketStartIndex + selectionLen);

+                        // Remember the current focus node name before updating
+                        string currentFocusName = FocusName;


Please revert all of the current changes that touch currentFocusName, not just the scope change.

I've reverted all changes related to currentFocusName in commit 932a7f6, including the variable declarations and the code that was added to preserve focus when setting time range.

Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>

brianrob · 2025-05-23T01:11:28Z

This issue has been a good experiment for testing out copilot. A few notes:

Copilot did a great job of getting me setup to write a unit test. It wasn't 100% right, but it was close. And when I wasn't sure how I wanted to set the time range, it did it for me modulo a compile error.
It didn't do great at the code change itself, but the test really helped me to find what was missing and it was faster for me to write the few lines of code at that point.

Overall, this is a stronger change than I would have otherwise brought because I likely would not have written a test case.

NOTE: Copilot can't build the source code right now because it must be built on Windows, and copilot runs on Linux. #2207 should at least help with some of that.

src/PerfView/StackViewer/StackWindow.xaml.cs

Co-authored-by: leculver <8496639+leculver@users.noreply.github.com>

Copilot · 2025-05-23T01:23:50Z

This issue has been a good experiment for testing out copilot. A few notes:

Copilot did a great job of getting me setup to write a unit test. It wasn't 100% right, but it was close. And when ...

Thanks for the feedback on how Copilot performed on this task. Testing is indeed a valuable part of the development process, and I'm glad I could help with that aspect. The Linux/Windows build compatibility issues are understandable limitations - looking forward to the improvements in #2207.

marklio · 2025-05-23T01:16:43Z

src/PerfView.Tests/StackViewer/StackWindowTests.cs

+        [UseCulture("en-US")]
+        public Task TestSetTimeRangeAfterGotoCalleesAsync()
+        {
+            Func<Task<StackWindow>> setupAsync = async () =>


OPINION: these would be much more readable as inline functions instead of rendered as delegates

I'm actually not sure why these are inline at all. They don't seem to be capturing anything

Looks like this is maybe a "when in Rome" situation :)

Probably - there are several others that follow this pattern.

Initial plan for issue

f7dcdf9

Copilot AI assigned Copilot and brianrob May 22, 2025

Copilot started work on behalf of brianrob May 22, 2025 23:34 View session

Fix StackViewer: preserve focus when setting time range

8d937e8

Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Stack Viewer: "Set Time Range" resets "Goto Items by callees" (etc)~~ Fix StackViewer: "Set Time Range" resets "Goto Items by callees" May 22, 2025

Copilot AI requested a review from brianrob May 22, 2025 23:40

Copilot finished work on behalf of brianrob May 22, 2025 23:40

brianrob added 2 commits May 22, 2025 17:23

Clean-up compiler errors and remove the breaking call to SetFocus.

533aee1

Fix unit test implementation.

59b64a0

brianrob reviewed May 23, 2025

View reviewed changes

Copilot started work on behalf of brianrob May 23, 2025 00:26 View session

Update test to use stacksource min/max times and revert currentFocusN…

2687b73

…ame scope change Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>

Copilot finished work on behalf of brianrob May 23, 2025 00:31

brianrob reviewed May 23, 2025

View reviewed changes

Copilot started work on behalf of brianrob May 23, 2025 00:33 View session

Revert all changes related to currentFocusName

932a7f6

Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>

Copilot finished work on behalf of brianrob May 23, 2025 00:36

brianrob added 2 commits May 22, 2025 18:03

Keep track of the focus node and set focus again after the update.

b956f09

Fix-up test compile errors

23c7c16

brianrob marked this pull request as ready for review May 23, 2025 01:14

brianrob requested review from cincuranet, leculver, marklio and mconnew as code owners May 23, 2025 01:14

leculver previously approved these changes May 23, 2025

View reviewed changes

src/PerfView/StackViewer/StackWindow.xaml.cs Outdated Show resolved Hide resolved

Copilot started work on behalf of leculver May 23, 2025 01:22 View session

Use null-coalescing operator for FocusName

d46e3a4

Co-authored-by: leculver <8496639+leculver@users.noreply.github.com>

Copilot AI dismissed leculver’s stale review via d46e3a4 May 23, 2025 01:23

leculver approved these changes May 23, 2025

View reviewed changes

Copilot finished work on behalf of leculver May 23, 2025 01:24

marklio approved these changes May 23, 2025

View reviewed changes

brianrob enabled auto-merge (squash) May 23, 2025 01:26

brianrob merged commit 646d07a into main May 23, 2025
5 checks passed

brianrob deleted the copilot/fix-2179 branch May 23, 2025 01:32

Fix StackViewer: "Set Time Range" resets "Goto Items by callees" #2208

Fix StackViewer: "Set Time Range" resets "Goto Items by callees" #2208

Conversation

Copilot AI commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Description

Root Cause

Fix

Testing

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

brianrob May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 23, 2025

Choose a reason for hiding this comment

Uh oh!

brianrob May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 23, 2025

Choose a reason for hiding this comment

Uh oh!

brianrob May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 23, 2025

Choose a reason for hiding this comment

Uh oh!

brianrob commented May 23, 2025

Uh oh!

Uh oh!

Copilot AI commented May 23, 2025

Uh oh!

marklio May 23, 2025

Choose a reason for hiding this comment

Uh oh!

marklio May 23, 2025

Choose a reason for hiding this comment

Uh oh!

marklio May 23, 2025

Choose a reason for hiding this comment

Uh oh!

brianrob May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented May 22, 2025 •

edited

Loading