Expose custom runtime configuration in settings UI#15
Conversation
|
Hey @Alexanderbtw, thanks for the contribution, the PR looks great! I have a couple of minor notes that I'll add shortly, then we should be good to merge |
smolchanovsky
left a comment
smolchanovsky
left a comment
There was a problem hiding this comment.
Worth knowing: a CoreCLR build can contain multiple JIT variants (clrjit_universal_arm64_x64.dll, clrjit_win_x86_x64.dll, etc) for targeting different architectures, but currently users can only use the default clrjit.dll. Would be a nice follow-up PR if you're interested :)
| val runAppMode: Boolean get() = runAppModeCheckbox.isSelected | ||
| val disassemblyTimeoutSeconds: Int get() = timeoutSpinner.value as Int | ||
| val useCustomRuntime: Boolean get() = useCustomRuntimeCheckbox.isSelected | ||
| val pathToLocalCoreClr: String? get() = pathField.text.takeIf { it.isNotBlank() && useCustomRuntime } |
There was a problem hiding this comment.
Would be great to add a validation to warn the user when "Use Custom Runtime" is checked but the path is empty - otherwise they only find out something's wrong when the disassembler actually runs
| .addLabeledComponent(AsmViewerBundle.message("runtime.timeout.label"), timeoutSpinner.withHelp(timeoutHelp)) | ||
| .addVerticalGap(10) | ||
| .addComponent(useCustomRuntimeCheckbox.withHelp(customRuntimeHelp)) | ||
| .addLabeledComponent(AsmViewerBundle.message("runtime.custom.runtime.path.label"), pathField) |
There was a problem hiding this comment.
The path field stretches to fill the full dialog width as the window resizes - could you constrain its width?
| .addComponent(runAppModeCheckbox.withHelp(runAppModeHelp)) | ||
| .addLabeledComponent(AsmViewerBundle.message("runtime.timeout.label"), timeoutSpinner.withHelp(timeoutHelp)) | ||
| .addVerticalGap(10) | ||
| .addComponent(useCustomRuntimeCheckbox.withHelp(customRuntimeHelp)) |
There was a problem hiding this comment.
The path field below is visually at the same level as the checkbox, making it hard to tell they're related. Would be nice to indent it slightly to show the dependency - same way noRestoreCheckbox is indented under the build radio buttons
| runtime.custom.runtime=Use Custom Runtime (local dotnet/runtime build) | ||
| runtime.custom.runtime.help=Use a locally built dotnet/runtime repo instead of the system .NET SDK. Requires .NET 7+ target framework. | ||
| runtime.custom.runtime.path.label=Path to dotnet/runtime: | ||
| runtime.custom.runtime.path.placeholder=e.g. C:\dotnet\runtime |
There was a problem hiding this comment.
This key doesn't seem to be used anywhere
|
|
||
| public class JitPathUtils | ||
| { | ||
| private static string OsPrefix => |
There was a problem hiding this comment.
Nit: this is a computed property so it calls RuntimeInformation.IsOSPlatform on every access - static readonly field might be more appropriate here
2 participants
No description provided.