IEP-1262 Changing the configuration should also change the mode #993
Conversation
WalkthroughThe recent changes enhance launch handling, refactor code for improved readability, and update methods for better consistency and efficiency. Key modifications include adding imports, introducing and updating methods, and improving implementations in multiple files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IDE
participant LaunchBarListener
participant ILaunchManager
User->>IDE: Change active launch configuration
IDE->>LaunchBarListener: Notify activeLaunchDescriptorChanged
LaunchBarListener->>ILaunchManager: Retrieve specific launch mode
ILaunchManager-->>LaunchBarListener: Return the retrieved mode
LaunchBarListener-->>IDE: Update the launch mode
sequenceDiagram
participant User
participant IDE
participant RunActionHandler
User->>IDE: Execute a run action
IDE->>RunActionHandler: Trigger execute method
RunActionHandler->>RunActionHandler: Determine descriptor count
alt Single descriptor
RunActionHandler->>RunActionHandler: setLaunchBarWithFirstAppropriateDescriptor
else Multiple descriptors
RunActionHandler->>RunActionHandler: runActionBasedOnDescCount
end
RunActionHandler-->>IDE: Complete execution
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.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java (4 hunks)
Additional comments not posted (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java (2)
9-10: The new importsOptional,Stream,ILaunchManager, andILaunchModeare appropriate for the functionalities being implemented. Ensure that these classes are being used optimally to justify their inclusion.Also applies to: 22-23
56-85: The updatedactiveLaunchDescriptorChangedmethod now effectively handles different launch configurations. However, the method could be refactored to improve readability and maintainability by extracting the conditional logic into a separate method.
[REFACTOR_SUGGESTion]- if (IDFLaunchConstants.RUN_LAUNCH_CONFIG_TYPE.equals(configTypeIdentifier)) - { - setMode(launchBarManager, ILaunchManager.RUN_MODE); - } - else if (IDFLaunchConstants.DEBUG_LAUNCH_CONFIG_TYPE.equals(configTypeIdentifier)) - { - setMode(launchBarManager, ILaunchManager.DEBUG_MODE); - } + setModeBasedOnConfigType(launchBarManager, configTypeIdentifier);And then define a new method:
private void setModeBasedOnConfigType(ILaunchBarManager launchBarManager, String configTypeIdentifier) { if (IDFLaunchConstants.RUN_LAUNCH_CONFIG_TYPE.equals(configTypeIdentifier)) { setMode(launchBarManager, ILaunchManager.RUN_MODE); } else if (IDFLaunchConstants.DEBUG_LAUNCH_CONFIG_TYPE.equals(configTypeIdentifier)) { setMode(launchBarManager, ILaunchManager.DEBUG_MODE); } }
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java (4 hunks)
Additional comments not posted (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java (1)
9-10: Imports Approved.The newly added imports are necessary for the new functionality and are correctly included.
Also applies to: 22-23
| private void setMode(ILaunchBarManager launchBarManager, String mode) | ||
| { | ||
| try | ||
| { | ||
| Optional<ILaunchMode> runMode = Stream.of(launchBarManager.getLaunchModes()) | ||
| .filter(m -> m.getIdentifier().equals(mode)).findFirst(); | ||
| if (runMode.isPresent()) | ||
| { | ||
| launchBarManager.setActiveLaunchMode(runMode.get()); | ||
| } | ||
|
|
||
| } | ||
| catch (CoreException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Enhance Error Logging in setMode.
The error logging in the catch block can be improved to provide more context about the error.
- Logger.log(e);
+ Logger.log("Error setting launch mode", e);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.
| private void setMode(ILaunchBarManager launchBarManager, String mode) | |
| { | |
| try | |
| { | |
| Optional<ILaunchMode> runMode = Stream.of(launchBarManager.getLaunchModes()) | |
| .filter(m -> m.getIdentifier().equals(mode)).findFirst(); | |
| if (runMode.isPresent()) | |
| { | |
| launchBarManager.setActiveLaunchMode(runMode.get()); | |
| } | |
| } | |
| catch (CoreException e) | |
| { | |
| Logger.log(e); | |
| } | |
| } | |
| private void setMode(ILaunchBarManager launchBarManager, String mode) | |
| { | |
| try | |
| { | |
| Optional<ILaunchMode> runMode = Stream.of(launchBarManager.getLaunchModes()) | |
| .filter(m -> m.getIdentifier().equals(mode)).findFirst(); | |
| if (runMode.isPresent()) | |
| { | |
| launchBarManager.setActiveLaunchMode(runMode.get()); | |
| } | |
| } | |
| catch (CoreException e) | |
| { | |
| Logger.log("Error setting launch mode", e); | |
| } | |
| } |
| public void activeLaunchDescriptorChanged(ILaunchDescriptor descriptor) | ||
| { | ||
| descriptor.getAdapter(ILaunchConfiguration.class); | ||
| ILaunchBarManager launchBarManager = IDFCorePlugin.getService(ILaunchBarManager.class); | ||
|
|
||
| try | ||
| { | ||
| ILaunchConfiguration activeLaunchConfiguration = launchBarManager.getActiveLaunchConfiguration(); | ||
|
|
||
| if (activeLaunchConfiguration != null && activeLaunchConfiguration.getType() != null) | ||
| { | ||
| String configTypeIdentifier = activeLaunchConfiguration.getType().getIdentifier(); | ||
| if (IDFLaunchConstants.RUN_LAUNCH_CONFIG_TYPE.equals(configTypeIdentifier)) | ||
| { | ||
| // Set debug mode first to ensure a mode change, triggering listeners. | ||
| setMode(launchBarManager, ILaunchManager.DEBUG_MODE); | ||
| setMode(launchBarManager, ILaunchManager.RUN_MODE); | ||
| } | ||
| else if (IDFLaunchConstants.DEBUG_LAUNCH_CONFIG_TYPE.equals(configTypeIdentifier)) | ||
| { | ||
| // Set run mode first to ensure a mode change, triggering listeners. | ||
| setMode(launchBarManager, ILaunchManager.RUN_MODE); | ||
| setMode(launchBarManager, ILaunchManager.DEBUG_MODE); | ||
| } | ||
| } | ||
| } | ||
| catch (CoreException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
|
|
||
| ILaunchBarListener.super.activeLaunchDescriptorChanged(descriptor); | ||
| } |
There was a problem hiding this comment.
Enhance Error Logging in activeLaunchDescriptorChanged.
The error logging in the catch block can be improved to provide more context about the error.
- Logger.log(e);
+ Logger.log("Error in activeLaunchDescriptorChanged", e);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.
| public void activeLaunchDescriptorChanged(ILaunchDescriptor descriptor) | |
| { | |
| descriptor.getAdapter(ILaunchConfiguration.class); | |
| ILaunchBarManager launchBarManager = IDFCorePlugin.getService(ILaunchBarManager.class); | |
| try | |
| { | |
| ILaunchConfiguration activeLaunchConfiguration = launchBarManager.getActiveLaunchConfiguration(); | |
| if (activeLaunchConfiguration != null && activeLaunchConfiguration.getType() != null) | |
| { | |
| String configTypeIdentifier = activeLaunchConfiguration.getType().getIdentifier(); | |
| if (IDFLaunchConstants.RUN_LAUNCH_CONFIG_TYPE.equals(configTypeIdentifier)) | |
| { | |
| // Set debug mode first to ensure a mode change, triggering listeners. | |
| setMode(launchBarManager, ILaunchManager.DEBUG_MODE); | |
| setMode(launchBarManager, ILaunchManager.RUN_MODE); | |
| } | |
| else if (IDFLaunchConstants.DEBUG_LAUNCH_CONFIG_TYPE.equals(configTypeIdentifier)) | |
| { | |
| // Set run mode first to ensure a mode change, triggering listeners. | |
| setMode(launchBarManager, ILaunchManager.RUN_MODE); | |
| setMode(launchBarManager, ILaunchManager.DEBUG_MODE); | |
| } | |
| } | |
| } | |
| catch (CoreException e) | |
| { | |
| Logger.log(e); | |
| } | |
| ILaunchBarListener.super.activeLaunchDescriptorChanged(descriptor); | |
| } | |
| public void activeLaunchDescriptorChanged(ILaunchDescriptor descriptor) | |
| { | |
| descriptor.getAdapter(ILaunchConfiguration.class); | |
| ILaunchBarManager launchBarManager = IDFCorePlugin.getService(ILaunchBarManager.class); | |
| try | |
| { | |
| ILaunchConfiguration activeLaunchConfiguration = launchBarManager.getActiveLaunchConfiguration(); | |
| if (activeLaunchConfiguration != null && activeLaunchConfiguration.getType() != null) | |
| { | |
| String configTypeIdentifier = activeLaunchConfiguration.getType().getIdentifier(); | |
| if (IDFLaunchConstants.RUN_LAUNCH_CONFIG_TYPE.equals(configTypeIdentifier)) | |
| { | |
| // Set debug mode first to ensure a mode change, triggering listeners. | |
| setMode(launchBarManager, ILaunchManager.DEBUG_MODE); | |
| setMode(launchBarManager, ILaunchManager.RUN_MODE); | |
| } | |
| else if (IDFLaunchConstants.DEBUG_LAUNCH_CONFIG_TYPE.equals(configTypeIdentifier)) | |
| { | |
| // Set run mode first to ensure a mode change, triggering listeners. | |
| setMode(launchBarManager, ILaunchManager.RUN_MODE); | |
| setMode(launchBarManager, ILaunchManager.DEBUG_MODE); | |
| } | |
| } | |
| } | |
| catch (CoreException e) | |
| { | |
| Logger.log("Error in activeLaunchDescriptorChanged", e); | |
| } | |
| ILaunchBarListener.super.activeLaunchDescriptorChanged(descriptor); | |
| } |
|
@sigmaaa hi ! change the configuration it automatically sets the mode to Run/Debug accordingly ✔️ |
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java
Outdated
Show resolved
Hide resolved
|
Follow-up behavior - Would you also like to handle the case where changing the mode (Run to Debug and vice versa) updates the launch configuration in the dropdown menu? I understand we currently handle this during launch with a pop-up, but if there's only one configuration, we could simply switch it and eliminate the pop-up choice for the user.
|
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/LaunchBarListener.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/LaunchBarListener.java
Hi @kolipakakondal, thanks for the review. Implemented in the latest PR |
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/handlers/RunActionHandler.java (6 hunks)
Additional comments not posted (6)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/RunActionHandler.java (6)
10-10: Import forStreamis appropriate.The import for
Streamis necessary for stream processing in the new methods.
16-16: Import forIStatusis appropriate.The import for
IStatusis necessary for status reporting in the new methods.
70-73: Improvement in theexecutemethod.Using
Boolean.TRUE.equals(isYes)improves readability and reduces the risk of NullPointerException.
111-117: New methodrunActionBasedOnDescCountimproves modularity.The method encapsulates logic for handling actions based on the count of suitable descriptors, enhancing code readability and modularity.
119-134: New methodsetLaunchBarWithFirstAppropriateDescriptorimproves modularity.The method encapsulates logic for setting the launch bar with the first appropriate descriptor, enhancing code readability and modularity.
161-161: Simplification infindSuitableDescNamesmethod.Using
toList()instead ofcollect(Collectors.toList())simplifies the code and leverages the new method introduced in Java 16.
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/dialogs/DeleteResourceWizard.java (4 hunks)
Additional comments not posted (10)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/DeleteResourceWizard.java (10)
69-72: LGTM! Improved readability and maintainability.The added braces and adjusted spacing enhance the readability and maintainability of the constructor.
75-79: LGTM! Improved readability and maintainability.The added braces and adjusted spacing enhance the readability and maintainability of the method.
Line range hint
101-297: LGTM! Improved readability, maintainability, and functionality.The added braces, adjusted spacing, and enhanced implementation improve the readability, maintainability, and functionality of the method.
Line range hint
298-319: LGTM! Improved readability, maintainability, and functionality.The added braces, adjusted spacing, and improved implementation enhance the readability, maintainability, and functionality of the method.
320-336: LGTM! Improved readability and maintainability.The added braces and adjusted spacing enhance the readability and maintainability of the method.
337-348: LGTM! Improved readability and maintainability.The added braces and adjusted spacing enhance the readability and maintainability of the method.
350-364: LGTM! Improved readability and maintainability.The added braces and adjusted spacing enhance the readability and maintainability of the method.
376-422: LGTM! Improved readability, maintainability, and functionality.The added braces, adjusted spacing, and enhanced implementation improve the readability, maintainability, and functionality of the method.
425-434: LGTM! Improved functionality.The added call to
deleteRelatedConfigurationsensures that related configurations are deleted when necessary, improving the functionality of the method.
435-452: LGTM! Well-implemented method.The
deleteRelatedConfigurationsmethod is well-implemented and improves functionality by ensuring related configurations are deleted when necessary.
Hi @sigmaaa I could still see the same issue where changing the mode is not changing the configuration. Can you please check. @AndriiFilippov fyi. |
Hi @kolipakakondal, this is the expected behavior for this PR since the goal of this PR was to change the mode based on the configuration, not vice versa. To change the configuration, click run or debug with the wrong configuration. |
|
Hi @sigmaaa I'm happy if you would like to revist the existing PR on this problem and fix this issue. |
Description
Now, when we change the configuration it automatically sets the mode to Run/Debug accordingly
Fixes # (IEP-1262)
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
New Features
Refactor
Bug Fixes
Style