Skip to content

Stack Trace Explorer improvements#58458

Merged
ryzngard merged 10 commits intodotnet:mainfrom
ryzngard:issues/activate_stacktrace_on_focus
Dec 28, 2021
Merged

Stack Trace Explorer improvements#58458
ryzngard merged 10 commits intodotnet:mainfrom
ryzngard:issues/activate_stacktrace_on_focus

Conversation

@ryzngard
Copy link
Copy Markdown
Contributor

@ryzngard ryzngard commented Dec 22, 2021

  • Add option to open explorer automatically if a callstack is in the clipboard when VS becomes the active window
  • Add close button
  • Add "clear" button (clears current tab only)

a31744ac-1a1c-4659-b7e3-78a6e8b9b85d

image

TODO:

  • Expose option to user
  • Cleanup strings
  • Make screenshots for this PR

@ryzngard ryzngard requested a review from a team as a code owner December 22, 2021 05:47
Copy link
Copy Markdown
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

I skipped reviewing the xaml files since I have nothing to contribute. Curious what the scenario is for clear vs close? Or is clear actually "Close All"?

Copy link
Copy Markdown
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

I'm not sure I'm convinced the Clear button is necessary, versus just closing a tab, but it's up to you.

<data name="Prefer_extended_property_pattern" xml:space="preserve">
<value>Prefer extended property pattern</value>
</data>
<data name="String1" xml:space="preserve">
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oops

<value>Sort usings</value>
</data>
<data name="Automatically_open_stack_trace_explorer_on_focus" xml:space="preserve">
<value>Automatically open Stack Trace Explorer on focus</value>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm guessing its too long to put "when a stack trace is on the clipboard" on the end here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think so. I wonder where we would put explainers like this. Honestly might be worth a short paragraph below the checkbox explaining the behavior, but I don't think we normally do that.

/// Returns true if there's a tab that already matches the text
/// </summary>
public bool ContainsTab(string text)
=> Tabs.Any(tab => tab.Content.ViewModel.Matches(text));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this slow and/or does it need the UI thread? I wonder what the trade off here is, if you have a lot of tabs, versus just keeping the most recent clipboard text seen in memory, and rejecting that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's pretty fast just because it's a hashcode comparison. It doesn't need UI thread as long as the underlying Tabs isn't being modified...

@ryzngard
Copy link
Copy Markdown
Contributor Author

I'm not sure I'm convinced the Clear button is necessary, versus just closing a tab, but it's up to you.

Yea, this was just taken from feedback. We'll see what people think. It's present in the right click menu as well (plus paste)

@ryzngard ryzngard enabled auto-merge (squash) December 28, 2021 07:35
@ryzngard ryzngard merged commit 3a2a2a6 into dotnet:main Dec 28, 2021
@ghost ghost added this to the Next milestone Dec 28, 2021
@Cosifne Cosifne modified the milestones: Next, 17.1.P3 Jan 5, 2022
@ryzngard ryzngard added UX Review Not Required UX Review Not Required and removed Needs UX Triage labels Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants