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

Refactor skin factory? #6

Open
eckig opened this issue Oct 29, 2014 · 4 comments
Open

Refactor skin factory? #6

eckig opened this issue Oct 29, 2014 · 4 comments

Comments

@eckig
Copy link
Contributor

eckig commented Oct 29, 2014

Currently the skin factories work this way:

void setNodeSkin(final String type, final Class<? extends GNodeSkin> skin);

In my opinion this has many disadvantages, mostly costing flexibility.

JavaFX uses the very elegant javafx.util.Callback interface to the same thing.
So I would like to propose to change the skin factories as follows:

void setNodeSkin(final String type, final Callback<GNode, ? extends GNodeSkin> skinCallback);

Instead of using the error prone way of reflection API you can simple call

skinCallback.call(xyz);

This way it is of no interest to you how the third party developer implemented his skin and how it is created.

@eckig
Copy link
Contributor Author

eckig commented Dec 4, 2014

I tried to port my application to the new version and failed. Some parts can be fixed on my side, but it seems that programmatically adding new connections (you may remember our earliert conversation on this) is even harder: I got lots and lots of NullPointerExceptions of Skins that do not exist.

So I implented this issue (refactoring to CallBack) and refactored the SkinManager to functional programming style to prevent the NullPointerExceptions: eckig/graph-editor@34cb1a6
This way the SkinManager never returns null (of course sill able to remove old skins, but we can easily swap the HashMap to WeakHashMap if you are memory sensitive).

@eckig
Copy link
Contributor Author

eckig commented Dec 10, 2014

Since the other changes got merged, may I create a pull request for the refactored SkinManager?

@rmfisher
Copy link
Contributor

I think this is a good idea but I need some more time to consider how the API should behave exactly. Don't submit a pull request just yet.

If you are getting NPE's there must be something else going wrong. Is this the same issue as #10?

@eckig
Copy link
Contributor Author

eckig commented Dec 10, 2014

I fixed most of the NPE's, the only remaining is the one in the mentioned issue.
So these changes are "just" API changes. Let me know, if and when I should create a pull request.

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

No branches or pull requests

2 participants