Cloning issues arise with subclasses

[JordanMartinez]

I've run into another issue revolving around cloning.

Currently, each StyledTextArea (the View) creates its own StyledTextAreaModel (the Model). I'm guessing that the model itself is not passed to the View in order to prevent multiple views sharing the same model. (Each model is tasked with storing the data for where the caret position and selection values should be. If two views share the same model, it defeats the whole point).

However, this also creates another issue: StyledTextAreaModel cannot be subclassed. The following problems arise:

• Since StyledTextAreaModel's constructor takes an EditableStyledDocument as an argument, which is currently package-private, not public, subclasses can't have constructors that are used to clone those subclassed models since they can't fulfill super's constructor requirement.
• Since StyledTextArea constructs its own StyledTextAreaModel, even if one subclassed StyledTextAreaModel, they would not be able to create a StyledTextArea (a view) that uses such a model.

So, it seems like EditableStyledDocument should be made public, and a StyledTextArea would be updated to below:

public class StyledTextArea<PS, S, Model extends StyledTextAreaModel<PS, S>> {
    private final Model model;
    StyledTextAreaModel(
        BiFunction<?, ?> applyStyle, 
        BiFunction<?,?> applyParagraphStyle, 
        Model model
    ) {
        this.model = model;
        // the remaining constructor code....
    }
}

With a subclass of StyledTextAreaModel like the following:

public class AnotherModel<PS, S> extends StyledTextAreaModel<PS, S> { /* code */ }

...and a subclass of StyledTextArea like the following:

public class AnotherArea<PS, S, AnotherModel<PS, S>> extends StyledTextArea<PS, S> { /* code */ }

To create an area, one would use the following code:

AnotherModel<PS, S> model = new AnotherModel<>(/* args... */);
AnotherArea<PS, S, AnotherModel<PS, S>> area = new AnotherArea<>(model);

or use the following code:

AnotherModel<PS, S> model = new AnotherModel<>(/* args... */);
StyledTextArea<PS, S, AnotherModel<PS, S>> area = new StyledTextArea<>(model);

I think this was the conclusion I reached when I was working on #243.

[JordanMartinez]

Before the changes done in #253, a developer could subclass either of the four areas (StyledTextArea, InlineCssTextArea, StyleClassedTextArea, and CodeArea) without any issue. With the approach I have taken in #253, if a developer wants to subclass either of those areas but use their own subclass of StyledTextAreaModel, an annoyance arises.

Currently, the base is StyledTextAreaBase. Through generics, developers can specify their own subclass of StyledTextAreaModel. However, the other three areas (InlineCssTextArea, StyleClassedTextArea, and CodeArea) subclass StyledTextArea, which uses StyledTextAreaModel as its model. If a developer subclasses any of these four non-base areas, they are restricted to using StyledTextAreaModel as the model and not a subclass of it. If they want to use a subclass of the model class and use one of the other three areas, they will have to reimplement the code in them.
For example, their subclass of CodeArea that uses a subclass of the model would need to reimplement StyleClassedTextArea's code that applies the styles via StyleClass and apply the monospace font style to the area's text.

Although we could provide 2 classes for each area (one as the base that extends StyledTextAreaBase and another that extends its base but uses the regular StyledTextAreaModel class for its model), it doesn't scale well. Beyond this, it's not very inconvenient to the developer to specify one generic.

So, the names of the four areas should remain the same, but they will act as a base-class: their model will need to be specified each time they are used.

[JordanMartinez]

For those developers who still want to use just one of the four regular areas without any subclassing, AreaFactory can be used to provide those items.

[JordanMartinez]

Another question I've run into: should each text area have its own subclass of StyledTextAreaModel? Then, if a developer plans on subclassing something like StyleClassedTextArea, they would naturally subclass StyleClassedTextAreaModel as opposed to StyledTextAreaModel. This makes things more readable because the verbosity of generics is reduced.

For example, without following this "each area class has its own model class" approach, the code would be:

public class SubClassModel extends StyledTextAreaModel<
    Collection<String>, Collection<String>
> {
    // constructor

    // other code for stuff
}

public class SubClassArea extends StyleClassedTextArea<SubClassModel>
> {
    // constructor

   // other code for stuff
}

If this approach was followed, then RichTextFX would also include StyleClassedTextAreaModel:

public class StyleClassedTextAreaModel extends StyledTextAreaModel<
    Collection<String>, Collection<String>
> {
    // Constructor
}

and the above code is reduced to:

public class SubClassModel extends StyleClassedTextAreaModel {
    // constructor

    // other methods
}
public class SubClassArea extends StyleClassedTextArea<SubClassModel> {
    // constructor

    // other methods that relate to SubClassModel
}

I'm going to say yes, they should.

[JordanMartinez]

After implementing the above code in #263, I've learned that this is the right approach to use. Since the Area classes are now base classes to be extended, there are some pieces of code in the Area classes that must now be in the Model class. By following this approach, I can insure that these pieces of code don't get lost both in subclasses of the model and when using AreaFactory to construct a regular InlineCssTextArea or one of the other three areas.

