Skip to content

IEP-1074: Python dropdown removed #856

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.

Sign up for GitHub

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

Merged
kolipakakondal merged 3 commits into from
Dec 6, 2023
Merged

IEP-1074: Python dropdown removed #856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39c2967
IEP-1074: Python dropdown removed
alirana01 Nov 20, 2023
47de176
IEP-1074: Python path validation
alirana01 Nov 27, 2023
4f1f03d
added check to verify python binary
alirana01 Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 67 additions & 72 deletions bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/DirectorySelectionDialog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@
*******************************************************************************/
package com.espressif.idf.ui.update;

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.util.Map;

import org.eclipse.core.runtime.Platform;
import org.eclipse.core.runtime.preferences.ConfigurationScope;
import org.eclipse.jface.dialogs.Dialog;
import org.eclipse.jface.dialogs.IDialogConstants;
Expand All @@ -19,7 +20,6 @@
import org.eclipse.swt.layout.GridData;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Button;
import org.eclipse.swt.widgets.Combo;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.swt.widgets.Control;
import org.eclipse.swt.widgets.DirectoryDialog;
Expand All @@ -41,25 +41,22 @@
public class DirectorySelectionDialog extends TitleAreaDialog
{

private Shell shell;
private Text text;
private String idfDirPath;
private String pythonExecutablePath;
private Combo pythonVersionCombo;
private Map<String, String> pythonVersions;
private String gitPath;
private Text gitLocationtext;
private Text pythonLocationtext;
private String commandId;
private static final String pythonPathNodeKey = "PYTHON_EXECUTABLE"; //$NON-NLS-1$
private static final String gitPathNodeKey = "GIT_EXECUTABLE"; //$NON-NLS-1$

protected DirectorySelectionDialog(Shell parentShell, String commandId, String pythonExecutablePath,
Map<String, String> pythonVersions, String idfPath, String gitExecutablePath)
{
super(parentShell);
setShellStyle(getShellStyle() | SWT.RESIZE);
this.shell = parentShell;
this.pythonExecutablePath = getPythonPreferenceOrDefault(pythonExecutablePath);
this.pythonVersions = pythonVersions;
this.idfDirPath = idfPath;
Expand Down Expand Up @@ -154,76 +151,65 @@ public void widgetSelected(SelectionEvent event)
});

// Python version selection
if (Platform.OS_WIN32.equals(Platform.getOS()) && pythonVersions != null && !pythonVersions.isEmpty())
{
new Label(composite, SWT.NONE).setText(Messages.DirectorySelectionDialog_ChoosePyVersion);
addPythonVersionSelectionControls(composite);

pythonVersionCombo = new Combo(composite, SWT.DROP_DOWN | SWT.READ_ONLY);
GridData gridData = new GridData(SWT.NONE, SWT.NONE, true, false, 2, 1);
gridData.widthHint = 250;
pythonVersionCombo.setLayoutData(gridData);
Dialog.applyDialogFont(composite);
return composite;
}

String[] versions = pythonVersions.keySet().toArray(new String[pythonVersions.size()]);
pythonVersionCombo.setItems(versions);
pythonVersionCombo.select(0); // select the first one
private void addPythonVersionSelectionControls(Composite composite)
{
// Python executable location
new Label(composite, SWT.NONE).setText(Messages.DirectorySelectionDialog_PyExeLocation);

}
else
pythonLocationtext = new Text(composite, SWT.BORDER);
GridData data = new GridData();
data.widthHint = convertHorizontalDLUsToPixels(IDialogConstants.ENTRY_FIELD_WIDTH);
pythonLocationtext.setLayoutData(data);

pythonLocationtext.setText(pythonExecutablePath != null ? pythonExecutablePath : StringUtil.EMPTY);
pythonLocationtext.addModifyListener(new ModifyListener()
{
new Label(composite, SWT.NONE).setText(Messages.DirectorySelectionDialog_PyExeLocation);

pythonLocationtext = new Text(composite, SWT.BORDER);
data = new GridData();
data.widthHint = convertHorizontalDLUsToPixels(IDialogConstants.ENTRY_FIELD_WIDTH);
pythonLocationtext.setLayoutData(data);
pythonLocationtext.setText(pythonExecutablePath != null ? pythonExecutablePath : StringUtil.EMPTY);
pythonLocationtext.addModifyListener(new ModifyListener()
@Override
public void modifyText(ModifyEvent e)
{
@Override
public void modifyText(ModifyEvent e)
{
validate();
}
});
validate();
}
});

Button pyBrowseBtn = new Button(composite, SWT.PUSH);
pyBrowseBtn.setText(Messages.DirectorySelectionDialog_Browse);
pyBrowseBtn.addSelectionListener(new SelectionAdapter()
Button pyBrowseBtn = new Button(composite, SWT.PUSH);
pyBrowseBtn.setText(Messages.DirectorySelectionDialog_Browse);
pyBrowseBtn.addSelectionListener(new SelectionAdapter()
{
@Override
public void widgetSelected(SelectionEvent event)
{
@Override
public void widgetSelected(SelectionEvent event)
FileDialog dlg = new FileDialog(Display.getDefault().getActiveShell());
dlg.setText(Messages.DirectorySelectionDialog_PyExecutableLocation);
String pythonLocationPathString = dlg.open();
if (pythonLocationPathString != null)
{
FileDialog dlg = new FileDialog(Display.getDefault().getActiveShell());
dlg.setText(Messages.DirectorySelectionDialog_PyExecutableLocation);

String dir = dlg.open();
if (dir != null)
{
pythonLocationtext.setText(dir);
}
pythonLocationtext.setText(pythonLocationPathString);
}
});
}

Dialog.applyDialogFont(composite);
return composite;
}
});
}

