IEP-1755 Build folder path fails to save on first apply in Launch Configuration#1447
IEP-1755 Build folder path fails to save on first apply in Launch Configuration#1447kolipakakondal merged 1 commit intomasterfrom
Conversation
📝 WalkthroughWalkthroughThe PR relocates the project build folder update logic from the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java`:
- Around line 78-84: The method launchConfigurationChanged currently calls
IDFUtil.updateProjectBuildFolder unconditionally; add the same ownership guard
used in launchConfigurationAdded by early-returning when
CoreBuildLaunchConfigDelegate.getProject(configuration) is null so you only call
IDFUtil.updateProjectBuildFolder for configurations this provider owns, avoiding
unnecessary working-copy allocation and attribute lookups for foreign
configuration types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a12b01bb-f923-4b16-9be3-cd23d25bfde8
📒 Files selected for processing (2)
bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.javabundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/SerialFlashLaunchConfigTabGroup.java
💤 Files with no reviewable changes (1)
- bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/SerialFlashLaunchConfigTabGroup.java
| @Override | ||
| public boolean launchConfigurationChanged(ILaunchConfiguration configuration) throws CoreException | ||
| { | ||
| // nothing to do | ||
| IDFUtil.updateProjectBuildFolder(configuration.getWorkingCopy()); | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm listener fires for all configs and inspect ownsLaunchConfiguration semantics.
fd -e java ownsLaunchConfiguration -x rg -nP -C3 'ownsLaunchConfiguration' {}
rg -nP -C3 'launchConfigurationChanged' --type=java
rg -nP -C5 'getProject\s*\(\s*ILaunchConfiguration' --type=javaRepository: espressif/idf-eclipse-plugin
Length of output: 1112
🏁 Script executed:
# Get the full file to see method implementations
wc -l bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.javaRepository: espressif/idf-eclipse-plugin
Length of output: 194
🏁 Script executed:
# Read the full file with line numbers
cat -n bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.javaRepository: espressif/idf-eclipse-plugin
Length of output: 4398
🏁 Script executed:
# Find ownsLaunchConfiguration method definition
rg -nP 'ownsLaunchConfiguration' bundles/com.espressif.idf.launch.serial.core/src/ --type=java -B2 -A8Repository: espressif/idf-eclipse-plugin
Length of output: 1670
🏁 Script executed:
# Find updateProjectBuildFolder implementation
rg -nP 'void updateProjectBuildFolder|updateProjectBuildFolder' bundles/com.espressif.idf.launch.serial.core/src/ --type=java -B2 -A10Repository: espressif/idf-eclipse-plugin
Length of output: 2038
🏁 Script executed:
# Search for updateProjectBuildFolder across all bundles to find implementation
rg -nP 'updateProjectBuildFolder' --type=java -B2 -A12Repository: espressif/idf-eclipse-plugin
Length of output: 10051
Add ownership check to gate unnecessary processing of foreign configuration types.
launchConfigurationChanged fires for every launch configuration change in the workspace. While the framework will call this method for all configuration types, this provider should only process configurations it owns—mirroring the pattern used in launchConfigurationAdded (line 75).
Although IDFUtil.updateProjectBuildFolder internally returns early if CoreBuildLaunchConfigDelegate.getProject(configuration) returns null (protecting against true "clobbering"), unconditional processing still wastes resources: each call allocates a working copy and performs attribute lookups even for unrelated configuration types.
Recommend early-returning for unowned configurations:
Proposed change
`@Override`
public boolean launchConfigurationChanged(ILaunchConfiguration configuration) throws CoreException
{
+ if (!ownsLaunchConfiguration(configuration))
+ {
+ return false;
+ }
IDFUtil.updateProjectBuildFolder(configuration.getWorkingCopy());
-
return false;
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public boolean launchConfigurationChanged(ILaunchConfiguration configuration) throws CoreException | |
| { | |
| // nothing to do | |
| IDFUtil.updateProjectBuildFolder(configuration.getWorkingCopy()); | |
| return false; | |
| } | |
| `@Override` | |
| public boolean launchConfigurationChanged(ILaunchConfiguration configuration) throws CoreException | |
| { | |
| if (!ownsLaunchConfiguration(configuration)) | |
| { | |
| return false; | |
| } | |
| IDFUtil.updateProjectBuildFolder(configuration.getWorkingCopy()); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java`
around lines 78 - 84, The method launchConfigurationChanged currently calls
IDFUtil.updateProjectBuildFolder unconditionally; add the same ownership guard
used in launchConfigurationAdded by early-returning when
CoreBuildLaunchConfigDelegate.getProject(configuration) is null so you only call
IDFUtil.updateProjectBuildFolder for configurations this provider owns, avoiding
unnecessary working-copy allocation and attribute lookups for foreign
configuration types.
AndriiFilippov
left a comment
AndriiFilippov
left a comment
There was a problem hiding this comment.
@sigmaaa hi !
Tested on:
Linux Ubuntu / Windows 11
the Launch Config does save changes to 'custom build folder' after first launch ✔️
LGTM 👍
3 participants
Description
Moved IDFUtil.updateProjectBuildFolder() from the UI apply phase to the launchConfigurationChanged listener in IDFCoreLaunchConfigProvider. Previously, reading from an unsaved WorkingCopy resulted in saving the old build folder property. Deferring this to the listener ensures the configuration is completely saved to disk before we attempt to update the project properties.
Fixes # (IEP-1755)
Type of change
Please delete options that are not relevant.
How has this been tested?
Edit the launch configuration -> set new custom build folder -> save the changes -> click build -> make sure the build folder is updated immediately.
Create new configuration and set custom build folder -> click build -> make sure the build folder is updated immediately
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit