fix: fixing duplicates configs when creating new project #918
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes encompass improvements to the ESP-IDF tooling in Eclipse, focusing on enhancing usability and efficiency. These updates aim to streamline the development workflow by refining the launch configuration process for readability and performance, introducing methods to find existing and debug launch configurations, and updating project handling logic to facilitate easier management and debugging of IoT projects. Changes
Poem
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java (3 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (3 hunks)
Additional comments: 6
bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java (2)
- 28-36: The refactoring of
getLaunchConfigurationimproves readability and efficiency by streamlining the process of obtaining and creating launch configurations. Consider adding comments to explain the logic, especially for the use ofcomputeIfAbsentand the decision-making process for when to create a new configuration. Ensure comprehensive testing to cover all edge cases.- 102-116: The
findExistingLaunchConfigurationmethod correctly searches for existing launch configurations based on the project and descriptor name. However, consider the performance implications of iterating over all launch configurations, especially in environments with a large number of configurations. If performance becomes a concern, you might want to explore more efficient search strategies or caching mechanisms.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (4)
- 11-21: The addition of imports for
ILaunchConfigurationandILaunchManageris necessary for the new functionality introduced in this file, specifically for handling launch configurations. This change is appropriate and aligns with the modifications made in the file.- 118-133: Replacing the
DisposeListenerwith a lambda expression modernizes the code and enhances readability. This change is a good practice that leverages modern Java features to make the code more concise and maintainable.- 139-153: The introduction of
findAppropriateDebugConfigmethod is a logical addition for searching debug configurations based on the project and descriptor. Similar to thefindExistingLaunchConfigurationmethod inIDFCoreLaunchConfigProvider.java, consider the performance implications of iterating over all configurations and explore more efficient search strategies if necessary. Ensure comprehensive testing to cover various scenarios.- 118-133: The updated logic in the
performFinishmethod to handle debug configurations more effectively is well-integrated with the existing flow. It correctly manages the creation and activation of debug configurations. Consider adding comments to explain the logic, especially for the new parts related to debug configurations. Ensure comprehensive testing to cover all edge cases.
alirana01
reviewed
Mar 26, 2024
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java
Outdated
Show resolved
Hide resolved
alirana01
requested changes
Mar 26, 2024
sigmaaa
commented
Mar 26, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LaunchConfigFinder.java (1 hunks)
- bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java (3 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (3 hunks)
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/LaunchConfigFinderTest.java (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LaunchConfigFinder.java
Outdated
Show resolved
Hide resolved
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.core/src/com/espressif/idf/core/util/LaunchConfigFinder.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/LaunchConfigFinder.java
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LaunchConfigFinder.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LaunchConfigFinder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (4)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/LaunchUtil.java (1 hunks)
- bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java (3 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (3 hunks)
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/LaunchConfigFinderTest.java (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java
- tests/com.espressif.idf.core.test/src/com/espressif/idf/core/util/test/LaunchConfigFinderTest.java
Additional Context Used
Comment on lines
+23
to
+36
| public ILaunchConfiguration findAppropriateLaunchConfig(ILaunchDescriptor descriptor, String configIndentifier) | ||
| throws CoreException | ||
| { | ||
| IProject project = descriptor.getAdapter(IProject.class); | ||
| for (ILaunchConfiguration config : launchManager.getLaunchConfigurations()) | ||
| { | ||
| IResource[] mappedResource = config.getMappedResources(); | ||
| if (mappedResource != null && mappedResource.length > 0 && mappedResource[0].getProject().equals(project) | ||
| && config.getType().getIdentifier().contentEquals(configIndentifier)) | ||
| { | ||
| return config; | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
The method findAppropriateLaunchConfig is well-implemented and follows good practices in searching for the appropriate launch configuration. However, consider adding a null check for descriptor at the beginning of the method to prevent potential NullPointerExceptions if a null descriptor is passed. This would enhance the robustness of the method.
public ILaunchConfiguration findAppropriateLaunchConfig(ILaunchDescriptor descriptor, String configIndentifier)
throws CoreException
{
+ if (descriptor == null) {
+ throw new IllegalArgumentException("Descriptor cannot be null");
+ }
IProject project = descriptor.getAdapter(IProject.class);
for (ILaunchConfiguration config : launchManager.getLaunchConfigurations())
{
IResource[] mappedResource = config.getMappedResources();
if (mappedResource != null && mappedResource.length > 0 && mappedResource[0].getProject().equals(project)
&& config.getType().getIdentifier().contentEquals(configIndentifier))
{
return config;
}
}
return 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.
Suggested change
| public ILaunchConfiguration findAppropriateLaunchConfig(ILaunchDescriptor descriptor, String configIndentifier) | |
| throws CoreException | |
| { | |
| IProject project = descriptor.getAdapter(IProject.class); | |
| for (ILaunchConfiguration config : launchManager.getLaunchConfigurations()) | |
| { | |
| IResource[] mappedResource = config.getMappedResources(); | |
| if (mappedResource != null && mappedResource.length > 0 && mappedResource[0].getProject().equals(project) | |
| && config.getType().getIdentifier().contentEquals(configIndentifier)) | |
| { | |
| return config; | |
| } | |
| } | |
| return null; | |
| public ILaunchConfiguration findAppropriateLaunchConfig(ILaunchDescriptor descriptor, String configIndentifier) | |
| throws CoreException | |
| { | |
| if (descriptor == null) { | |
| throw new IllegalArgumentException("Descriptor cannot be null"); | |
| } | |
| IProject project = descriptor.getAdapter(IProject.class); | |
| for (ILaunchConfiguration config : launchManager.getLaunchConfigurations()) | |
| { | |
| IResource[] mappedResource = config.getMappedResources(); | |
| if (mappedResource != null && mappedResource.length > 0 && mappedResource[0].getProject().equals(project) | |
| && config.getType().getIdentifier().contentEquals(configIndentifier)) | |
| { | |
| return config; | |
| } | |
| } | |
| return null; |
|
@AndriiFilippov Could you please check this? |
|
@sigmaaa hi ! Tested under: Followed steps from description -> don't see duplicates are created, config name stay same. able to build - debug.
LGTM 👍 |
AndriiFilippov
approved these changes
Apr 3, 2024
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java
AndriiFilippov
pushed a commit
that referenced
this pull request
Apr 22, 2024
* fix: fixing duplicates configs when creating new project * fix: refactoring code and adding unit tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Steps to reproduce for me:
The problem happens because the default launch config is removed during project deletion and then we can't find it in a map always creating duplicates of the original one. Fixed by looking for existing config via launch manager first before creating a new configuration.
Fixes # (IEP-1187)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
performFinishmethod for better clarity in the project creation wizard.LaunchConfigFinderclass.