Custom object support

[afester]

This is an enhanced proposal to support custom objects which can be rendered by user specific nodes.

In the model layer, all occurrences of StyledTexthave been replaced by the new interface Segment. StyledTextis now just one flavor of segment types. CustomObjectis an abstract class which implements the Segmentinterface and which provides a base class for user defined objects. Out-of-the-box, RichTextFX would support images to be inserted into the document, but the user can define any other custom objects by registering a factory method for the JavaFX node and a decode method for deserializing the object through a Codec. See CustomObjectDemofor an example how to register and use custom objects.

The code already supports copy&paste (if the selection contains a custom object, it is serialized when copying). Selecting one single custom object by clicking on it is not supported yet.

Any feedback/comments are welcome - especially registering the decode method is very unclean yet, see the SegmentFactory class. Essentially, decoding must be delegated to the custom object implementation since only the custom object knows how to recreate itself from a DataInputStream, but the method in ReadOnlyStyledDocument which does the decoding is static which makes it difficult to access a delegate. I tried to make those methods non-static, but this looked like a huge bunch of refactoring is necessary then.

[JordanMartinez]

A minor detail, but this should probably be renamed to type now, not typeId as it was when you were using an int rather than your current enum/interface approach

[TomasMikula]

Thanks for the work you have done on this so far!

My main question (for everyone) is: Can we find a way to turn this RuntimeException into a compile time error? For any application, the types of custom objects are known in advance (at compile time). It's not like they are being defined dynamically or loaded dynamically into the JVM at runtime. So in principle, it should be possible. I know Java is not great at these kinds of abstractions, but can we give it some thought?

[JordanMartinez]

Can we find a way to turn this RuntimeException into a compile time error?

Couldn't we store the NodeFactory getter inside the object implementing the Segment interface? For example:

public interface Segment<S> {
    public <T extends Segment<S>> NodeFactory<S, T> getFactory();
}

@FunctionalInterface
public interface NodeFactory<S, T extends Segment<S>> {
    public Node apply(T seg);
}

// implementations
class StyledText<S> extends Segment<S> {
    public NodeFactory<S, StyledText<S>> getFactory() {
        return DefaultNodeFactories.getStyledTextFactory();
    }
}

public class DefaultNodeFactories<S> {
    private static final NodeFactory<S, StyledText<S>> STYLED_TEXT_FACTORY = 
            seg -> { /* view construction code here... */ };

    public static final StyledTextFactory<S, StyledText<S>> getStyledTextFactory() { 
           return STYLED_TEXT_FACTORY; 
    }

   // other default factories here (e.g. table, shapes, etc.)
}

[TomasMikula]

Something like that would probably work, but isn't a very good separation
between model an view.

On Sep 11, 2016 7:00 PM, "JordanMartinez" notifications@github.com wrote:

Can we find a way to turn this RuntimeException into a compile time error?

Couldn't we store the NodeFactory getter inside the object implementing
the Segment interface? For example:

public interface Segment {
public <T extends Segment> NodeFactory<S, T> getFactory();
}
@FunctionalInterfacepublic interface NodeFactory<S, T extends Segment> {
public Node apply(T seg);
}
// implementationsclass StyledText extends Segment {
public NodeFactory<S, StyledText> getFactory() {
return DefaultNodeFactories.getStyledTextFactory();
}
}
public class DefaultNodeFactories {
private static final NodeFactory<S, StyledText> STYLED_TEXT_FACTORY =
seg -> { /* view construction code here... */ };

public static final StyledTextFactory<S, StyledText<S>> getStyledTextFactory() {
       return STYLED_TEXT_FACTORY;
}

// other default factories here (e.g. table, shapes, etc.)
}

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#356 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAadH5xIVztN5itSYFzLef44Rd5ZrH5Wks5qpIf0gaJpZM4J48xE
.

[JordanMartinez]

it isn't a very good separation between model an view.

Edit: Updated to read more clearly
True... It's almost like we'll need to add another generic to StyledTextArea with a class that does know how to display everything. For example:

