Skip to content

Conversation

tsmaeder
Copy link
Contributor

This adds a VS Code command that

  1. Opens a "Open Directory" dialog to select the root of a VM install
  2. Allows to select a "Known JRE Name" for the VM install
  3. If all goes well, adds an entry to the user settings

Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems mostly fine. Just be aware this overlaps with https://github.com/microsoft/vscode-java-pack/blob/main/src/java-runtime/index.ts and it probably would have been ideal to move that page here.

canSelectMany: false,
title: 'Select JDK Directory',
});
if (directory) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth validating the directory by at least checking ${directory}/bin/javac exists to ensure it's probably a valid JDK ?


export namespace JavaRuntimes {
export async function initialize(context: vscode.ExtensionContext): Promise<void> {
context.subscriptions.push(vscode.commands.registerCommand('java.runtimes.add', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably add "java.runtimes.add" to Commands.

Copy link
Collaborator

@fbricon fbricon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rebase against the main branch? JavaSE-25 is available now.

const config = vscode.workspace.getConfiguration('java.configuration').get('runtimes');
if (Array.isArray(config)) {
if (config.some(r => r.path === directory[0].fsPath)) {
vscode.window.showErrorMessage(`JDK Directory ${directory[0].fsPath} already configured`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No you need to ensure uniqueness of the jre name.

Screenshot 2025-10-09 at 10 09 08

It's OK having the same JDK used for 2 runtimes, eg. use JDK 11 for JavaSE-11 and JavaSE-1.8

if (config.some(r => r.path === directory[0].fsPath)) {
vscode.window.showErrorMessage(`JDK Directory ${directory[0].fsPath} already configured`);
} else {
const name = await vscode.window.showQuickPick(getSupportedJreNames(), {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would improve it by:

  • detecting the java version of the selected path
  • preselect the matching execution environment name
  • filter out incompatible environments, i.e don't show JavaSE-22 and above if you selected a JDK 21.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also pre Java 8 support has been dropped over a year ago. Can you remove the J2SE-1.5 ... JavaSE-7 values in package.json

export namespace JavaRuntimes {
export async function initialize(context: vscode.ExtensionContext): Promise<void> {
context.subscriptions.push(vscode.commands.registerCommand('java.runtimes.add', async () => {
const directory = await vscode.window.showOpenDialog({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe default to parent folder of last selected JDK (just to make my testing easier)

Comment on lines +32 to +33
vscode.workspace.getConfiguration('java.configuration').update('runtimes', config, vscode.ConfigurationTarget.Global);
vscode.window.showInformationMessage(`JDK Directory ${directory[0].fsPath} added`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be executed only if the quickpick was complete. If I press ESC while the wizard is open, I see the "...added" message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants