Comment elements should have a reference to the previous element

[bbottema]

Currently it is impossible to perform context-based formatting, because while formatting a CommentElement, we don't have access to the entire comment structure. For example, in formatLink(..), I would like to perform different kind of formatting based on the preceding CommentElement, which is now impossible.

This can be solved in two ways:

1. Add a field in CommentElement: private final CommentElement previousCommentElement; which is passed in constructor.
2. Add the list of comment elements plus current index to formatLink(..) and all other formatting methods so you have the information needed.

Personally, I find the first solution the cleanest. Only the constructor call is expanded as opposed to expanding all the formatting methods. And 3rd parties creating a custom CommentFormatter can decide where to use this new API if at all.

If you agree this would be a useful addition, please let me know which approach you prefer and I'll get it sorted.

[dnault]

I don't see CommentFormatter as a core part of the library; its purpose is primarily to get people thinking about how they want to handle inline tags, and to provide simple default behavior.

My preference would be to leave CommentElements as they are, and to encourage users with advanced rendering requirements to write custom formatters that don't necessarily extend CommentFormatter. Is that an acceptable resolution?

[bbottema]

Well, that's kind of my point: I'm extending CommentFormatter because I have advanced rendering requirements, but because of the chosen collection structure (Comment implements Iterable<CommentElement>), I'm unable to access other data inside the formatting functions as defined by CommentFormatter.

Unless I do the following:

public class MyFormatter extends CommentFormatter {

    private Comment currentComment;

    @Override
    public String format(Comment comment) {
        // non-thread safe global state that can change in between formatting calls
        currentComment = comment;
        return super.format(comment);
    }

    @Override
    protected String renderLink(InlineLink e) {
        CommentElement previousElement = getPreviousElement(e);

        if (previousElement != null) {
            return "result of processing link in context";
        } else {
            return "result of processing link knowing it is the first item";
        }
    }

    private CommentElement getPreviousElement(InlineLink e) {
        int currentElementIndex = currentComment.getElements().indexOf(e); // :(
        return currentElementIndex == 0 ? null : currentComment.getElements().get(currentElementIndex - 1);
    }
}

But if instead the Comment structure itself was a little more accommodating:

public class MyFormatter extends CommentFormatter {

    @Override
    protected String renderLink(InlineLink e) {
        if (e.getPreviousElement() != null) {
            return "result of processing link in context";
        } else {
            return "result of processing link knowing it is the first item";
        }
    }
}

That results in a much cleaner API for your library users.

It is up to you. I can continue either way.

[bbottema]

In fact I am recursively invoking format() from within a format call and this global Comment field is overwritten in the recursive call (I'm including certain referenced documentation from methods {@link}ed} and the javadoc included from those methods are formatted as well).

In order to fix it now I have to create a new instance for each formatting needed or use a Stack or ArrayDeque to push and pop nested Comment objects in LIFO order. My proposed change would avoid this problem altogether.

[kirkch]

Has a decision been made on this issue? If we are not adding the request then this issue should be closed. If we are adding it, then I am happy to help make it happen too.

When I needed to format the comments, I went the way of creating my own formatter and did not extend the existing one. I just used the existing one for reference to help me figure out what to do. One thing that would have helped me would have been the use of the visitor pattern as that would remove the need for using instanceof in my code and would also notify me at compile time when new types get added.

[dnault]

@kirkch If you'd like to rework the comment formatter to use the visitor pattern, that would be a nice improvement.

[kirkch]

VisitorPattern done. It does not address the request in this issue directly, but it will make it easier for people to create and maintain their own formatters.

[dnault]

Marking this as resolved, due to the CommentVisitor enhancement in https://github.com/dnault/therapi-runtime-javadoc/pull/48