New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend Debugging tools: Event Viewer #1888

Merged
merged 4 commits into from Sep 16, 2018

Conversation

Projects
None yet
2 participants
@siegfriedpammer
Contributor

siegfriedpammer commented Sep 11, 2018

This template is not intended to be prescriptive, but to help us review pull requests it would be useful if you included as much of the following information as possible:

  • What does the pull request do?
    It extends Avalonia.Diagnostics. It adds an event viewer:
    grafik
  • What is the current behavior?
    No such feature exists.
  • What is the updated/expected behavior with this PR?
    Event routes can be analyzed.
  • How was the solution implemented (if it's not obvious)?
    I added a new tab to the F12 debugging tools.

Checklist:

If the pull request fixes issue(s) list them like this:

None

@siegfriedpammer siegfriedpammer changed the title from Initial commit of Event Viewer. to Extend Debugging tools: Event Viewer Sep 11, 2018

@grokys

Thanks for this - it's going to be really useful. Just a few comments, mainly naming-related but a few other things I noticed.

namespace Avalonia.Diagnostics.Models
{
internal class ChainLink

This comment has been minimized.

@grokys

grokys Sep 13, 2018

Member

This isn't a very descriptive name, it should probably be EventChainLink or something to give a clue that it relates to the Events tab.

this.IsExpanded = true;
}
RoutedEvent[] defaultEvents = new RoutedEvent[]

This comment has been minimized.

@grokys

grokys Sep 13, 2018

Member

This can be made static readonly I think?

This comment has been minimized.

@grokys

grokys Sep 13, 2018

Member

Also fields should be declared at the top of the class.

public bool Handled { get; private set; }
public RoutingStrategies Route { get; private set; }
public ChainLink(object handler, bool handled, RoutingStrategies route)

This comment has been minimized.

@grokys

grokys Sep 13, 2018

Member

Nit: constructor should come before properties.

namespace Avalonia.Diagnostics.ViewModels
{
internal class ControlTreeNode : EventTreeNode

This comment has been minimized.

@grokys

grokys Sep 13, 2018

Member

Could there be a clearer name for this? Maybe something like EventOwnerTreeNode or something to a clue that it's part of the events tree?

public ControlTreeNode(Type type, IEnumerable<RoutedEvent> events, EventsViewModel vm)
: base(null, type.Name)
{
this.Children = new AvaloniaList<EventTreeNode>(events.OrderBy(e => e.Name).Select(e => new EventEntryTreeNode(this, e, vm) { IsEnabled = IsDefault(e) }));

This comment has been minimized.

@grokys

grokys Sep 13, 2018

Member

Could you split this into multiple lines? It gets cut off when viewing the file on GitHub.com.

.ToArray();
}
private void ClearExecute()

This comment has been minimized.

@grokys

grokys Sep 13, 2018

Member

Why not just call this Clear?

private void ClearExecute()
{
Action action = delegate

This comment has been minimized.

@grokys

grokys Sep 13, 2018

Member

You could use a local function here rather than an Action.

{
RecordedEvents.Clear();
};
if (!Dispatcher.UIThread.CheckAccess())

This comment has been minimized.

@grokys

grokys Sep 13, 2018

Member

Actually... why would this be called on anything but the UI thread?

}
}
public ChainLink Originator

This comment has been minimized.

@grokys

grokys Sep 13, 2018

Member

Why is this property mutable? As far as I can see it's set in the ctor and never changed.

Show resolved Hide resolved src/Avalonia.Diagnostics/ViewModels/FiredEvent.cs
@grokys

grokys approved these changes Sep 16, 2018

@grokys grokys merged commit fbc4eaa into AvaloniaUI:master Sep 16, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment