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

Moved model classes in model package #313

Merged
merged 8 commits into from
Jun 10, 2016
Merged

Conversation

afester
Copy link
Collaborator

@afester afester commented May 17, 2016

This commit moves the model related classes into a separate "model" package. Meanwhile, there are plenty of classes in the "org.fxmisc.richtext" package, both UI related (with a strong dependency on JavaFX Node classes to create the scene nodes) and model related. Separating out the model classes would definitely help to understand the overall structure of the editor, specifically when getting started.
The drawback might be that some classes and methods had to be made public rather than package scoped, but I think this is reasonable.
Any comments are welcome ...

@TomasMikula
Copy link
Member

The change makes sense to me. I think it is a much better separation than what we used to have before: the main package and the skin sub-package, where in the main package we had a mixture of model and some parts of the view, and skin contained the rest of the view.

Can you also move model-related test classes to the model subpackage?

We should try to avoid circular dependencies between packages. Can we make model independent of the main package (i.e. not import anything from there).

You will also need to resolve the merge conflict.

@afester
Copy link
Collaborator Author

afester commented Jun 9, 2016

Hi Tomas,
I have updated this branch to the head of master. I had already solved the merge conflict, and made sure that there are no dependencies from classes in the model package to other RichTextFX classes. The model package is now self-contained. I also moved the model tests to the org.fxmisc.richtext.model package. Please let me know if anything additional is required ...

import java.util.Collection;
import java.util.List;

public interface SuperCodec<S, T extends S> extends Codec<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this need to become public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its used in StyleClassedTextArea:

SuperCodec.upCast(SuperCodec.collectionListCodec(Codec.STRING_CODEC)),
SuperCodec.upCast(SuperCodec.collectionListCodec(Codec.STRING_CODEC))

@TomasMikula
Copy link
Member

Other than the SuperCodec made public, looks good to me 👍

@TomasMikula
Copy link
Member

👍

@TomasMikula TomasMikula merged commit 8459717 into FXMisc:master Jun 10, 2016
@afester afester deleted the modelPackage branch June 13, 2016 07:14
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

Successfully merging this pull request may close these issues.

2 participants