Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the R View node to the WebUI-based scripting dialog infrastructure (similar to the R Snippet node), introducing a dedicated NodeParameters model to drive the new dialog and updating the node factory accordingly.
Changes:
- Added
RViewNodeParametersdefining WebUI dialog sections/widgets and custom persistence behavior for nested “R settings”. - Refactored
RViewNodeFactoryto extendAbstractWebUIRSnippetNodeFactoryand integrate the new parameters model while preserving the 3 node views and legacy dialog.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
org.knime.r/src/org/knime/r/RViewNodeParameters.java |
New WebUI parameters model + custom persistors intended to match RViewNodeSettings nested config structure. |
org.knime.r/src/org/knime/r/RViewNodeFactory.java |
Switches factory base class to WebUI scripting factory and adapts model/view/dialog creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JannikRocket
force-pushed
the
enh/AP-25782-web-ui-migration-r-view
branch
from
6ce0e23 to
4abd5a6
Compare
JannikRocket
force-pushed
the
enh/AP-25782-web-ui-migration-r-view
branch
from
4abd5a6 to
b375918
Compare
|
The failing tests are also all failing on the master branch... |
| throw new InvalidSettingsException("Specified color code \"" + colorCode + "\" is not valid!"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I removed this, because it through a visible a exception. And I guess it is enough if there is a warning shown at the node. This is the way we implemented it for the math formula nodes at least...
|
| @Layout(ImageSection.class) | ||
| Void m_imgResolutionWarning; | ||
|
|
||
| static final class ImgResolutionWarningProvider implements StateProvider<Optional<TextMessage.Message>> { |
There was a problem hiding this comment.
This could instead be a PatternValidation on the m_imgResolution field.
| @Layout(AppearanceSection.class) | ||
| Void m_imgBackgroundColorWarning; | ||
|
|
||
| static final class ImgBackgroundColorWarningProvider implements StateProvider<Optional<TextMessage.Message>> { |
There was a problem hiding this comment.
This can use pattern validation instead.
| } | ||
| } | ||
|
|
||
| // --- Script fields (saved under "R settings" sub-config) --- |
There was a problem hiding this comment.
Instead of re-defining all of these, is it possible to simply re-use the parameters class from the "R Snippet"? The RViewNodeSettings also just re-uses the RSnippetSettings.
RSnippetNodeParameters m_rSettings = new RSnippetNodeParameters();| final String colorCode = s.getImageBackgroundColor(); | ||
| if (!colorCode.matches("^#[0-9aAbBcCdDeEfF]{6}")) { | ||
| throw new InvalidSettingsException("Specified color code \"" + colorCode + "\" is not valid!"); | ||
| } |
There was a problem hiding this comment.
Removing the check from the validateSettings method is reasonable because we want the settings to be appliable, even if it is wrong. However, the check should be added to the configure method to ensure that the node cannot be executed if the background color is invalid.
| private final PortType m_portType; | ||
| @SuppressWarnings("restriction") | ||
| public class RViewNodeFactory extends AbstractWebUIRSnippetNodeFactory { | ||
| private final RViewNodeConfig m_viewConfig; |
There was a problem hiding this comment.
There is already the field m_config in the parent class. Make it protected, such that it can be used in createNodeModel and delete this field.
| @SuppressWarnings("unchecked") | ||
| @Override | ||
| public NodeView<RViewNodeModel> createNodeView(final int viewIndex, final RViewNodeModel nodeModel) { | ||
| public NodeView<RSnippetNodeModel> createNodeView(final int viewIndex, final RSnippetNodeModel nodeModel) { |
There was a problem hiding this comment.
Working around the wrong NodeModel class being used for the generic type in NodeFactory is seems wrong.
Instead, adapt the AbstractWebUIRSnippetNodeFactory such that it has a generic to define the type of the model class. E.g.
public abstract class AbstractWebUIRSnippetNodeFactory<T extends RSnippetNodeModel>
extends AbstractFallbackScriptingNodeFactory<T> {
// [...] (replace `RSnippetNodeModel` with `T` where approprate)
@Override
public NodeView<T> createNodeView(final int viewIndex, final T nodeModel) {
// [...]
}
}
This is the Migration of R View, which is based on the https://knime-com.atlassian.net/browse/AP-25737 Node