-
Notifications
You must be signed in to change notification settings - Fork 50
Fixed Update-FB-Type quickfix to include SubAppTypes #1999
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: 3.1.x
Are you sure you want to change the base?
Fixed Update-FB-Type quickfix to include SubAppTypes #1999
Conversation
Test Results 110 files ±0 110 suites ±0 1m 11s ⏱️ -1s Results for commit 5772311. ± Comparison against base commit c01798d. This pull request removes 30 and adds 11 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| final DataTypeTreeSelectionDialog dialog = new DataTypeTreeSelectionDialog( | ||
| PlatformUI.getWorkbench().getActiveWorkbenchWindow().getShell(), | ||
| FBTypeSelectionTreeContentProvider.INSTANCE, FBTreeNodeLabelProvider.INSTANCE); | ||
| FBSelectionTreeContentProvider.INSTANCE, FBTreeNodeLabelProvider.INSTANCE); |
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 will also allow to select SubApps and Function FBs for internal FBs. I believe we have to restrict the selection as follows:
- allow only FB types for Base FB types,
- allow only FB and FC types for Composite FB types,
- allow all FB, FC, and SubApp types for SubApp types and systems.
I would suggest to use TypeEntry.getTypeEClass() to determine the class of the type for each marker and show an error when an incompatible type is selected.
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.
Thank you for pointing that out.
However, I'm not sure how to get the marker class using TypeEntry.getTypeEClass.
I would have gotten the markers TypeEntry over the Resource and TypeLibrary
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.
Yes, I would have gotten it that way, as well.
|
|
||
| private boolean isCompatible(final LibraryElement newElement, final IMarker[] markers) { | ||
| return Arrays.stream(markers).map(IMarker::getResource).filter(IFile.class::isInstance).map(IFile.class::cast) | ||
| .map(file -> getTypeLibrary().getTypeEntry(file).getType()) |
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.
Please add a null check/filter for the result of getTypeEntry(file) in case the file has been deleted in the meantime. Please also use TypeEntry.getTypeEClass() instead of getType() to avoid potentially loading the type unnecessaily. I would also suggest to cache the list of EClasses in prepare to avoid re-retrieving them on each selection change.
No description provided.