protected void validate()
{
idfDirPath = text.getText();
if (pythonVersionCombo != null)
{
String version = pythonVersionCombo.getText();
pythonExecutablePath = pythonVersions.getOrDefault(version, null);
}
else
pythonExecutablePath = pythonLocationtext.getText();

gitPath = gitLocationtext.getText();

boolean isValidPythonPath = validatePythonExecutable(pythonExecutablePath);

if (!isValidPythonPath)
{
pythonExecutablePath = pythonLocationtext.getText();
setErrorMessage(Messages.DirectorySelectionDialog_InvalidPythonPath);
getButton(IDialogConstants.OK_ID).setEnabled(false);
}
gitPath = gitLocationtext.getText();

if (StringUtil.isEmpty(pythonExecutablePath) || StringUtil.isEmpty(gitPath) || StringUtil.isEmpty(idfDirPath))
else if (StringUtil.isEmpty(pythonExecutablePath) || StringUtil.isEmpty(gitPath) || StringUtil.isEmpty(idfDirPath))
{
setErrorMessage(Messages.DirectorySelectionDialog_CantbeEmpty);
getButton(IDialogConstants.OK_ID).setEnabled(false);
Comment on lines 198 to 215
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Nov 27, 2023

Choose a reason for hiding this comment

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

The validate method has been updated to include validation for the new Python executable path. However, the validation logic only checks if the path is not empty and if it is a valid executable. It might be beneficial to also check if the path actually points to a Python executable, as the current validation could pass for any executable file.

Expand Down Expand Up @@ -254,12 +240,12 @@ private String getPythonPreferenceOrDefault(String pythonExecutablePath)
{
return getPreferences().get(pythonPathNodeKey, pythonExecutablePath);
}
private String getGitPreferenceOrDefault(String gitExecutablePath)

private String getGitPreferenceOrDefault(String gitExecutablePath)
{
return getPreferences().get(gitPathNodeKey, gitExecutablePath);
}

public String getIDFDirectory()
{
return idfDirPath;
Expand All @@ -279,15 +265,8 @@ public String getGitExecutable()
protected void okPressed()
{
idfDirPath = text.getText();
if (pythonVersionCombo != null)
{
String version = pythonVersionCombo.getText();
pythonExecutablePath = pythonVersions.getOrDefault(version, null);
}
else
{
pythonExecutablePath = pythonLocationtext.getText();
}
pythonExecutablePath = pythonLocationtext.getText();

gitPath = gitLocationtext.getText();

super.okPressed();
Expand All @@ -313,4 +292,20 @@ private Preferences getPreferences()
{
return ConfigurationScope.INSTANCE.getNode(UIPlugin.PLUGIN_ID).node("preference"); //$NON-NLS-1$
}

private boolean validatePythonExecutable(String path)
{
try
{
Process process = new ProcessBuilder(path, "--version").start();
BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));
String output = reader.readLine();
int exitCode = process.waitFor();
return exitCode == 0 && output.startsWith("Python");
}
catch (Exception e)
{
return false;
}
}
Comment on lines +296 to +310
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Nov 27, 2023

Choose a reason for hiding this comment

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

The validatePythonExecutable method is a good addition for validating the Python executable path. It checks if the executable can run and if it outputs a string starting with "Python". However, it might be beneficial to handle the case where the process outputs the version information to the error stream instead of the input stream.

private boolean validatePythonExecutable(String path) {
    try {
        Process process = new ProcessBuilder(path, "--version").start();
        BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));
        BufferedReader errorReader = new BufferedReader(new InputStreamReader(process.getErrorStream()));
        String output = reader.readLine();
        String errorOutput = errorReader.readLine();
        int exitCode = process.waitFor();
        return exitCode == 0 && (output != null && output.startsWith("Python") || errorOutput != null && errorOutput.startsWith("Python"));
    } catch (Exception e) {
        return false;
    }
}

}
1 change: 1 addition & 0 deletions bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public class Messages extends NLS
public static String DirectorySelectionDialog_PyExecutableLocation;
public static String DirectorySelectionDialog_PyExeLocation;
public static String DirectorySelectionDialog_SelectIDFDirMessage;
public static String DirectorySelectionDialog_InvalidPythonPath;
public static String IDFToolsHandler_ToolsManagerConsole;
public static String InstallToolsHandler_AutoConfigureToolchain;
public static String InstallToolsHandler_ConfiguredBuildEnvVarMsg;
Expand Down
1 change: 1 addition & 0 deletions bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ AbstractToolsHandler_ExecutingMsg=Executing
AbstractToolsHandler_RunningCommandFormatString=Running command: %s
DirectorySelectionDialog_Browse=Browse...
DirectorySelectionDialog_CantbeEmpty=Fields can't be empty\!
DirectorySelectionDialog_InvalidPythonPath=Invalid Python Path
DirectorySelectionDialog_CheckTools=Check Tools
DirectorySelectionDialog_ChoosePyVersion=Choose Python version:
DirectorySelectionDialog_GitExecutableLocation=Git Executable Location
Expand Down