public class StyledText<S> implements Segment<S> {}
public class Table<S> implements Segment<S> {}

public interface NodeFactory<S> {
    public Node apply(Segment<S> seg);
}

public class DefaultNodeFactory<S> implements NodeFactory<S> {
    public Node apply(StyledText<S> seg) {
        // code
    }
    public Node apply(Table<S> seg) {
        // code
    }

    public Node apply(Segement<S> seg) {
        throw RunTimeException("No known way to display Segment of class: " +
            Segment.getClass().getName());
    }
}

public class StyledTextArea<PS, S, NF extends NodeFactory<S>> {
    private final NF nodeFactory = new DefaultNodeFactory();

    // code to construct node for a segment
    return nodeFactory.apply(seg);
}

...except it still isn't a compile-time error but a runtime one...

[TomasMikula]

What if we abstract over the type of segments in a type parameter:

/**
 * Segment operations on type Seg.
 * Notice that each method takes the segment as an argument.
 */
public interface SegmentOps<Seg> {
    int length(Seg segment);
    char charAt(Seg segment, int index);
    /* ... */
}

public class StyledTextArea0<PS, Seg, S> {
    public StyledTextArea0(
            PS initialParagraphStyle, BiConsumer<TextFlow, PS> applyParagraphStyle, // as before
            S initialTextStyle, // as before
            SegmentOps<Seg> segmentOps,
            Function<Seg, Node> nodeFactory,
            BiFunction<Seg, S, Seg> applyStyle // Given a segment, update its style. The result is a new segment.
    ) {
        /* ... */
    }
}

Note that any type Foo can now represent a segment, without having to implement any interface, as long as we provide an implementation of segment operations for that type, i.e. an instance of SegmentOps<Foo>. This is similar to how you can create a TreeSet<E> for any type E whatsoever, given that you provide a Comparator<E>.

Our original StyledTextArea is then

public class StyledTextArea<PS, S> extends StyledTextArea0<PS, StyledText<S>, S> {}

If one wanted to embed images in addition to StyledText, they could define their own

public class MyArea<PS, S> extends StyledTextArea0<PS, Either<StyledText<S>, LinkedImage<S>>, S> {}

Extending is not even really necessary, it is just to provide a shorthand for the rather long type StyledTextArea0<PS, Either<StyledText<S>, LinkedImage<S>>, S>.

[JordanMartinez]

What if we abstract over the type of segments in a type parameter

That seems like it would work. Additionally, it looks like it would address the canJoin strategy issue I raised earlier since that would be user-defined.

[afester]

Hi Tomas and Jordan,

thanks for your feedback!

Something like that would probably work, but isn't a very good separation
between model an view.

Right, that is what I also observed while I tried to implement the proposal from @JordanMartinez ... I will check if the additional type parameter is feasible (even though I dont feel completely comfortable with adding yet another type parameter ...)
Besides the Node factory, I am more concerned about the factory which creates a specific segment type from its serialized representation (decode). I dont think that the SegmentFactory I am currently using is a very clean solution - do you have any idea on that one`?

[TomasMikula]

even though I dont feel completely comfortable with adding yet another type parameter

I feel the pain from the user point of view. As an implementor, I'm completely comfortable with having lots of type parameters and a very generic implementation. I even think abstracting something away as a type parameter reduces the potential for bugs in the implementation, since you don't have access to the specifics of the concrete type.

From the user point of view, it is a major pain if you have to keep specifying 3 type arguments, especially when they are the same 3 concrete types throughout your whole application. I believe this issue could be resolved satisfactorily by using a type alias. Unfortunately, Java doesn't have type aliases. Type aliases exist e.g. in

C++:

using Style = Collection<String>
using MyArea = StyledTextArea0<Style, StyledText<Style>, Style>

Scala:

type Style = Collection[String]
type MyArea = StyledTextArea0[Style, StyledText[Style], Style]

and other languages. What we can do in Java is

class MyArea extends StyledTextArea0<Collection<String>, StyledText<Collection<String>>, Collection<String>> {
    /* define constructors */
}

but then StyledTextArea0<Collection<String>, StyledText<Collection<String>>, Collection<String>> cannot be used where MyArea is required, because they are not the same type.

I'm somewhat reluctant to compromise what otherwise seems like a reasonable design due to a deficiency in the language.

I am more concerned about the factory which creates a specific segment type from its serialized representation (decode).

decode in StyledText would produce StyledText, decode in LinkedImage would produce LinkedImage. Then if we were to decode Either<StyledText<S>, LinkedImage<S>>, we need a Codec<Either<StyledText<S>, LinkedImage<S>>>. Such codec can be defined easily, if we already have Codec<StyledText<S>> and Codec<LinkedImage<S>>. (In the same spirit as Codec<Optional<T>> is defined based on Codec<T>.) Does this answer your concern?

[JordanMartinez]

I'm somewhat reluctant to compromise what otherwise seems like a reasonable design due to a deficiency in the language.

I'd stick with the good design since it will lead to less bugs and only Java-language users are affected. Java will hopefully add that feature in a later release, but until then one can work around this annoyance through ways that work for them. For example, typing in a placeholder text (e.g. Style) that they then later programmatically replace in the IDE's editor with the correct concrete class (e.g Collection<String>), or defining a keyboard shortcut that will paste the correct code in when needed.

[TomasMikula]

Those IDE tricks help with writing those programs, but not with reading. Code is much more often read than written. Otherwise, I agree with the rest.

[JordanMartinez]

Those IDE tricks help with writing those programs, but not with reading. Code is much more often read than written. Otherwise, I agree with the rest.

Good point. 😞

[JordanMartinez]

How long would it take for a feature like this to make it into Java if a JSR was started for it (I'm not sure if there already is one / has been one)?

[alt-grr]

How long would it take for a feature like this to make it into Java if a JSR was started for it (I'm not sure if there already is one / has been one)?

Probably no earlier than Java 11.

[JordanMartinez]

Probably no earlier than Java 11.

Lame....

Let's put it this way. There have been four approaches so far:

• afester's approach (which has a runtime exception),
• my "include a NodeFactory in the implemented Segment" approach (which has a design issue as the model not separated from view),
• my second approach (which leads to a runtime exception eventually), or
• Tomas' approach (which leads to a reading issue).

It seems like we've exhausted all the approaches we could take to this problem, correct? So, which issue is the least problematic holistically?

• My first approach is a design issue, so it should be excluded immediately to prevent a greater complexity of bugs arising from that design issue
• Afester's approach or my second approach's flaws can be minimized via user's unit testing
• Tomas' approach's flaws can only be fixed when Java does(sometime in the next decade), or if users decide to take it upon themselves to understand the code, or if we have good documentation that happens to not change frequently.

In other words, is there a way for us to minimize Tomas' approach's flaw as I do think it's the best approach to resolving this problem?

[afester]

I feel the pain from the user point of view. As an implementor, I'm completely comfortable with having lots of type parameters and a very generic implementation. I even think abstracting something away as a type parameter reduces the potential for bugs in the implementation, since you don't have access to the specifics of the concrete type.

Definitely, using appropriate language features to make the implementation more robust is the right approach. However, its also important to consider the API design - complex APIs prevent people from using the component. When evaluating a component, the first things people look at are probably the code examples and the JavaDoc - for the time being, the component itself is a black box for them.

Anyway, @JordanMartinez thanks for summarizing the various approaches - I will see if I can come up with a proposal for Tomas' generics approach especially to see how the API "feels" with that approach.

[afester]

@TomasMikula

Note that any type Foo can now represent a segment, without having to implement any interface, as long as we provide an implementation of segment operations for that type

Would Foonot be required to at least extend from some abstract base class? Otherwise, where would you store the style information, that is (today in StyledText)

private final S style;

public S getStyle() {
    return style;
 }

[JordanMartinez]

@afester

Would Foo not be required to at least extend from some abstract base class? Otherwise, where would you store the style information, that is (today in StyledText)

private final S style;

public S getStyle() {
return style;
}

I believe you're right. As it stands, we can't store that info in SegOp due to its generic type:

public class Segment {}
public interface SegOp<Segment> {
    public int length(Segment segment);
    public S getStyle(Segment segment); // compilation error
}

So we'd need another interface like Styleable (or some better-named interface to prevent confusion with paragraph styles) for that. For example:

public interface Styleable<S> {
    public S getStyle();
}
public class Foo implements Styleable<String> {}
Segment<Foo> seg = //
assert seg.getStyle().getClass() == String.getClass();

[TomasMikula]

complex APIs prevent people from using the component. When evaluating a component, the first things people look at are probably the code examples and the JavaDoc

I believe we could preserve current StyledTextArea API as is, even maintaining source compatibility. We will just make it extend a more generic class (which in my example I named StyledTextArea0).

Otherwise, where would you store the style information

Good catch! In my sketch, I intended the applyStyle function passed to the constructor to update style of a segment:

BiFunction<Seg, S, Seg> applyStyle

But I forgot about extracting style back (Function<Seg, S> getStyle). We could either make SegmentOps style-aware, i.e.

public interface SegmentOps<Seg, Style> {
    Style getStyle(Seg segment);

    // and then `setStyle` can go here as well
    Seg setStyle(Seg segment, Style style);
}

Or add the setter to the parameter list of the StyledTextArea0 constructor.

Btw, a pair of getter and setter functions is also called a Lens, so we could package it up in an interface

public interface Lens<T, U> {
    U get(T t); // getStyle
    T set(T t, U u); // setStyle

    // factory method to create a Lens given two lambdas
    static <A, B> Lens<A, B> create(Function<A, B> getter, BiFunction<A, B, A> setter) {
        return new Lens<A, B> {
            // implementation trivial
        }
    }
}

so that it is a single constructor parameter.

Also, I think in the most generic version of StyledTextArea (say, StyledTextArea00 😄), we could even do without any notion of a segment style at all. Then StyledTextArea0 introduces the notion of segment style and since it has the Lens to get/set styles of segments, it can add segment style-related methods to its API. But this is just a thought for a potential next iteration :)

[JordanMartinez]

But I forgot about extracting style back (Function<Seg, S> getStyle). We could ... make SegmentOps style-aware

I didn't list your suggestion in my previous comment because I thought it would be easier for users if they did not need to keep another generic type in mind when doing segment operations. Thinking about it now, that's probably not true.

Would there be any advantage to adding it to the StyledTextArea0 constructor parameter list as opposed to the SegOps interface? The latter makes more sense in my mind because any style-related operations (e.g. merge) could also be contained there, though that could be stored in another interface StyleOps that is later stored in a SegOps implementation.

StyledTextArea00 😄

:-) I feel like that could get confusing after a while... Could we just call StyledTextArea00 RichAreaBase / RichTextAreaBase instead (since it would no longer have 'styles' associated with it and there can be more than just text displayed) and call StyledTextArea0 StyledRichAreaBase (because that's where styles would be introduced), and keep StyledTextArea as is?

[TomasMikula]

And then add a GenericRichAreaBase, which will be just a subclass of AbstractGenericRichAreaBase 😄

Just kidding, naming is hard. StyledTextArea0 was just a working name :)

Would there be any advantage to adding it to the StyledTextArea0 constructor parameter list as opposed to the SegOps interface? The latter makes more sense in my mind

Yeah, makes more sense to me as well. If we then decide to generalize to StyledTextArea00/RichTextAreaBase, we will need a SegOps0 (:wink:) that doesn't talk about styles.

[JordanMartinez]

And then add a GenericRichAreaBase, which will be just a subclass of AbstractGenericRichAreaBase 😄

😆 Lol!

If we then decide to generalize to StyledTextArea00/RichTextAreaBase, we will need a SegOps0 (:wink:) that doesn't talk about styles.

Or we could have a base interface with no style, SegOps (my idea for your SegOps0), and possibly an extension of that interface that adds style, StyledSegOps (my idea for our current SegOps).

[JordanMartinez]

@afester Can you give us an update? Or have you been working on other things recently?

[afester]

I started with the Generics approach (you can have a look at it at https://github.com/afester/RichTextFX/tree/genericSegment), but I found that the separate implementation of the operations unnecessarily bloats up the code, since the operations have to be looped through to many classes and in some cases also to static methods. If we still want to persue a Generics based approach, I would suggest to at least provide a base class for the generic segment, like <SEG extends Segment>.

Anyway, I would like to make another proposal which I just committed: Instead of adding an enum for each segment type, this approach uses the class name as the type id for the segment types. This is similar to what Java serialization does and allows to recreate the Segment objects through reflection. It avoids the mapping between type id and factory, and thus the RuntimeException. Naturally, a bunch of other errors might occur during instantiation of the Segments through reflection, but these are really severe runtime errors like invalid classpaths etc. The only place where the objects need to be recreated is the decode method of the Codec, and all those errors are thrown as IOException with the actual error as nested exception.

To create the JavaFX nodes, the segments now provide a createNode() method. The little drawback is that there is now a small dependency from the model to the JavaFX Node, but I think that dependencies to the Java runtime are fine. The Node factories for the default segment types (currently StyledText and LinkedImage) are injected into the model package to avoid dependencies from the model layer up to the view layer.

The segments now also have a method canJoin which checks if a particular segment can be joined with another segment - this is more flexible than the earlier approach.

Finally, I had to rework the algorithm which restyles part of the document. The earlier approach split the corresponding text and re-created the StyledText objects for each style span, but this is not possible anymore since there might be other Segment types involved. I created some Unit tests for that and the new approach works fine (including the RichText demo when selecting and restyling a range which includes an image), but there might still be corner cases and/or better approaches.

Please let me know any feedback ...

[JordanMartinez]

Thanks for looking into the generic approach!

I found that the separate implementation of the operations unnecessarily bloats up the code, since the operations have to be looped through to many classes and in some cases also to static methods.

I went through your code in my IDE and I think I see your point about how the SegOps bloats the code. In my edits to your genericApproach branch (see my PR), it starts with StyledDocument: I added a getSegOps method to the interface so that ReadOnlyStyledDocument and EditableStyledDocument could return their related object for various parameters in methods. ParagraphText (and thus ParagraphBox which creates it) needs the SegOps object which an area's EditableStyledDocument can provide through the aforementioned interface's new method, so that it can extract the text and style from the Segment. Then there is the model class, Paragraph, that needs to know how to operate on its segments.
Finally, StyledTextArea's most generic constructor is the only one that needs the object as a parameter so it can properly construct a SimpleEditableStyledDocument. If the ESD s already constructed, the rest of the constructors' parameter lists never change.

In my PR to your branch, I also added a SegmentOps object to the static methods of ReadOnlyStyledDocument. I'm not sure of all the implications of this, but it seems like it could cause a release-dependent issue depending on how one implements the code. What happens if the SegmentOps class is updated/modified later on? Will that screw up anything?

Part of the issue here is also readability, correct? For example, the repetitive calls of segmentOps.[method] looks weird as shown in a portion of the method getStyleSpans:

        if(startSegIdx == endSegIdx) {
            SEG seg = segments.get(startSegIdx);
            builder.add(segmentOps.getStyle(seg), to - from);
        } else {
            SEG startSeg = segments.get(startSegIdx);
            builder.add(segmentOps.getStyle(startSeg), segmentOps.length(startSeg) - start.getMinor());

            for(int i = startSegIdx + 1; i < endSegIdx; ++i) {
                SEG seg = segments.get(i);
                builder.add(segmentOps.getStyle(seg), 
                            segmentOps.length(seg));
            }

            SEG endSeg = segments.get(endSegIdx);
            builder.add(segmentOps.getStyle(endSeg), end.getMinor());
        }

We could add method wrappers to make it easier to read, but I'm not sure how that would affect performance:

class Paragraph {
    private final SegmentOps<Seg, S> segOps

    // method wrappers for "segOps.length(seg)"
    private int segLength(Seg segment) { return segOps.length(segment); }
}

Then the above method might look like this:

        if(startSegIdx == endSegIdx) {
            SEG seg = segments.get(startSegIdx);
            builder.add(segStyle(seg), to - from);
        } else {
            SEG startSeg = segments.get(startSegIdx);
            builder.add(segStyle(startSeg), segLength(startSeg) - start.getMinor());

            for(int i = startSegIdx + 1; i < endSegIdx; ++i) {
                SEG seg = segments.get(i);
                builder.add(segStyle(seg), segLength(seg));
            }

            SEG endSeg = segments.get(endSegIdx);
            builder.add(segStyle(endSeg), end.getMinor());
        }

[JordanMartinez]

I've updated my PR to your generic approach (once you merge it) to now compile completely. The only demo that actually works (I used SegOps place holders on the rest of the demos) is the RichText demo.

You can see my branch here: https://github.com/JordanMartinez/RichTextFX/tree/genericSegment

[TomasMikula]

but I found that the separate implementation of the operations unnecessarily bloats up the code, since the operations have to be looped through to many classes and in some cases also to static methods.

Can you point me to this problematic code?

I would suggest to at least provide a base class for the generic segment, like <SEG extends Segment>.

That limits the flexibility somewhat. Without requiring inheritance from a base class (or implementing an interface), even classes that were not originally intended to represent a segment can be used as such.

Anyway, I would like to make another proposal which I just committed: Instead of adding an enum for each segment type, this approach uses the class name as the type id for the segment types. This is similar to what Java serialization does and allows to recreate the Segment objects through reflection.

I'm afraid we are asking for a lot of trouble down the road if we use reflection. How would your decode method for Segments work for a segment that has an additional type parameter, like

class MySegment<T, S> implements Segment<S> {
    private T t;
    public void decode(DataInputStream is, Codec<S> styleCodec) throws IOException {
        // how to implement this???
        // We have no way to decode T (because we have no Codec<T>)
    }
}

Not to mention that all Segments now need to know how to serialize/deserialize themselves. I don't feel comfortable requiring users to implement serialization for their segments even when they are not at all interested in saving the content of the text area.

Btw, you are addressing two concerns in this PR: custom objects and serialization. I think we can address them separately, which would make your work on this PR easier.

To create the JavaFX nodes, the segments now provide a createNode() method.

My objection is the same as before, that this doesn't allow for separation of model from view.

[JordanMartinez]

Btw, you are addressing two concerns in this PR: custom objects and serialization. I think we can address them separately, which would make your work on this PR easier.

But can we implement custom objects without it also affecting the serialization? Or are you suggesting we export the 'serialization' part to be outside the area? For example, putting it into EditableStyledDocument and allowing the developer to determine whether or not this method exists.

Wouldn't the "custom object" feature also require some API for how/when to display the caret (e.g. tables)?

[afester]

@JordanMartinez if you can still remember what you implemented, it would be great if you could redo this work ... 😃

[JordanMartinez]

See my work here.

[afester]

@JordanMartinez I am getting a NullPointerException in the RichTextEditor demo when I insert an image into an empty document, set the cursor before the image and enter more than two letters:

Exception in thread "JavaFX Application Thread" java.lang.NullPointerException
	at org.fxmisc.richtext.demo.richtext.RichText.lambda$18(RichText.java:79)
	at org.fxmisc.richtext.StyledTextArea.createStyledTextNode(StyledTextArea.java:104)
	at org.fxmisc.richtext.demo.richtext.RichText.createNode(RichText.java:292)
	at org.fxmisc.richtext.demo.richtext.RichText.lambda$1(RichText.java:79)
	at org.fxmisc.richtext.ParagraphText.<init>(ParagraphText.java:102)
	at org.fxmisc.richtext.ParagraphBox.<init>(ParagraphBox.java:77)
	at org.fxmisc.richtext.GenericStyledArea.createCell(GenericStyledArea.java:1176)
        ...

I'll continue to investigate this issue, but if you have any additional feedback please let me know 😃

[UPDATE]
This seems to be a problem with the style promotion. Dumping the segment in StyledTextArea.createStyledTextNode(), directly before the style is applied:

   ...
   System.err.println(seg);
   applyStyle.accept(t, segOps.getStyle(seg));
   ...

leads to the output

StyledText[text="", style=12,Serif,0x000000ff]
StyledText[text="j", style=]
StyledText[text="s", style=null]

when adding an image and then typing "j" followed by "s" in front of the image. The null style finally leads to the NPE.

Default method StyledDocument.getStyleAtPosition(int pos) called from StyledTextAreaModel.getStyleForInsertionAt() returns the wrong style information for the given position (empty string first, then null after typing the second letter).

[JordanMartinez]

@afester if it's a null style, the fix is very easy. If you look at StyledText's textOps (which returns the SegmentOps for StyledText), you'll see I passed in a null value for the style when creating an empty segment.
I didn't know whether this would have any adverse affects, but I thought it would be fine. It also made the code easier to write because one wouldn't have to pass in an empty style when calling that method.

I've changed it on my side and the NPE goes away now. See the branch to which I referred above with the fix.

However, if I now follow the steps you said above

1. Insert image in empty area
2. Move caret to be in front of image
3. Type "j"
4. Type "s"

An odd thing occurs on step 3. Once "j" is typed, the caret moved left one position, so that the next letter I insert ("s") is then inserted in front of the "j". Thus I have sj[image].

Do you know why this might occur? It only occurs when the image is the first segment in a paragraph and the caret is at position 0 in the line (i.e. to the left of the image).

Update: Figured it out. LinkedImageOps always returns 1 for its length, even if the segment passed in is empty. This also fixes a few other issues I came across. See my branch for the fix. It now works!

Update: On second thought, the issue seems to arise because LinkedImageOps always returns 1, even for an empty segment, not because the style is null. I tried out the code as originally done with a null style value and it works as long as length returns the correct value. You can see the difference in this branch: alternative empty seg.

[JordanMartinez]

We'll definitely need to write a guide to help developers implement this code correctly as there are a number of things one needs to be aware of when developing their own custom objects.

[JordanMartinez]

@TomasMikula I believe this custom object PR is now ready to be merged.

[JordanMartinez]

@afester One thought that came to mind today that I haven't thought about / investigated more: will this PR break undo/redo?

[Jugen]

Provided the following three (3) conditions are met undo/redo generally works: 1. The preserveStyle parameter must be TRUE when invoking the GenericStyledArea constructor. 2. A properly implemented Codec for your SEG must be set via GenericStyledArea.setStyleCodecs 3. The Object.equals( Object obj ) method must be overridden and properly implemented by your SEG

[afester]

@Jugen Thanks for that analysis - I believe that this is the kind of information we need to add to the developer's guide which was proposed by @JordanMartinez some time ago.
I did some basic testing with undo/redo and did not observe any issue with the RichText demo (using the LinkedImage segment), but probably more testing is required. I am currently evaluating how to implement the concrete component I have in mind, so I will probably find some additional issues there ...

[JordanMartinez]

Let's move any discussion about the developer guide to the issue I just opened.

[TomasMikula]

Thanks guys for moving things forward! @afester @JordanMartinez are you both happy with the current state of this PR?

[JordanMartinez]

@TomasMikula Glad to see you're back again! Yes, I think this PR is good.

[afester]

@TomasMikula Me too !

Happy holidays

[TomasMikula]

🎉