tesis-dynaware / graph-editor

Eclipse Public License 1.0
132 stars 42 forks source link

Refactor skin factory? #6

Open eckig opened 9 years ago

eckig commented 9 years ago

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 commented 9 years ago

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: https://github.com/eckig/graph-editor/commit/34cb1a6b04b8406b1f6e36c179b385e084b6fba4 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 commented 9 years ago

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

rmfisher commented 9 years ago

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 https://github.com/tesis-dynaware/graph-editor/issues/10?

eckig commented 9 years ago

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.