Use PDEState target models to compare version to set security manager allow flag #100
Conversation
|
@vik-chand Please review |
| addAllowSecurityManager = true; | ||
| argLength++; // Need to add the argument | ||
|
|
||
| if (isEclipseBundleGreaterThanVersion(4, 24)) { // Don't add allow flags for eclipse before 4.24 |
There was a problem hiding this comment.
The int-arguments are not used. Furthermore I suggest to rename this method to something like isSecurityManagerAllowSupported() and to make the local variable Version comparedVersion = new Version(4, 24, 0); a static constant with an expressive name like SECURITY_MANAGER_ALLOW_MIN_VERSION.
There was a problem hiding this comment.
I have updated the PR to use incoming arguments and avoid List method.
Method name wise I feel this is good enough and there may be other checks in future.
| PDEState pdeState = TargetPlatformHelper.getPDEState(); | ||
| if (pdeState != null) { | ||
| try { | ||
| Optional<IPluginModelBase> platformBaseModel = Arrays.asList(pdeState.getTargetModels()).stream().filter(x -> Objects.nonNull(x.getBundleDescription())).filter(x -> ("org.eclipse.platform").equals(x.getBundleDescription().getSymbolicName()))//$NON-NLS-1$ |
There was a problem hiding this comment.
In reply to the conversation at the same place in the previous PR:
ok, we will still need one not null filter(x -> Objects.nonNull(x.getBundleDescription()) before running through x.getBundleDescription().getSymbolicName().
Yes. Furthermore you could use Arrays.stream() instead of Arrays.asList().stream().
I also prefer to go the normal for each loop way for read and debug but this might be faster I guess and if we provide normal for loop codes, we get suggestions to use new ways to attract contributions.
As somebody who contributes for not very long I think that always using the newest thing just because it is newer does not attract more contributors.
IMHO clean code that uses the 'best' (fastest and especially most readable) style attracts more contributors. Sometimes this is a stream, sometimes a classic for-loop and sometimes a nested block should be refactored into a separate method.
But what's readable is of course also a matter of preference. :)
Besides that: Just checking all target-models is not the 100% right thing to do. Actually you should check the list of plug-ins that participate in the launched application since somebody could also have "org.eclipse.platform" in its workspace and if "org.eclipse.platform" is not a singleton there could be multiple versions in the active TP.
Unfortunately only the sub-classes have a List/Map/Set of actually launched plug-ins. So an abstract method to get the List of launched o.e.platform plug-ins has to be added and that list can be checked (e.g. if all plug-ins are above version 4.24.
The question is if we want to make this method abstract and protected so that all sub-classes of AbstractPDELaunchConfiguration have to implement it or if we not make it abstract and only package and return a default value (that always results either in a true or false result?) only EquinoxLaunchConfiguration and EclipseApplicationLaunchConfiguration can overwrite it with the really launched plug-ins.
There was a problem hiding this comment.
Current change is a good workaround for the problem the user is facing, We can discuss further in a new issue and take the best approach.
@vik-chand WDYT ?
There was a problem hiding this comment.
Created the followup issue #105
vik-chand
dismissed
HannesWell’s stale review
Most of the concerns are taken care of. We can take up other issues in a new PR
fixes #49