IEP-1385 ESP-IDF:MenuConfig: feature uses any sdkconfig file if multiple projects open. #1096
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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
🧹 Outside diff range and nitpick comments (1)
bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/OpenSdkConfigEditor.java (1)
25-25: Add null check for project selectionThe change correctly uses the selected project instead of searching through all projects, which fixes IEP-1385. However, we should add a null check to handle cases where no project is selected in the Project Explorer.
Consider adding a null check:
- IProject project = EclipseUtil.getSelectedProjectInExplorer(); + IProject project = EclipseUtil.getSelectedProjectInExplorer(); + if (project == null) { + MessageDialog.openError(HandlerUtil.getActiveShell(event), + Messages.SDKConfigurationFileNotFound_Title, + "Please select a project in the Project Explorer"); + return null; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/OpenSdkConfigEditor.java(3 hunks)
🔇 Additional comments (2)
bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/OpenSdkConfigEditor.java (2)
14-15: LGTM: Required import for the new project selection approach
Line range hint 33-36: Verify consistency of error handling messages
The error handling looks good, but let's verify if the message constants are consistently used across the codebase.
✅ Verification successful
Error handling follows consistent patterns across the codebase
The verification shows that:
- The error message constants are properly defined in
Messages.javaand localized inmessages.properties - The error dialog pattern
MessageDialog.openErroris consistently used across the codebase with similar structure:- Using appropriate shell context
- Following title + message pattern
- Leveraging the Messages class for localization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these specific message constants and similar error handling patterns
rg -A 2 "SDKConfigurationFileNotFound_Title|SDKConfigFileNotFound_ErrorMessage"
# Look for similar MessageDialog.openError patterns
ast-grep --pattern 'MessageDialog.openError($$$)'
Length of output: 4234
|
@sigmaaa hi ! Tested under: do see error message ✅ @alirana01 please, review |
Description
Providing selected project in the project explorer instead of a first open project with SDK config
Fixes # (IEP-1385)
Type of change
Please delete options that are not relevant.
How has this been tested?
Create two projects -> build one of them -> Try to use menu config command on the project without sdkconfig -> error expected
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes