-
Notifications
You must be signed in to change notification settings - Fork 5
Enh/UI ext 3150 webui migration excel reader #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates Excel Reader functionality to the modern WebUI framework by adding conversion methods between legacy and modern file chooser filter implementations, and removes an obsolete warning message related to flow variable overrides.
Changes:
- Added conversion methods to transform legacy filter settings to modern
DefaultFileChooserFilters - Exposed previously internal methods and fields to support the migration
- Removed warning message about flow variable overrides being saved as manual settings
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| LegacyMultiFileSelection.java | Added conversion methods to transform legacy filter settings to modern DefaultFileChooserFilters format and made filters field public |
| SettingsApplier.java | Removed obsolete warning message about flow variable overrides |
| LegacyCredentials.java | Added public accessors for credentials and flow variable name |
| DefaultFileChooserFilters.java | Made NameFilterType enum public and added bulk setter method for internal conversions |
| DefaultNodeDialogTest.java | Removed assertion checking for the now-deleted warning message |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...re.ui/src/eclipse/org/knime/node/parameters/persistence/legacy/LegacyMultiFileSelection.java
Outdated
Show resolved
Hide resolved
...se/org/knime/core/webui/node/dialog/defaultdialog/setting/credentials/LegacyCredentials.java
Outdated
Show resolved
Hide resolved
...se/org/knime/core/webui/node/dialog/defaultdialog/setting/credentials/LegacyCredentials.java
Outdated
Show resolved
Hide resolved
091ad7b to
f254f35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public MultiFileChooserFilters m_filters = new MultiFileChooserFilters(); | ||
|
|
||
| /** |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this field public exposes internal state directly. Consider adding a public getter method instead of making the field itself public to maintain better encapsulation and allow for future validation or transformation logic.
| public MultiFileChooserFilters m_filters = new MultiFileChooserFilters(); | |
| /** | |
| private MultiFileChooserFilters m_filters = new MultiFileChooserFilters(); | |
| /** | |
| * Returns the filters to apply when selecting multiple files within a folder. | |
| * | |
| * @return the current multi-file chooser filters | |
| */ | |
| public MultiFileChooserFilters getFilters() { | |
| return m_filters; | |
| } | |
| /** |
| * Set all values at once. To be used by internal conversion methods. | ||
| * | ||
| * @param filterFilesExtension whether to filter files by extension | ||
| * @param filesExtensionExpression the file extension expression | ||
| * @param filesExtensionCaseSensitive whether file extension filtering is case sensitive | ||
| * @param filterFilesName whether to filter files by name | ||
| * @param filesNameExpression the file name filter expression | ||
| * @param filesNameFilterType the file name filter type | ||
| * @param filesNameCaseSensitive whether file name filtering is case sensitive | ||
| * @param includeHiddenFiles whether to include hidden files | ||
| * @param includeSpecialFiles whether to include special files | ||
| * @param filterFoldersName whether to filter folders by name | ||
| * @param foldersNameExpression the folder name filter expression | ||
| * @param foldersNameFilterType the folder name filter type | ||
| * @param foldersNameCaseSensitive whether folder name filtering is case sensitive | ||
| * @param includeHiddenFolders whether to include hidden folders | ||
| * @param followSymlinks whether to follow symbolic links | ||
| */ | ||
| public void setValues(final boolean filterFilesExtension, final String filesExtensionExpression, | ||
| final boolean filesExtensionCaseSensitive, final boolean filterFilesName, final String filesNameExpression, | ||
| final NameFilterType filesNameFilterType, final boolean filesNameCaseSensitive, | ||
| final boolean includeHiddenFiles, final boolean includeSpecialFiles, final boolean filterFoldersName, | ||
| final String foldersNameExpression, final NameFilterType foldersNameFilterType, | ||
| final boolean foldersNameCaseSensitive, final boolean includeHiddenFolders, final boolean followSymlinks) { | ||
| this.m_filterFilesExtension = filterFilesExtension; | ||
| this.m_filesExtensionExpression = filesExtensionExpression; | ||
| this.m_filesExtensionCaseSensitive = filesExtensionCaseSensitive; | ||
| this.m_filterFilesName = filterFilesName; | ||
| this.m_filesNameExpression = filesNameExpression; | ||
| this.m_filesNameFilterType = filesNameFilterType; | ||
| this.m_filesNameCaseSensitive = filesNameCaseSensitive; | ||
| this.m_includeHiddenFiles = includeHiddenFiles; | ||
| this.m_includeSpecialFiles = includeSpecialFiles; | ||
| this.m_filterFoldersName = filterFoldersName; | ||
| this.m_foldersNameExpression = foldersNameExpression; | ||
| this.m_foldersNameFilterType = foldersNameFilterType; | ||
| this.m_foldersNameCaseSensitive = foldersNameCaseSensitive; | ||
| this.m_includeHiddenFolders = includeHiddenFolders; | ||
| this.m_followSymlinks = followSymlinks; |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method has 15 parameters, making it difficult to use and maintain. Consider using a builder pattern or accepting a dedicated data transfer object instead.
| * Set all values at once. To be used by internal conversion methods. | |
| * | |
| * @param filterFilesExtension whether to filter files by extension | |
| * @param filesExtensionExpression the file extension expression | |
| * @param filesExtensionCaseSensitive whether file extension filtering is case sensitive | |
| * @param filterFilesName whether to filter files by name | |
| * @param filesNameExpression the file name filter expression | |
| * @param filesNameFilterType the file name filter type | |
| * @param filesNameCaseSensitive whether file name filtering is case sensitive | |
| * @param includeHiddenFiles whether to include hidden files | |
| * @param includeSpecialFiles whether to include special files | |
| * @param filterFoldersName whether to filter folders by name | |
| * @param foldersNameExpression the folder name filter expression | |
| * @param foldersNameFilterType the folder name filter type | |
| * @param foldersNameCaseSensitive whether folder name filtering is case sensitive | |
| * @param includeHiddenFolders whether to include hidden folders | |
| * @param followSymlinks whether to follow symbolic links | |
| */ | |
| public void setValues(final boolean filterFilesExtension, final String filesExtensionExpression, | |
| final boolean filesExtensionCaseSensitive, final boolean filterFilesName, final String filesNameExpression, | |
| final NameFilterType filesNameFilterType, final boolean filesNameCaseSensitive, | |
| final boolean includeHiddenFiles, final boolean includeSpecialFiles, final boolean filterFoldersName, | |
| final String foldersNameExpression, final NameFilterType foldersNameFilterType, | |
| final boolean foldersNameCaseSensitive, final boolean includeHiddenFolders, final boolean followSymlinks) { | |
| this.m_filterFilesExtension = filterFilesExtension; | |
| this.m_filesExtensionExpression = filesExtensionExpression; | |
| this.m_filesExtensionCaseSensitive = filesExtensionCaseSensitive; | |
| this.m_filterFilesName = filterFilesName; | |
| this.m_filesNameExpression = filesNameExpression; | |
| this.m_filesNameFilterType = filesNameFilterType; | |
| this.m_filesNameCaseSensitive = filesNameCaseSensitive; | |
| this.m_includeHiddenFiles = includeHiddenFiles; | |
| this.m_includeSpecialFiles = includeSpecialFiles; | |
| this.m_filterFoldersName = filterFoldersName; | |
| this.m_foldersNameExpression = foldersNameExpression; | |
| this.m_foldersNameFilterType = foldersNameFilterType; | |
| this.m_foldersNameCaseSensitive = foldersNameCaseSensitive; | |
| this.m_includeHiddenFolders = includeHiddenFolders; | |
| this.m_followSymlinks = followSymlinks; | |
| * Container for all filter values that can be applied at once. | |
| * <p> | |
| * Use {@link DefaultFileChooserFilters.FilterValues.Builder} to create instances. | |
| */ | |
| public static final class FilterValues { | |
| private final boolean m_filterFilesExtension; | |
| private final String m_filesExtensionExpression; | |
| private final boolean m_filesExtensionCaseSensitive; | |
| private final boolean m_filterFilesName; | |
| private final String m_filesNameExpression; | |
| private final NameFilterType m_filesNameFilterType; | |
| private final boolean m_filesNameCaseSensitive; | |
| private final boolean m_includeHiddenFiles; | |
| private final boolean m_includeSpecialFiles; | |
| private final boolean m_filterFoldersName; | |
| private final String m_foldersNameExpression; | |
| private final NameFilterType m_foldersNameFilterType; | |
| private final boolean m_foldersNameCaseSensitive; | |
| private final boolean m_includeHiddenFolders; | |
| private final boolean m_followSymlinks; | |
| private FilterValues(final Builder builder) { | |
| m_filterFilesExtension = builder.m_filterFilesExtension; | |
| m_filesExtensionExpression = builder.m_filesExtensionExpression; | |
| m_filesExtensionCaseSensitive = builder.m_filesExtensionCaseSensitive; | |
| m_filterFilesName = builder.m_filterFilesName; | |
| m_filesNameExpression = builder.m_filesNameExpression; | |
| m_filesNameFilterType = builder.m_filesNameFilterType; | |
| m_filesNameCaseSensitive = builder.m_filesNameCaseSensitive; | |
| m_includeHiddenFiles = builder.m_includeHiddenFiles; | |
| m_includeSpecialFiles = builder.m_includeSpecialFiles; | |
| m_filterFoldersName = builder.m_filterFoldersName; | |
| m_foldersNameExpression = builder.m_foldersNameExpression; | |
| m_foldersNameFilterType = builder.m_foldersNameFilterType; | |
| m_foldersNameCaseSensitive = builder.m_foldersNameCaseSensitive; | |
| m_includeHiddenFolders = builder.m_includeHiddenFolders; | |
| m_followSymlinks = builder.m_followSymlinks; | |
| } | |
| public boolean isFilterFilesExtension() { | |
| return m_filterFilesExtension; | |
| } | |
| public String getFilesExtensionExpression() { | |
| return m_filesExtensionExpression; | |
| } | |
| public boolean isFilesExtensionCaseSensitive() { | |
| return m_filesExtensionCaseSensitive; | |
| } | |
| public boolean isFilterFilesName() { | |
| return m_filterFilesName; | |
| } | |
| public String getFilesNameExpression() { | |
| return m_filesNameExpression; | |
| } | |
| public NameFilterType getFilesNameFilterType() { | |
| return m_filesNameFilterType; | |
| } | |
| public boolean isFilesNameCaseSensitive() { | |
| return m_filesNameCaseSensitive; | |
| } | |
| public boolean isIncludeHiddenFiles() { | |
| return m_includeHiddenFiles; | |
| } | |
| public boolean isIncludeSpecialFiles() { | |
| return m_includeSpecialFiles; | |
| } | |
| public boolean isFilterFoldersName() { | |
| return m_filterFoldersName; | |
| } | |
| public String getFoldersNameExpression() { | |
| return m_foldersNameExpression; | |
| } | |
| public NameFilterType getFoldersNameFilterType() { | |
| return m_foldersNameFilterType; | |
| } | |
| public boolean isFoldersNameCaseSensitive() { | |
| return m_foldersNameCaseSensitive; | |
| } | |
| public boolean isIncludeHiddenFolders() { | |
| return m_includeHiddenFolders; | |
| } | |
| public boolean isFollowSymlinks() { | |
| return m_followSymlinks; | |
| } | |
| /** | |
| * Builder for {@link FilterValues}. | |
| */ | |
| public static final class Builder { | |
| private boolean m_filterFilesExtension; | |
| private String m_filesExtensionExpression; | |
| private boolean m_filesExtensionCaseSensitive; | |
| private boolean m_filterFilesName; | |
| private String m_filesNameExpression; | |
| private NameFilterType m_filesNameFilterType; | |
| private boolean m_filesNameCaseSensitive; | |
| private boolean m_includeHiddenFiles; | |
| private boolean m_includeSpecialFiles; | |
| private boolean m_filterFoldersName; | |
| private String m_foldersNameExpression; | |
| private NameFilterType m_foldersNameFilterType; | |
| private boolean m_foldersNameCaseSensitive; | |
| private boolean m_includeHiddenFolders; | |
| private boolean m_followSymlinks; | |
| public Builder withFilterFilesExtension(final boolean filterFilesExtension) { | |
| m_filterFilesExtension = filterFilesExtension; | |
| return this; | |
| } | |
| public Builder withFilesExtensionExpression(final String filesExtensionExpression) { | |
| m_filesExtensionExpression = filesExtensionExpression; | |
| return this; | |
| } | |
| public Builder withFilesExtensionCaseSensitive(final boolean filesExtensionCaseSensitive) { | |
| m_filesExtensionCaseSensitive = filesExtensionCaseSensitive; | |
| return this; | |
| } | |
| public Builder withFilterFilesName(final boolean filterFilesName) { | |
| m_filterFilesName = filterFilesName; | |
| return this; | |
| } | |
| public Builder withFilesNameExpression(final String filesNameExpression) { | |
| m_filesNameExpression = filesNameExpression; | |
| return this; | |
| } | |
| public Builder withFilesNameFilterType(final NameFilterType filesNameFilterType) { | |
| m_filesNameFilterType = filesNameFilterType; | |
| return this; | |
| } | |
| public Builder withFilesNameCaseSensitive(final boolean filesNameCaseSensitive) { | |
| m_filesNameCaseSensitive = filesNameCaseSensitive; | |
| return this; | |
| } | |
| public Builder withIncludeHiddenFiles(final boolean includeHiddenFiles) { | |
| m_includeHiddenFiles = includeHiddenFiles; | |
| return this; | |
| } | |
| public Builder withIncludeSpecialFiles(final boolean includeSpecialFiles) { | |
| m_includeSpecialFiles = includeSpecialFiles; | |
| return this; | |
| } | |
| public Builder withFilterFoldersName(final boolean filterFoldersName) { | |
| m_filterFoldersName = filterFoldersName; | |
| return this; | |
| } | |
| public Builder withFoldersNameExpression(final String foldersNameExpression) { | |
| m_foldersNameExpression = foldersNameExpression; | |
| return this; | |
| } | |
| public Builder withFoldersNameFilterType(final NameFilterType foldersNameFilterType) { | |
| m_foldersNameFilterType = foldersNameFilterType; | |
| return this; | |
| } | |
| public Builder withFoldersNameCaseSensitive(final boolean foldersNameCaseSensitive) { | |
| m_foldersNameCaseSensitive = foldersNameCaseSensitive; | |
| return this; | |
| } | |
| public Builder withIncludeHiddenFolders(final boolean includeHiddenFolders) { | |
| m_includeHiddenFolders = includeHiddenFolders; | |
| return this; | |
| } | |
| public Builder withFollowSymlinks(final boolean followSymlinks) { | |
| m_followSymlinks = followSymlinks; | |
| return this; | |
| } | |
| public FilterValues build() { | |
| return new FilterValues(this); | |
| } | |
| } | |
| } | |
| /** | |
| * Set all values at once. To be used by internal conversion methods. | |
| * | |
| * @param values the filter values to apply | |
| */ | |
| public void setValues(final FilterValues values) { | |
| m_filterFilesExtension = values.isFilterFilesExtension(); | |
| m_filesExtensionExpression = values.getFilesExtensionExpression(); | |
| m_filesExtensionCaseSensitive = values.isFilesExtensionCaseSensitive(); | |
| m_filterFilesName = values.isFilterFilesName(); | |
| m_filesNameExpression = values.getFilesNameExpression(); | |
| m_filesNameFilterType = values.getFilesNameFilterType(); | |
| m_filesNameCaseSensitive = values.isFilesNameCaseSensitive(); | |
| m_includeHiddenFiles = values.isIncludeHiddenFiles(); | |
| m_includeSpecialFiles = values.isIncludeSpecialFiles(); | |
| m_filterFoldersName = values.isFilterFoldersName(); | |
| m_foldersNameExpression = values.getFoldersNameExpression(); | |
| m_foldersNameFilterType = values.getFoldersNameFilterType(); | |
| m_foldersNameCaseSensitive = values.isFoldersNameCaseSensitive(); | |
| m_includeHiddenFolders = values.isIncludeHiddenFolders(); | |
| m_followSymlinks = values.isFollowSymlinks(); |
af098fb to
200aa2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void setValues(final boolean filterFilesExtension, final String filesExtensionExpression, | ||
| final boolean filesExtensionCaseSensitive, final boolean filterFilesName, final String filesNameExpression, | ||
| final NameFilterType filesNameFilterType, final boolean filesNameCaseSensitive, | ||
| final boolean includeHiddenFiles, final boolean includeSpecialFiles, final boolean filterFoldersName, | ||
| final String foldersNameExpression, final NameFilterType foldersNameFilterType, | ||
| final boolean foldersNameCaseSensitive, final boolean includeHiddenFolders, final boolean followSymlinks) { |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setValues() method has 15 parameters which makes it error-prone to use correctly. Consider using a builder pattern or introducing a parameter object to group related parameters (e.g., file filter settings, folder filter settings) for improved maintainability and reduced risk of parameter ordering mistakes.
06368a5 to
10f5076
Compare
10f5076 to
1da47a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void setValues(final boolean filterFilesExtension, final String filesExtensionExpression, | ||
| final boolean filesExtensionCaseSensitive, final boolean filterFilesName, final String filesNameExpression, | ||
| final NameFilterType filesNameFilterType, final boolean filesNameCaseSensitive, | ||
| final boolean includeHiddenFiles, final boolean includeSpecialFiles, final boolean filterFoldersName, | ||
| final String foldersNameExpression, final NameFilterType foldersNameFilterType, | ||
| final boolean foldersNameCaseSensitive, final boolean includeHiddenFolders, final boolean followSymlinks) { |
Copilot
AI
Jan 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method has 15 parameters, which exceeds common maintainability thresholds. Consider using a builder pattern or accepting a parameter object to improve readability and maintainability.
UIEXT-3150 (WebUI-Migration Excel Reader)
* This warning was visible when legacy credentials were migrated upon showing the password widget. However, it is not useful to the user. UIEXT-3150 (WebUI-Migration Excel Reader)
* Those exceptions were thrown in various workflow tests at this place UIEXT-3150 (WebUI-Migration Excel Reader)
UIEXT-3150 (WebUI-Migration Excel Reader)
1da47a4 to
19f1cb8
Compare
|


No description provided.