IEP-1226: added filter for python3 in dialog #947
Conversation
WalkthroughThe recent updates enhance the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java (2 hunks)
Files not reviewed due to errors (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java (no review received)
|
@alirana01 hi ! Tested under: Is it possible to combine both filters into 1 filter without any manual manipulations ? Because still, to display python3.exe I have to switch filter manually. The same works for PATH. I have to open "Browse" first -> then switch filter to python3.exe -> then close "Browse" -> and just then my path to python3 will be recognize by validator. |
I have pushed the changes but beware that now all python*.exe will be shown as the FileDialog can only use simple wildcards and does not support full regex |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/FileParseTag.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFDownloadPage.java
Additional comments not posted (4)
bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1)
30-31: New dependencies added:com.sun.jnaandcom.sun.jna.platform.Ensure these dependencies are necessary for the new functionality and are correctly added.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/FileParseTag.java (3)
18-41: Ensure theWIN32_FIND_DATAstructure is correctly defined.Verify that the structure fields match the expected Windows API structure.
43-51: Ensure theKernel32interface methods are correctly defined.Verify that the methods
FindFirstFileandFindClosematch the expected Windows API methods.
82-85: LGTM! The methodisReparseTagMicrosoftcorrectly checks the reparse tag.
| public static int getReparseTag(String filePath) | ||
| { | ||
| WIN32_FIND_DATA findFileData = new WIN32_FIND_DATA(); | ||
| WinNT.HANDLE hFind = Kernel32.INSTANCE.FindFirstFile(filePath, findFileData); | ||
|
|
||
| if (WinBase.INVALID_HANDLE_VALUE.equals(hFind)) | ||
| { | ||
| Logger.log("FindFirstFile failed: " + Native.getLastError()); //$NON-NLS-1$ | ||
| } | ||
|
|
||
| try | ||
| { | ||
| if ((findFileData.dwFileAttributes.intValue() & WinNT.FILE_ATTRIBUTE_REPARSE_POINT) != 0) | ||
| { | ||
| int reparseTag = findFileData.dwReserved0.intValue(); | ||
| Logger.log("The file has a reparse point. Reparse Tag: 0x" + Integer.toHexString(reparseTag)); //$NON-NLS-1$ | ||
| return reparseTag; | ||
| } | ||
| else | ||
| { | ||
| Logger.log("The file does not have a reparse point."); //$NON-NLS-1$ | ||
| } | ||
| } finally | ||
| { | ||
| Kernel32.INSTANCE.FindClose(hFind); | ||
| } | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling in getReparseTag.
Add error handling for the case when FindFirstFile fails.
- if (WinBase.INVALID_HANDLE_VALUE.equals(hFind))
+ if (WinBase.INVALID_HANDLE_VALUE.equals(hFind) || hFind == null)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| public static int getReparseTag(String filePath) | |
| { | |
| WIN32_FIND_DATA findFileData = new WIN32_FIND_DATA(); | |
| WinNT.HANDLE hFind = Kernel32.INSTANCE.FindFirstFile(filePath, findFileData); | |
| if (WinBase.INVALID_HANDLE_VALUE.equals(hFind)) | |
| { | |
| Logger.log("FindFirstFile failed: " + Native.getLastError()); //$NON-NLS-1$ | |
| } | |
| try | |
| { | |
| if ((findFileData.dwFileAttributes.intValue() & WinNT.FILE_ATTRIBUTE_REPARSE_POINT) != 0) | |
| { | |
| int reparseTag = findFileData.dwReserved0.intValue(); | |
| Logger.log("The file has a reparse point. Reparse Tag: 0x" + Integer.toHexString(reparseTag)); //$NON-NLS-1$ | |
| return reparseTag; | |
| } | |
| else | |
| { | |
| Logger.log("The file does not have a reparse point."); //$NON-NLS-1$ | |
| } | |
| } finally | |
| { | |
| Kernel32.INSTANCE.FindClose(hFind); | |
| } | |
| return -1; | |
| } | |
| public static int getReparseTag(String filePath) | |
| { | |
| WIN32_FIND_DATA findFileData = new WIN32_FIND_DATA(); | |
| WinNT.HANDLE hFind = Kernel32.INSTANCE.FindFirstFile(filePath, findFileData); | |
| if (WinBase.INVALID_HANDLE_VALUE.equals(hFind) || hFind == null) | |
| { | |
| Logger.log("FindFirstFile failed: " + Native.getLastError()); //$NON-NLS-1$ | |
| } | |
| try | |
| { | |
| if ((findFileData.dwFileAttributes.intValue() & WinNT.FILE_ATTRIBUTE_REPARSE_POINT) != 0) | |
| { | |
| int reparseTag = findFileData.dwReserved0.intValue(); | |
| Logger.log("The file has a reparse point. Reparse Tag: 0x" + Integer.toHexString(reparseTag)); //$NON-NLS-1$ | |
| return reparseTag; | |
| } | |
| else | |
| { | |
| Logger.log("The file does not have a reparse point."); //$NON-NLS-1$ | |
| } | |
| } finally | |
| { | |
| Kernel32.INSTANCE.FindClose(hFind); | |
| } | |
| return -1; | |
| } |
| public static boolean isReparseTag(File file) | ||
| { | ||
| if (!Platform.getOS().equals(Platform.OS_WIN32)) | ||
| return false; | ||
| int reparseTag = FileParseTag.getReparseTag(file.getAbsolutePath()); | ||
| if (reparseTag != -1) | ||
| { | ||
| return FileParseTag.isReparseTagMicrosoft(reparseTag); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Ensure platform-specific code is correctly handled in isReparseTag.
Add a comment explaining why the method is specific to Windows.
// This method is specific to Windows as it uses Windows API to check reparse tags.
There was a problem hiding this comment.
Self Review @kolipakakondal @sigmaaa kindly review as well these changes will affect more areas as well since there is now some change in SystemExecutableFinder as well
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/SystemExecutableFinder.java (2 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/WinNativeFileTagOperations.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
Additional comments not posted (8)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/SystemExecutableFinder.java (2)
15-17: New imports forIDFUtilandWinNativeFileTagOperationsare appropriate for the added functionality.
Line range hint
124-130: The remaining part of theisExecutablemethod correctly checks if the file exists and is not a directory.bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/WinNativeFileTagOperations.java (6)
28-53: TheWIN32_FIND_DATAclass is correctly defined with appropriate fields and constructors for JNA structure.
55-63: TheKernel32interface is correctly defined and necessary for native file operations.
65-92: ThegetReparseTagmethod is correctly implemented with proper error handling and logging.
94-97: TheisReparseTagMicrosoftmethod is correctly implemented and straightforward.
99-116: ThefileExistsmethod is correctly implemented with proper error handling and logging.
118-136: TheisDirectorymethod is correctly implemented with proper error handling and logging.
|
|
||
| if (isPlatformWindows() && !file.exists()) | ||
| { | ||
| if (WinNativeFileTagOperations.fileExists(file)) | ||
| { | ||
| return IDFUtil.isReparseTag(file); | ||
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Simplify the nested conditions in the isExecutable method for better readability.
- if (isPlatformWindows() && !file.exists())
- {
- if (WinNativeFileTagOperations.fileExists(file))
- {
- return IDFUtil.isReparseTag(file);
- }
- }
+ if (isPlatformWindows() && !file.exists() && WinNativeFileTagOperations.fileExists(file)) {
+ return IDFUtil.isReparseTag(file);
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (isPlatformWindows() && !file.exists()) | |
| { | |
| if (WinNativeFileTagOperations.fileExists(file)) | |
| { | |
| return IDFUtil.isReparseTag(file); | |
| } | |
| } | |
| if (isPlatformWindows() && !file.exists() && WinNativeFileTagOperations.fileExists(file)) { | |
| return IDFUtil.isReparseTag(file); | |
| } | |
There was a problem hiding this comment.
coderabbit makes a good point here
There was a problem hiding this comment.
@sigmaaa, thank you for your agreement. The suggested change indeed simplifies the nested conditions, making the code more readable. If there are no further concerns, this change can be implemented as suggested.
|
|
||
| if (isPlatformWindows() && !file.exists()) | ||
| { | ||
| if (WinNativeFileTagOperations.fileExists(file)) | ||
| { | ||
| return IDFUtil.isReparseTag(file); | ||
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
coderabbit makes a good point here
|
@alirana01 hi ! Tested under: Filter works good now, able to see both python.exe / python3.exe without any manual filter switching 👍 LGTM 👍 |
Description
Python selection dialog was not showing python3 due to missing filter
Fixes # (IEP-1226)
Type of change
Please delete options that are not relevant.
How has this been tested?
Use esp-idf manager view to add esp-idf and see if in the dialog the python3 filter is visible now
Test Configuration:
Checklist
Summary by CodeRabbit
New Features
Improvements
isExecutablemethod to better handle file existence checks based on the platform.BrowseButtonSelectionAdapterclass for better cross-platform compatibility.