Some feedback on making this general purpose graph control

This is amazing, was able to create very quickly a good-looking graph-based window for my project (image below).
I think can made into a package as a graph control by letting the user set the content of each Node and Connector and customize them.
Few things I noticed when combining it with my implementation -

~~1. Check whether the node was moved and if so handle the event in NodeDragBehavior~~

Allow setting content of a node, possibly by having DataTemplates in DrawingNodeView or just add content property in the node object (not sure about this).

Not sure what are your plans with this, if you'd like I can make these changes as I already work on them in my project.

Thanks!

I think can made into a package as a graph control by letting the user set the content of each Node and Connector and customize them.

@adirh3 For nodes I was thinking to let users created their own nodes by inheriting from ConnectedNodeViewModel and adding their own view. The way I do that in demo can be seen here (the node view is resolved by view locator automatically).

NodeEditor/samples/NodeEditorDemo/ViewModels/RectangleNodeViewModel.cs

Line 5 in 3721f0b

public class RectangleNodeViewModel : ConnectedNodeViewModel

NodeEditor/samples/NodeEditorDemo/Views/RectangleNodeView.axaml

Line 1 in 3721f0b

<UserControl xmlns="https://github.com/avaloniaui"

NodeEditor/samples/NodeEditorDemo/ViewLocator.cs

Line 9 in 3721f0b

public class ViewLocator : IDataTemplate

Not sure what are your plans with this, if you'd like I can make these changes as I already work on them in my project.

@adirh3 For now I propose discussing changes in issues (each change on new issue). After that we can decide what to do. It's bit to early and stuff may change a lot I think until it's bit stable.

@adirh3 I will release NodeEditor as NuGet package soon with some alpha state so it's easy to test and get some more feedback.

Check whether the node was moved and if so handle the event in NodeDragBehavior -
In Moved-
   if (deltaX > 2 || deltaY > 2)
                _moved = true;
In Released -
            e.Handled = _moved && _enableDrag;          
            Released();

@adirh3 Not sure about that, added something like this 211a2a9

Check whether the node was moved and if so handle the event in NodeDragBehavior -
In Moved-
   if (deltaX > 2 || deltaY > 2)
                _moved = true;
In Released -
            e.Handled = _moved && _enableDrag;          
            Released();
@adirh3 Not sure about that, added something like this 211a2a9

@wieslawsoltes Looks good, now thinking about it I shouldn't have added the delta check since it's in the Moved event.

I think can made into a package as a graph control by letting the user set the content of each Node and Connector and customize them.

@adirh3 For nodes I was thinking to let users created their own nodes by inheriting from ConnectedNodeViewModel and adding their own view. The way I do that in demo can be seen here (the node view is resolved by view locator automatically).

NodeEditor/samples/NodeEditorDemo/ViewModels/RectangleNodeViewModel.cs

Line 5 in 3721f0b

public class RectangleNodeViewModel : ConnectedNodeViewModel

NodeEditor/samples/NodeEditorDemo/Views/RectangleNodeView.axaml

Line 1 in 3721f0b

<UserControl xmlns="https://github.com/avaloniaui"

NodeEditor/samples/NodeEditorDemo/ViewLocator.cs

Line 9 in 3721f0b

public class ViewLocator : IDataTemplate

I also inherited from the node type, and used it as you did, but wasn't sure if would be optimal in all use cases to combine the user object with the node object, but I am probably overthinking it.

Not sure what are your plans with this, if you'd like I can make these changes as I already work on them in my project.

@adirh3 For now I propose discussing changes in issues (each change on new issue). After that we can decide what to do. It's bit to early and stuff may change a lot I think until it's bit stable.

Removed the first point, only thing that might worth discussing is how the node view models are being used (user inherits or provides his content), but I as you suggested it might be better to just release it as alpha and get some more feedback.

Thanks :)

I also inherited from the node type, and used it as you did, but wasn't sure if would be optimal in all use cases to combine the user object with the node object, but I am probably overthinking it.

Yeah that's not the best solution but for now works. Will try to work on something better in meantime.

Removed the first point, only thing that might worth discussing is how the node view models are being used (user inherits or provides his content), but I as you suggested it might be better to just release it as alpha and get some more feedback.

