IEP-1074: Python dropdown removed #856
Conversation
WalkthroughThe changes reflect a shift in the user interface of the DirectorySelectionDialog, specifically in how users specify the Python executable. The Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/DirectorySelectionDialog.java (6 hunks)
Additional comments: 7
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/DirectorySelectionDialog.java (7)
6-11: The removal of the
org.eclipse.core.runtime.Platformimport suggests that platform-specific code or checks might have been removed. Verify that this does not affect other parts of the application that may rely on platform checks.18-23: The removal of the
org.eclipse.swt.widgets.Comboimport indicates that the dropdown UI component has been removed. Ensure that no other parts of the application are using this import for different purposes.39-60: The constructor no longer receives a
pythonVersionsmap, which suggests that the dropdown for selecting Python versions has been removed. Ensure that this change is reflected everywhere in the codebase and that no functionality is broken due to the removal of the Python version selection feature.149-195: The new implementation for selecting the Python executable path is introduced here. It is important to ensure that the
FileDialogis correctly configured to filter for executable files, especially on Windows where the Python executable has a.exeextension. Additionally, verify that thepythonLocationtextfield is properly validated to ensure that the path provided is indeed an executable and not a directory or a non-executable file.196-203: The
validatemethod has been updated to check the newpythonLocationtextfield. It is crucial to ensure that the validation logic is robust and correctly identifies valid and invalid paths. Consider edge cases such as symbolic links, environment variables, or relative paths.231-242: The
getPythonPreferenceOrDefaultmethod has been updated to fetch the Python executable path preference. Ensure that this method correctly handles default values and that the preference is stored and retrieved consistently across different platforms.256-263: The
okPressedmethod now saves the paths for the IDF directory, Python executable, and Git executable. It is important to ensure that these paths are correctly saved to preferences and that there are no issues with path separators or special characters on different operating systems.
|
Hi @AndriiFilippov Please verify |
|
@alirana01 hi ! Should be some validation here?
|
Yes will add this |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/DirectorySelectionDialog.java (7 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java
Additional comments: 5
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/DirectorySelectionDialog.java (3)
238-249: The
getPythonPreferenceOrDefaultmethod is a good addition for fetching the Python path preference. However, it's important to ensure that the default value provided is a sensible one, as the method will return this value if the preference is not set. If the default value is not a valid path, it could lead to issues when the path is used later on.263-270: The
okPressedmethod now captures the text frompythonLocationtextandgitLocationtextbefore callingsuper.okPressed(). This is a good practice to ensure that the values are stored before the dialog is closed. However, it's important to ensure that the values are validated before proceeding, as theokPressedmethod is the final step before the dialog is dismissed.290-292: The
getPreferencesmethod is used to fetch the preferences node. It's important to ensure that the node "preference" exists and is the correct place to store these preferences. If the node does not exist or is incorrect, it could lead to preferences not being saved or retrieved correctly.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (2)
9-9: The addition of the message string "DirectorySelectionDialog_InvalidPythonPath" aligns with the changes made in the pull request to handle Python path validation.
11-11: The message key "DirectorySelectionDialog_ChoosePyVersion" might be obsolete due to the removal of the Python version dropdown. Verify if this key is still in use elsewhere or if it should be removed.
| 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); |
There was a problem hiding this comment.
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.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/DirectorySelectionDialog.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/DirectorySelectionDialog.java (7 hunks)
Additional comments: 5
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/DirectorySelectionDialog.java (5)
7-8: The addition of
BufferedReaderandInputStreamReaderimports is appropriate for the new functionality to validate the Python executable.11-11: The removal of the
org.eclipse.core.runtime.Platformimport seems justified as it is not used in the provided code.44-53: The changes in member variables from
pythonVersionCombotopythonLocationtextreflect the shift from a dropdown to a text input for the Python path, which is consistent with the PR description.198-215: The
validatemethod has been updated to include a call tovalidatePythonExecutable, which is a good practice to encapsulate the validation logic. However, the previous comment about enhancing the validation logic to ensure the path points to a Python executable is still valid.265-272: The
okPressedmethod correctly assigns the Python executable path frompythonLocationtext, aligning with the changes made to the dialog.
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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;
}
}
|
@alirana01 hi ! Tested under: able to install tools with different Python versions, build + flash + monitor 👍 |
Description
Python dropdown was causing some confusions to the users so it is now removed on Windows and a simple text box that points to the executable path of the python is kept
Fixes # (IEP-1074)
Type of change
Please delete options that are not relevant.
How has this been tested?
Tested on windows with install tools.
Test Configuration:
Checklist
Summary by CodeRabbit
New Features
Refactor
Documentation