miroiu / nodify

Highly performant and modular controls for node-based editors designed for data-binding and MVVM.
https://miroiu.github.io/nodify
MIT License
1.3k stars 208 forks source link

Configurable arrowhead end #50

Closed mshumayl closed 1 year ago

mshumayl commented 1 year ago

📝 Description of the Change

This pull request aims to address #44. ArrowHeadEnds now dictates the location at which the arrow gets drawn. However, this will not alter the direction of the arrow.

The behavior is now as follows:

ArrowHeadEnds.End

nodify-arrowheadends-end

ArrowHeadEnds.Start

nodify-arrowheadends-start

ArrowHeadEnds.Both

nodify-arrowheadends-both

ArrowHeadEnds.None

nodify-arrowheadends-none

Please advice if this behavior is in line with the requirement, or if I misinterpreted the requirement.

🐛 Possible Drawbacks

I am not sure if this is the best way to implement this, and any advice to make the code better is much appreciated. I would also appreciate any pointers on code style. Thank you in advance for your help.

I will rebase and squash the commits once the changes are finalized.

mshumayl commented 1 year ago

Hi @miroiu, appreciate if you could help review this PR. Thank you in advance for your time.

mshumayl commented 1 year ago

Hi @miroiu, thank you very much for the code review. I really appreciate it.

What I had in mind was that this setting should also alter the direction of the arrowhead. I believe that Start and End should point in different directions, but if you find that this may be hard to implement we can skip it for now.

I think I can implement this by adding another switch statement inside the GetArrowHeadPoints method itself. Also, I think need another variable to know which arrow to draw for each of the two DrawArrowGeometry calls in the case of ArrowHeadEnds.Both.

I will push a new commit for this, then you can let me know if this approach is too convoluted, and if there is a better way to implement this functionality.

I forgot to mention in the issue but the Changelog should be updated with the new changes.

Will do! I will update this once this gets finalized.

mshumayl commented 1 year ago

The arrowhead ends now have the following behavior:

ArrowHeadEnds.End

nodify-arrowheadends2-end

ArrowHeadEnds.Start

nodify-arrowheadends2-start

ArrowHeadEnds.Both

nodify-arrowheadends2-both

ArrowHeadEnds.None

nodify-arrowheadends2-none

Appreciate if you can let me know if my approach is too convoluted or if there is a more succinct way to implement this. Thank you very much for your time.

miroiu commented 1 year ago

Hi, I want to have the arrowheads pointing to the connectors in general, not only for ArrowHeadEnds.Both. I think this could be easily implemented by sending the Direction as a parameter to the GetArrowHeadPoints utility and change it based on the arrowhead end.

miroiu commented 1 year ago

I'm on phone and didn't see the pictures, but that's exactly what I wanted. Just make sure that if you change direction it still works.

miroiu commented 1 year ago

Hi @mshumayl ! I will merge the PR once the coding style issues are fixed and the changelog updated. Great job!

mshumayl commented 1 year ago

Hi @miroiu, one last question. In changelog.md, do I need to create a new version header and provide the details there? If so, what version should it be?

miroiu commented 1 year ago

You can add it to the In Development section because the version will be added with the next release.

mshumayl commented 1 year ago

Thank you very much for your help and guidance, @miroiu!