For now I like inherited way, but they may not be the best when we take in regard the first point, there are ways we can make it working both ways (inheriting or content setting) by providing default template with content presenter.

@adirh3 Released alpha1 package https://www.nuget.org/packages/NodeEditorAvalonia

@adirh3 Released alpha1 package https://www.nuget.org/packages/NodeEditorAvalonia

Thanks will change my code to it and see if I have more feedback

@wieslawsoltes one thing I noticed with the inheritance approach, is that it's a bit awkward if you need to serialize the object, you either get many none needed properties serialized, or you need to make a different object for serialization and then copy the data using a JsonConverter.
It's not a big deal, just thought it's worth sharing.

@wieslawsoltes one thing I noticed with the inheritance approach, is that it's a bit awkward if you need to serialize the object, you either get many none needed properties serialized, or you need to make a different object for serialization and then copy the data using a JsonConverter. It's not a big deal, just thought it's worth sharing.

How would that be solved in other approach like setting node content? You still need to serialize all nodes. You can control what is serialized using property attributes and skip default values. We can add that.

I have done this here:
https://github.com/wieslawsoltes/Dock/blob/811685d5e555ebbad0e31d72769cf2f28a7ef6e1/src/Dock.Model.ReactiveUI/Core/DockBase.cs#L49

@wieslawsoltes one thing I noticed with the inheritance approach, is that it's a bit awkward if you need to serialize the object, you either get many none needed properties serialized, or you need to make a different object for serialization and then copy the data using a JsonConverter. It's not a big deal, just thought it's worth sharing.

How would that be solved in other approach like setting node content? You still need to serialize all nodes. You can control what is serialized using property attributes and skip default values. We can add that.

I have done this here: https://github.com/wieslawsoltes/Dock/blob/811685d5e555ebbad0e31d72769cf2f28a7ef6e1/src/Dock.Model.ReactiveUI/Core/DockBase.cs#L49

Having the view models automatically created based of the graph's control properties.
So, the user will only have to do something like -

<GraphControl Items="{Binding Items}"  Connectors="{Binding Connectors}">
     <GraphControl.DataTemplates>
             <GraphItemDataTemplate PinsPosition="Left" ...>
                    <Button ../>
               </GraphItemDataTemplate>
    </GraphControl.DataTemplates>
</GraphControl>

But I am probably missing a lot of obvious stuff why this wouldn't work, haven't read all of your code yet.

@wieslawsoltes one thing I noticed with the inheritance approach, is that it's a bit awkward if you need to serialize the object, you either get many none needed properties serialized, or you need to make a different object for serialization and then copy the data using a JsonConverter. It's not a big deal, just thought it's worth sharing.

How would that be solved in other approach like setting node content? You still need to serialize all nodes. You can control what is serialized using property attributes and skip default values. We can add that.
I have done this here: wieslawsoltes/Dock@811685d/src/Dock.Model.ReactiveUI/Core/DockBase.cs#L49

Having the view models automatically created based of the graph's control properties. So, the user will only have to do something like -
<GraphControl Items="{Binding Items}"  Connectors="{Binding Connectors}">
     <GraphControl.DataTemplates>
             <GraphItemDataTemplate PinsPosition="Left" ...>
                    <Button ../>
               </GraphItemDataTemplate>
    </GraphControl.DataTemplates>
</GraphControl>   
But I am probably missing a lot of obvious stuff why this wouldn't work, haven't read all of your code yet.

Yeah I did not investigate that part yet, did not have much time.

Having the view models automatically created based of the graph's control properties. So, the user will only have to do something like -
<GraphControl Items="{Binding Items}"  Connectors="{Binding Connectors}">
     <GraphControl.DataTemplates>
             <GraphItemDataTemplate PinsPosition="Left" ...>
                    <Button ../>
               </GraphItemDataTemplate>
    </GraphControl.DataTemplates>
</GraphControl>   
But I am probably missing a lot of obvious stuff why this wouldn't work, haven't read all of your code yet.

You need to somehow select proper DataTemplate for each node. What would be use instead of types?

We can do easily some custom view locator by implementing IDataTemplate, that's easy part:

NodeEditor/samples/NodeEditorDemo/ViewLocator.cs

Line 13 in 0388127

var name = data.GetType().FullName!.Replace("ViewModel", "View");

@adirh3 Next step to simplify #5