Add check for valid Dart SDK to preference page #99
Conversation
closes #83 Signed-off-by: Jonas Hungershausen <[email protected]>
Signed-off-by: Jonas Hungershausen <[email protected]>
| if (!isValid) { | ||
| setErrorMessage(Messages.Preference_SDKNotFound_Message); | ||
| showErrorMessage(); | ||
| } | ||
| return isValid; | ||
| } | ||
|
|
||
| @SuppressWarnings("nls") | ||
| private boolean isValidDartSDK(String location) { | ||
| Path path = Paths.get(location); |
There was a problem hiding this comment.
Are you sure that #88 won't persist with this change?
There was a problem hiding this comment.
Not sure what you mean. This is executed on every keystroke. Could you elaborate?
There was a problem hiding this comment.
I mean #88 will be reintroduced with this change.
Type \ in SDK location field
!ENTRY org.eclipse.ui 4 0 2019-07-13 19:25:00.362
!MESSAGE Unhandled event loop exception
!STACK 0
java.nio.file.InvalidPathException: UNC path is missing hostname: \
at sun.nio.fs.WindowsPathParser.parse(Unknown Source)
at sun.nio.fs.WindowsPathParser.parse(Unknown Source)
at sun.nio.fs.WindowsPath.parse(Unknown Source)
at sun.nio.fs.WindowsFileSystem.getPath(Unknown Source)
at java.nio.file.Paths.get(Unknown Source)
at org.eclipse.dartboard.preference.DartSDKLocationFieldEditor.isValidDartSDK(DartSDKLocationFieldEditor.java:40)
at org.eclipse.dartboard.preference.DartSDKLocationFieldEditor.doCheckState(DartSDKLocationFieldEditor.java:30)
at org.eclipse.dartboard.preference.DartPreferencePage.lambda$1(DartPreferencePage.java:151)
at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:183)
at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4131)
at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1056)
at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1080)
at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1061)
at org.eclipse.swt.widgets.Text.wmCommandChild(Text.java:2884)
at org.eclipse.swt.widgets.Control.WM_COMMAND(Control.java:4906)
at org.eclipse.swt.widgets.Control.windowProc(Control.java:4762)
at org.eclipse.swt.widgets.Display.windowProc(Display.java:4778)
at org.eclipse.swt.internal.win32.OS.CallWindowProc(Native Method)
at org.eclipse.swt.widgets.Text.callWindowProc(Text.java:265)
at org.eclipse.swt.widgets.Control.windowProc(Control.java:4856)
at org.eclipse.swt.widgets.Text.windowProc(Text.java:2528)
at org.eclipse.swt.widgets.Display.windowProc(Display.java:4770)
at org.eclipse.swt.internal.win32.OS.DispatchMessage(Native Method)
at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3545)
at org.eclipse.jface.window.Window.runEventLoop(Window.java:823)
at org.eclipse.jface.window.Window.open(Window.java:799)
at org.eclipse.ui.internal.OpenPreferencesAction.run(OpenPreferencesAction.java:66)
at org.eclipse.jface.action.Action.runWithEvent(Action.java:474)
at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:580)
at org.eclipse.jface.action.ActionContributionItem.lambda$4(ActionContributionItem.java:412)
at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4131)
at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1056)
at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:3944)
at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3547)
at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1173)
at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1062)
at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155)
at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:635)
at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:559)
at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:150)
at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:155)
at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:137)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:107)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:400)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
at java.lang.reflect.Method.invoke(Unknown Source)
at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:660)
at org.eclipse.equinox.launcher.Main.basicRun(Main.java:597)
at org.eclipse.equinox.launcher.Main.run(Main.java:1468)
at org.eclipse.equinox.launcher.Main.main(Main.java:1441)
There was a problem hiding this comment.
Could you try again? I added a catch for the Exception, as I'm not sure how to resolve this otherwise.
| } | ||
|
|
||
| String[] commands; | ||
| if (Platform.OS_WIN32.equals(Platform.getOS())) { |
There was a problem hiding this comment.
We can write like this also
String[] commands = PlatformUtil.isWindows()? new String[] {"cmd", "/c", executablePath + " --version": new String[] {"/bin/bash", "-c", executablePath + " --version"};
add a method for OS check in PlatformUtil class.
There was a problem hiding this comment.
This ternary operator makes it very hard to read. I like the If-else better.
We don't have a PlatformUtil class yet. Only PlatformUIUtil which doesn't seem to fit for this case. I don't like adding all the util classes with only one or two util method. For now I think this is fine. In the future we could also use a utility method to just pass a command to a function and it then executes the appropriate process.. But that's out of scope of this change IMO.
Does this work on Windows btw?
There was a problem hiding this comment.
Seems to be validation broken on windows. I can see validation error for valid installation.
There was a problem hiding this comment.
Yeah, I thought so. Could you check how we would have to execute a command with cmd on windows? The command should look like this: cmd /c C:\path\to\dart --version
There was a problem hiding this comment.
'C:\Program' is not recognized as an internal or external command,
There was a problem hiding this comment.
Change to commands = new String[] { "cmd", executablePath + " --version" };
There was a problem hiding this comment.
Something wrong in the windows side. It's returned windows version
Microsoft Windows [Version 10.0.18362.239]
There was a problem hiding this comment.
Something wrong in the windows side. It's returned windows version
What do you mean? Does Platform.OS_WIN32.equals(Platform.getOS()) evaluate to false?
There was a problem hiding this comment.
No. command has the wrong syntax
I suggest this code for windows: fix. Make sure working for other OSes.
boolean isWindows = Platform.OS_WIN32.equals(Platform.getOS());
String dartFile = isWindows ? "dart.exe" : "dart";
String executablePath = null;
if (path.endsWith("bin" + File.separator + dartFile)) {
executablePath = path.toAbsolutePath().toString();
} else if (path.endsWith("bin")) {
executablePath = path.toAbsolutePath().toString() + File.separator + dartFile;
} else {
executablePath = path.toAbsolutePath().toString() + File.separator + "bin" + File.separator + dartFile;
}
String[] commands;
if (isWindows) {
commands = new String[] { "cmd", "/c", executablePath, "--version" };
} else {
commands = new String[] { "/bin/bash", "-c", executablePath + "--version" };
}
There was a problem hiding this comment.
I removed the part with the if-else if-else because it allowed a few special locations through that weren't actual Dart SDK locations. I also added the correct syntax for windows. Please check again, if it works.
org.eclipse.dartboard/src/org/eclipse/dartboard/preference/DartSDKLocationFieldEditor.java
Show resolved
Hide resolved
Signed-off-by: Jonas Hungershausen <[email protected]>
Signed-off-by: Jonas Hungershausen <[email protected]>
Signed-off-by: Jonas Hungershausen <[email protected]>
Signed-off-by: Jonas Hungershausen <[email protected]>
org.eclipse.dartboard/src/org/eclipse/dartboard/preference/DartSDKLocationFieldEditor.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Jonas Hungershausen <[email protected]>
|
|
||
| String[] commands; | ||
| if (isWindows) { | ||
| commands = new String[] { "cmd", executablePath + ".exe --version" }; |
There was a problem hiding this comment.
Change this line to commands = new String[] { "cmd", "/c", executablePath, "--version" };
Signed-off-by: Jonas Hungershausen <[email protected]>
org.eclipse.dartboard/src/org/eclipse/dartboard/preference/DartSDKLocationFieldEditor.java
Outdated
Show resolved
Hide resolved
org.eclipse.dartboard/src/org/eclipse/dartboard/preference/DartSDKLocationFieldEditor.java
Outdated
Show resolved
Hide resolved
This patch adds a check to the preference page as-you-type validation.
One thing to answer:
Is there a better way to spawn processes? In this patch we would spawn a new process with
$ dart --versionfor every file/directory entered. Any suggestions?