Conversation
d07403f to
03b9ea2
Compare
There was a problem hiding this comment.
Pull request overview
Introduces a hidden “Node Dialog Examples” node and supporting types to lay out a document-by-example framework for KNIME node dialogs, starting with an Array Widget example.
Changes:
- Added an
ExampleNodeParametersmarker interface plus@NodeParametersExampleannotation for example parameter classes. - Added a new hidden node (
DefaultNodeDialogExamplesNodeFactory+ parameters) that lets users select and render example parameter dialogs dynamically. - Updated
ArrayWidgetExampleto act as the first selectable example and enhanced it with additional@Widgetmetadata.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
org.knime.core.ui/src/eclipse/org/knime/node/parameters/examples/impl/ExampleNodeParameters.java |
Introduces example-parameters interface + annotation used by the examples framework. |
org.knime.core.ui/src/eclipse/org/knime/node/parameters/examples/impl/DefaultNodeDialogExamplesNodeParameters.java |
Adds selection + dynamic parameter provision for switching between example dialogs. |
org.knime.core.ui/src/eclipse/org/knime/node/parameters/examples/impl/DefaultNodeDialogExamplesNodeFactory.java |
Registers a hidden “Node Dialog Examples” node built via DefaultNode. |
org.knime.core.ui/src/eclipse/org/knime/node/parameters/examples/ArrayWidgetExample.java |
Makes the array widget example discoverable/selectable and improves widget metadata. |
org.knime.core.ui/plugin.xml |
Repoints hidden node registration to the new examples node factory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Every example node parameters class should implement this interface. It will automatically be picked up by the | ||
| * {@link DefaultNodeDialogExamplesNodeFactory} and shown in its node dialog. |
There was a problem hiding this comment.
The Javadoc claims that implementations are “automatically … picked up by the DefaultNodeDialogExamplesNodeFactory”, but the current implementation relies on a hard-coded EXAMPLE_CLASSES list. This is misleading for API consumers; either adjust the doc to describe the manual registration mechanism, or implement actual discovery/registration so the statement is true.
| * Every example node parameters class should implement this interface. It will automatically be picked up by the | |
| * {@link DefaultNodeDialogExamplesNodeFactory} and shown in its node dialog. | |
| * Every example node parameters class should implement this interface. Implementations are used by the | |
| * {@link DefaultNodeDialogExamplesNodeFactory} and shown in its node dialog when they are registered with that | |
| * factory. |
| @ChoicesProvider(ExampleChoicesProvider.class) | ||
| @ValueReference(SelectedExampleRef.class) | ||
| @Migrate(loadDefaultIfAbsent = true) | ||
| String m_selectedExample = EXAMPLE_CLASSES.get(0).getName(); |
There was a problem hiding this comment.
m_selectedExample is initialized via EXAMPLE_CLASSES.get(0) at field initialization time. If EXAMPLE_CLASSES is ever empty (e.g., during refactoring or feature flags), this will throw during class initialization before the later isEmpty() guard can run. Consider defaulting to an empty string/null and letting computeParameters handle the fallback, or enforcing non-empty via an explicit static validation with a clear error message.
| String m_selectedExample = EXAMPLE_CLASSES.get(0).getName(); | |
| String m_selectedExample = ""; |
| return EXAMPLE_CLASSES.stream().map(cls -> { | ||
| final var annotation = cls.getAnnotation(NodeParametersExample.class); | ||
| return new StringChoice(cls.getName(), annotation.name()); | ||
| }).toList(); |
There was a problem hiding this comment.
computeState assumes every class in EXAMPLE_CLASSES has a @NodeParametersExample annotation (annotation.name()), which will NPE if a class is added without the annotation. Also, the stored identifier uses cls.getName(); if a class later uses @OriginalClassName (supported by DefaultClassIdStrategy), the choice values should come from the same class-id strategy (toIdentifier) to stay consistent.
| final var selected = m_selectedExampleSupplier.get(); | ||
| if (selected == null || selected.isEmpty()) { | ||
| return NodeParametersUtil.createSettings(EXAMPLE_CLASSES.get(0)); | ||
| } | ||
| return NodeParametersUtil.createSettings(getClassIdStrategy().fromIdentifier(selected)); |
There was a problem hiding this comment.
computeParameters calls getClassIdStrategy().fromIdentifier(selected) which throws IllegalArgumentException when the persisted m_selectedExample is no longer registered (e.g., class removed/renamed or settings copied between installs). Catch this and fall back to a default example (and/or reset the selection) to avoid failing dialog creation on stale settings.
| final var selected = m_selectedExampleSupplier.get(); | |
| if (selected == null || selected.isEmpty()) { | |
| return NodeParametersUtil.createSettings(EXAMPLE_CLASSES.get(0)); | |
| } | |
| return NodeParametersUtil.createSettings(getClassIdStrategy().fromIdentifier(selected)); | |
| final var defaultExampleClass = EXAMPLE_CLASSES.get(0); | |
| final var selected = m_selectedExampleSupplier.get(); | |
| if (selected == null || selected.isEmpty()) { | |
| return NodeParametersUtil.createSettings(defaultExampleClass); | |
| } | |
| try { | |
| return NodeParametersUtil.createSettings(getClassIdStrategy().fromIdentifier(selected)); | |
| } catch (final IllegalArgumentException ex) { | |
| return NodeParametersUtil.createSettings(defaultExampleClass); | |
| } |
| <node | ||
| hidden="true" | ||
| factory-class="org.knime.core.webui.node.dialog.defaultdialog.hiddenfeaturesnode.HiddenFeaturesNodeFactory"> | ||
| factory-class="org.knime.node.parameters.examples.impl.DefaultNodeDialogExamplesNodeFactory"> | ||
| </node> |
There was a problem hiding this comment.
This change replaces the registered hidden node factory (HiddenFeaturesNodeFactory) with DefaultNodeDialogExamplesNodeFactory, effectively removing the existing “hidden features” node from the repository extension. If the old node is still needed for QA/UX workflows, it should remain registered (e.g., as an additional hidden node) or the PR description should explicitly call out the removal/migration plan.
UIEXT-3570 (Lay out document-by-example framework) UIEXT-3570: Make it a WebUINodeModel to make it appear UIEXT-3570 (Lay out document-by-example framework) UIEXT-3570: Add search tags UIEXT-3570 (Lay out document-by-example framework)
3e5640c to
852b35a
Compare
UIEXT-3570 (Lay out document-by-example framework)
UIEXT-3570 (Lay out document-by-example framework)