[JordanMartinez]

StyledTextAreaModel is not meant to be subclassed, so this issue is no longer relevant.

[JordanMartinez]

Now that StyledTextAreaModel is package private, I can't clone subclasses of StyledTextArea. Either StyledTextAreaModel needs to be public AND final (so that it still cannot be subclassed) or StyledTextArea and its subclasses need to be final (so that modifications come by composition, not inheritance).

[JordanMartinez]

I'm assuming we'll follow the first approach. In other words, StyledTextArea and its subclasses (e.g. InlineCssTextArea, etc.) can still be subclassed. Currently, a subclass cannot clone itself for the following reasons:

• StyledTextAreaModel is not public
• EditableStyledDocument is not public

For example....

public class SomeArea extends InlineCssTextArea {
    /*
    If StyledTextAreaModel is public but EditableStyledDocument
      is not public, then this constructor could work
      to clone a subclass.
     */
    public SomeArea(SomeArea areaToClone) {
        super(areaToClone.getModel().getContent());
    }

    /*
    However, if EditableStyledDocument is not public, then
      subclasses of SomeArea cannot clone themselves
      because EditableStyledDocument cannot be used as a parameter
      in a constructor
     */
    public SomeArea(EditableStyledDocument<String, String> doc) {
        super(doc);
    }

    public static class AnotherArea extends SomeArea {

        // Will only work if EditableStyledDocument is public
        public AnotherArea(EditableStyledDocument<String, String> doc) {
            super(doc);
        }

    }

}

[TomasMikula]

I would make the EditableStyledDocument public as an interface (see #244) and keep StyledTextAreaModel package private.

[JordanMartinez]

Ok, that will work if we add the necessary API to expose a getter for StyledTextAreaModel's EditableStyledDocument in StyledTextArea.

public class StyledTextArea<PS, S> ... {

    public EditableStyledDocument<PS, S> getContent() {
       return model.getContent();
    }

}

[TomasMikula]

👍

[TomasMikula]

I am also quite skeptical about the usefulness of "cloning", especially if the user can pass an EditableStyledDocument to StyledTextArea's constructor. I struggle imagining a use case where one would create a StyledTextArea and then clone it, instead of just creating two StyledTextAreas right away, using the same EditableStyledDocument.

[JordanMartinez]

I struggle imagining a use case where one would create a StyledTextArea and then clone it, instead of just creating two StyledTextAreas right away, using the same EditableStyledDocument.

I think the only one that I can think of is when a user wants a second or third view. In such a case, one wouldn't immediately create that second or third view when the first is created.
For example, an IDE will only display one tab that shows some file's content. If the user wants to open another view of that same file, then they would need to clone the area. Once finished with the second (or perhaps first) view, they would close it. Thus, the initial area would be created and its possible that a second (or even third) would be created sometime later within the application's lifecycle but that's not guaranteed.

[TomasMikula]

In such a case, I'm still wondering whether it is simpler to use the cloning methods or just get the EditableStyledDocument from the original area and use it to create the new area. You know, in the second area, you might even want to display the style differently (or display a different aspect of style), so you might want to provide different applyStyle methods.

[JordanMartinez]

You know, in the second area, you might even want to display the style differently (or display a different aspect of style), so you might want to provide different applyStyle methods.

Right, but that option to use different apply functions still exists. And for those who don't want to do that, they can insure the same functions are used to apply the styles by using those functions' getters.

[TomasMikula]

I'm fine with having some convenience methods provided if it is a common enough use case. But it's hopeless to try to cover all the use cases and users should not fear construct their StyledTextArea directly. (Just as they should not fear using StyledTextArea directly when StyleClassedTextArea or InlineCssTextArea doesn't fit their purpose. I'm afraid that by providing those convenient subclasses I might have created an impression that using StyledTextArea directly is somehow taboo or complicated.)

[JordanMartinez]

I'm afraid that by providing those convenient subclasses I might have created an impression that using StyledTextArea directly is somehow taboo or complicated.

When I first started using your library, I didn't think about using StyledTextArea because it wasn't demonstrated in your Readme but rather its subclasses were (meaning its flavors were demonstrated but not the area itself). It might help to include a small note in the Readme about StyledTextArea being the base (before mentioning the "flavors") with three convenient subclasses (its flavors).

[TomasMikula]

Good point. Or maybe after, since the flavors are still easier to start with, especially for people who are not too used to a lot of type parameters and lambdas.

[JordanMartinez]

Or maybe after, since the flavors are still easier to start with, especially for people who are not too used to a lot of type parameters and lambdas.

Good point. Perhaps a small note that StyledTextArea is the base class before introducing its flavors (since they are easier to understand) and how they work. Once that's presented, the reader should have a better idea of the library and can now better understand an explanation of StyledTextArea itself with a custom "MyStyle" example.

[JordanMartinez]

This was fixed in #277