Add multi line viewer for Event Window #2175
Conversation
This will add a multi line viewer to the Event Viewer, which can be turned on and off with F2
|
@microsoft-github-policy-service agree |
|
Hi, I am a student and I don't have much experience yet. I hope my code is fine and I would be happy about some feedback. |
src/PerfView/EtwEventSource.cs
Outdated
| @@ -552,7 +554,12 @@ internal ETWEventRecord(ETWEventSource source, TraceEvent data, Dictionary<strin | |||
|
|
|||
| m_timeStampRelativeMSec = data.TimeStampRelativeMSec; | |||
| m_idx = data.EventIndex; | |||
| m_payloads = new Payload[data.PayloadNames.Length]; | |||
There was a problem hiding this comment.
I think it would be good to change the type of m_payloads to a List<Payload> so that you can add new items to it after iterating over the fields in the event. If you look at ETWEventRecord..ctor, there are a number of conditional calls to AddField. It would be ideal to add these fields to m_payloads as well, so that what's in the single line view is also in the multi-line view. Bonus points if the order matches.
src/PerfView/EtwEventSource.cs
Outdated
| @@ -666,7 +673,8 @@ internal ETWEventRecord(ETWEventSource source, TraceEvent data, Dictionary<strin | |||
| public DateTime OriginTimeStamp { get { return TimeZoneInfo.ConvertTime(LocalTimeStamp, this.m_source.OriginTimeZone); } } | |||
| public override string Rest { get { return m_asText; } set { } } | |||
| public EventIndex Index { get { return m_idx; } } | |||
|
|
|||
| public override Payload[] Payloads { get { return m_payloads; } } | |||
There was a problem hiding this comment.
If you change m_payloads to be a List<Payload>, you can call .ToArray() here if an array is required for binding (not sure). If not, you can keep it as a List and let it be enumerated by the GUI framework.
| @@ -17,6 +17,7 @@ | |||
| </Window.Resources> | |||
| <DockPanel Background="{DynamicResource BackgroundColour}"> | |||
| <DockPanel.CommandBindings> | |||
| <CommandBinding Command="{x:Static src:EventWindow.ToggleMultiLineViewPaneCommand}" Executed="DoToggleMultiLineViewPane" /> | |||
There was a problem hiding this comment.
I think we need a way to make sure users know how to toggle this view. Would it be possible to show some text in the window that says something like "Press F2 to hide." whenever there is not any data shown?
| @@ -17,6 +17,7 @@ | |||
| </Window.Resources> | |||
| <DockPanel Background="{DynamicResource BackgroundColour}"> | |||
| <DockPanel.CommandBindings> | |||
| <CommandBinding Command="{x:Static src:EventWindow.ToggleMultiLineViewPaneCommand}" Executed="DoToggleMultiLineViewPane" /> | |||
There was a problem hiding this comment.
Would it be possible to make the new window take up the bottom 25% of the window by default? I think 50% is likely too much.
|
|
||
| if (value) | ||
| { | ||
| App.UserConfigData["MultiLineViewPaneHidden"] = "true"; |
There was a problem hiding this comment.
This is great. Thanks for keeping track of the user's preference across runs.
|
@caaavik-msft, would you mind reviewing this and see if you think there might be any changes necessary from an accessibility perspective (or otherwise)? |
-Change m_payloads from Array to List -Add other fields from conditional calls to AddField -Add "Press f2 to hide/unhide" text -Set default size of multi line viewer to 25% -Fix scroll behaviour
|
Hi, I've changed some things according to your comments. I hope I could address all your comments properly. While I was implementing the changes I also noticed that the behaviour of the scrollbar in the multi-line viewer was not optimal. It always jumped to the next row of the grid, which can be problematic for cells bigger than the multi-line viewer itself, because it would jump over content and make it impossible for the user to see it. For this reason I changed the scrolling behaviour from logical scrolling to physical scrolling. |
Co-authored-by: Cameron Aavik <[email protected]>
|
This is looking good. @caaavik-msft do you want to see any additional changes? |
Removing unnecessary INullOrEmpty() call. Co-authored-by: Brian Robbins <[email protected]>
This will add a multi line viewer to the Event Viewer, which can be turned on and off with F2.
Issue #2158