Skip to content

feat: introduce core TreeNodeVisitor pattern scaffolding (#262)#312

Open
dyrpsf wants to merge 3 commits into
sbmlteam:masterfrom
dyrpsf:feat/tree-node-visitor
Open

feat: introduce core TreeNodeVisitor pattern scaffolding (#262)#312
dyrpsf wants to merge 3 commits into
sbmlteam:masterfrom
dyrpsf:feat/tree-node-visitor

Conversation

@dyrpsf
Copy link
Copy Markdown
Contributor

@dyrpsf dyrpsf commented May 13, 2026

Description

This PR implements the foundational architecture for the TreeNodeVisitor pattern discussed in #262. It provides a clean, generic way to traverse the JSBML AST without modifying core classes for every new feature.

Key Changes

  • TreeNodeVisitor<T>: Created the core interface with visit(SBase) as the primary method and visit(TreeNodeWithChangeSupport) as the fallback for auxiliary nodes.
  • TreeNodeWithChangeSupport: Added the <T> T accept(TreeNodeVisitor<T> visitor) contract to the root interface.
  • Java 7 Compliance: Since JSBML enforces Java 7 (which does not support default interface methods), the accept method implementation has been explicitly injected into the 18 required base classes (e.g., AbstractSBase, ASTNode, XMLNode, and the various math nodes).

All core tests are passing cleanly locally.

This PR serves purely as the structural scaffolding. Once this architecture is approved and merged, we can begin adapting tools like the AntimonySerializer to actively utilize this visitor!

@dyrpsf
Copy link
Copy Markdown
Contributor Author

dyrpsf commented May 15, 2026

The mock visitor testing is officially in place! I added TreeNodeVisitorTest.java using a custom DummyVisitor to verify the generic <T> scaffolding works perfectly.

To resolve the cascading interface implementation requirements across the JSBML extensions, I added the standard accept method to the foundational AbstractSBasePlugin and AbstractASTNodePlugin classes (as well as RelAbsVector). The entire multi-module project now compiles flawlessly! Let me know if everything looks good to go.

Copy link
Copy Markdown
Member

@draeger draeger left a comment

Choose a reason for hiding this comment

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

It is a bit surprising to me that the inherited method is to be implemented in so many classes. JSBML has a very detailed and abstract type hierarchy. I am convinced it is possible to exploit this hierarchy better so that the interface method is implemented only in a very few top-level classes and can be removed from most more specific classes; in particular, all the specific ASTNode types should inherit the method from a more abstract class. Please check the users' guide again, which contains a class diagram, see https://github.com/sbmlteam/jsbml/raw/master/doc/user_guide/User_Guide.pdf
Also, I left a few comments suggesting that import statements could simplify the type declarations of arguments passed to a method.

Comment thread core/src/org/sbml/jsbml/ext/AbstractASTNodePlugin.java Outdated
Comment thread core/src/org/sbml/jsbml/ext/AbstractSBasePlugin.java Outdated
Comment thread core/src/org/sbml/jsbml/TreeNodeVisitor.java Outdated
Comment thread extensions/render/src/org/sbml/jsbml/ext/render/RelAbsVector.java Outdated
@dyrpsf
Copy link
Copy Markdown
Contributor Author

dyrpsf commented May 16, 2026

Hi @draeger , thanks for the review!

I have implemented all the requested changes:

  • Updated the TreeNodeVisitor Javadoc to explicitly clarify the Void return type usage.
  • Cleaned up the method signatures in RelAbsVector, AbstractSBasePlugin, and AbstractASTNodePlugin to use standard imports instead of fully qualified package prefixes.

The multi-module build is completely clean and passing. Let me know if everything looks good to merge!

@dyrpsf dyrpsf requested a review from draeger May 16, 2026 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants