Skip to content
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

New and improved DevTools. #3462

Merged
merged 12 commits into from Feb 21, 2020
Merged

New and improved DevTools. #3462

merged 12 commits into from Feb 21, 2020

Conversation

@grokys
Copy link
Member

grokys commented Jan 23, 2020

What does the pull request do?

Re-implements DevTools with a few new features:

  • Adds a built-in console using roslyn scripting which allows running arbitrary code
  • Allow editing of property values
  • Improved display and grouping of control properties
  • Filtering control properties
  • Maintain selection when switching between logical/visual tree

image

Breaking Changes

Avalonia.Diagnostics is now a separate NuGet package so if you're using AttachDevTools you'll have to add a reference to that.

@kekekeks

This comment has been minimized.

Copy link
Member

kekekeks commented Jan 23, 2020

Adds a built-in console using roslyn scripting which allows running arbitrary code

Can we have that part to be pluggable? Preferably shipped in a separate package. So people won't need to ship the entire compiler with the app to use dev tools. Also not that it's not really linker-friendly and definitely not iOS-friendly

@grokys

This comment has been minimized.

Copy link
Member Author

grokys commented Jan 23, 2020

Can we have that part to be pluggable?

We could yeah, but I've got a bunch of stuff to finish that I think are higher-priority (such as ItemsRepeater as ItemsPresenter, fixing selection, fixing ScrollViewer etc) so I'm not sure when I'd be able to do it (this code was mostly already written, I just updated it to master and tidied it up a bit).

Given that, I think there are a few options:

  • Keep current devtools until I can make the console pluggable
  • Merge this devtools but remove the console for now
  • Tell people to #if the call to attach the devtools if they don't want a dependency on roslyn scripting

Not sure what people would prefer?

not that it's not really linker-friendly

Are you talking about CoreRT here?

@Gillibald

This comment has been minimized.

Copy link
Contributor

Gillibald commented Jan 23, 2020

Apps don't rely on DevTools so it's fine if the console breaks on iOS. I am sure there will be a solution for iOS the moment iOS support is stable.

@MarchingCube

This comment has been minimized.

Copy link
Member

MarchingCube commented Jan 23, 2020

I think new DevTools are quite a step forward as it is much more usable interface. Console is also really handy for development. We should give an option to the people to completely remove it, maybe entire diagnostics needs to be pluggable in the end. But for now since we just released new version I think it will be fine to have this for a while until we come up with an idea for separating this feature.

@kekekeks

This comment has been minimized.

Copy link
Member

kekekeks commented Jan 24, 2020

Avalonia.Diagnostics library is a part of Avalonia package. So this change effectively adds a dependency on Microsoft.CodeAnalysis.CSharp.Scripting to every single avalonia-based application.

@Gillibald

This comment has been minimized.

Copy link
Contributor

Gillibald commented Jan 24, 2020

And why exactly do we need a dependency on Avalonia.Diagnostics in the core of Avalonia? I think that is the only issue here. This is not required to run an application.

@grokys

This comment has been minimized.

Copy link
Member Author

grokys commented Jan 24, 2020

Note that Avalonia.Diagnostics now also has a dependency on Avalonia.Control.DataGrid.

Thing is, afaik there's no so thing as "Debug build-only" dependency. You either have the dependency or you don't. The best you can do it to not reference code in the dependency when you don't want to ship it. Or am I mistaken?

@grokys

This comment has been minimized.

Copy link
Member Author

grokys commented Jan 24, 2020

@pr8x

This comment has been minimized.

Copy link
Contributor

pr8x commented Jan 25, 2020

Hi I tested your PR locally and it seems to be working fine so far, except some smaller things.

  • Editing Color (via hex value), Size and some other types is broken
  • I would suggest adding the Type of the property (Property.PropertyType.Name) in the DataGrid as well. This will make editing a bit easier, because you don't have to manually lookup the type of the property. Also making the column headers visible and resizable would be great as well.

image

  • The GridSplitter on the event page seems to be locked somehow - You can't move it.
@MarchingCube

This comment has been minimized.

Copy link
Member

MarchingCube commented Jan 27, 2020

One extra potential improvement: Allow for searching for event types in the Events tab. It quickly gets tedious to find wanted event type every time you open DevTools.

@kekekeks

This comment has been minimized.

Copy link
Member

kekekeks commented Jan 27, 2020

Note that Avalonia.Diagnostics now also has a dependency on Avalonia.Control.DataGrid.

We probably need to extract it to a separate package then.

grokys added 8 commits Feb 7, 2020
This prevents them being added to the core `Avalonia.nupkg`. Because diagnostics now has a dependency on `DataGrid` and roslyn scripting, ship it as a separate package.
And add explicit dependency on `Avalonia.Diagnostics` to samples.
@grokys grokys marked this pull request as ready for review Feb 14, 2020
@grokys

This comment has been minimized.

Copy link
Member Author

grokys commented Feb 14, 2020

This is now ready for review, I've made most of the suggested changes:

  • Avalonia.Diagnostics is now a separate package
  • Added "Type" column to show property type
  • Editing colors, sizes etc now works

I didn't add the filter to the Events tab yet as that's a bit more involved and I'd like to get this finished for the moment so I can continue with other things.

Note that the DataGrid highlight color is currently set to the same as the TextBox highlight color so editing values looks a bit wrong, see #3564 (comment)

@grokys grokys changed the title WIP: New and improved DevTools. New and improved DevTools. Feb 14, 2020
Copy link
Member

MarchingCube left a comment

Glanced over, submitting my immediate feedback and I shall return later to evaluate it further :)

Functionality wise LGTM.

@@ -1,6 +1,3 @@
// Copyright (c) The Avalonia Project. All rights reserved.

This comment has been minimized.

Copy link
@MarchingCube

MarchingCube Feb 14, 2020

Member

Any reason why half the files have license header and half do not?

This comment has been minimized.

Copy link
@grokys

grokys Feb 17, 2020

Author Member

Simply, the ones where I modified an old file still have headers. I don't bother adding headers to files these days as it just feels like busywork and from what I've read legally it's meaningless.

This comment has been minimized.

Copy link
@grokys

grokys Feb 17, 2020

Author Member

Maybe I should delete existing headers on modified files? Not sure.

This comment has been minimized.

Copy link
@MarchingCube

MarchingCube Feb 17, 2020

Member

TBH it would be good to be consistent at least. I have no problem with or without license info as long as we stick to one option.

Copy link
Member

MarchingCube left a comment

Looks like a great improvement over current one so I see no reason to not merge this in :)

@grokys grokys merged commit 2d7b559 into master Feb 21, 2020
3 checks passed
3 checks passed
Avalonia (Pull Requests) Avalonia (Pull Requests) succeeded
Details
AvaloniaUI.Avalonia #20200221.2 succeeded
Details
WIP Ready for review
Details
@grokys grokys deleted the feature/new-devtools branch Feb 21, 2020
@kekekeks

This comment has been minimized.

Copy link
Member

kekekeks commented Feb 21, 2020

Note: we still need to make Roslyn dependency optional if we want devtools to be operational on iOS/WASM.

@grokys

This comment has been minimized.

Copy link
Member Author

grokys commented Feb 21, 2020

TBH, I'm not sure how useful it would be there anyway, but yeah we can think about that when the time comes.

@kekekeks

This comment has been minimized.

Copy link
Member

kekekeks commented Feb 21, 2020

For mobile the idea was to use our previewer protocol. You know, that one that can show our controls in browser.

@Gillibald

This comment has been minimized.

Copy link
Contributor

Gillibald commented Feb 22, 2020

Currently the app itself shows the dev tools on demand. Why not make this a service you can connect to? So we could integrate this into the Visual Studio Extension or have it standalone.

Snoop for example could just inject itself into the target application and it just worked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.