IEP-1204 Change the default name for the debug configuration #1000
Conversation
WalkthroughThe recent changes focus on updating icon references and enhancing exception handling in an Eclipse plugin. Specifically, icon paths for debugging features were revised, and methods handling project creation and configuration have been refined to improve robustness and clarity. These updates help ensure visual consistency and better error management in project setup workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EclipsePlugin as Eclipse Plugin
participant FileSystem as File System
User ->> EclipsePlugin: Initiate New Project Wizard
EclipsePlugin ->> FileSystem: Update clangd file
FileSystem -->> EclipsePlugin: Clangd file updated (or exception thrown)
EclipsePlugin ->> EclipsePlugin: Handle any exceptions
EclipsePlugin ->> EclipsePlugin: Create or update debug configuration
EclipsePlugin ->> FileSystem: Save new debug configuration
FileSystem -->> EclipsePlugin: Configuration saved
EclipsePlugin -->> User: Wizard finishes setup
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: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xml (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (2 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xml
| String originalName = wizard.getWorkingCopy().getName(); | ||
| String debugConfigName = originalName.substring(0, originalName.lastIndexOf("Configuration")) + " Debug"; //$NON-NLS-1$ //$NON-NLS-2$ | ||
| wizard.getWorkingCopy().copy(debugConfigName).doSave(); |
There was a problem hiding this comment.
Improve reliability of debug configuration name creation.
The method assumes the presence of the substring "Configuration" in the original name. This could lead to errors if "Configuration" is not part of the original name. Adding a check before performing the substring operation would enhance the robustness of this method.
- String debugConfigName = originalName.substring(0, originalName.lastIndexOf("Configuration")) + " Debug";
+ if (originalName.contains("Configuration")) {
+ String debugConfigName = originalName.substring(0, originalName.lastIndexOf("Configuration")) + " Debug";
+ wizard.getWorkingCopy().copy(debugConfigName).doSave();
+ } else {
+ Logger.log("Expected substring 'Configuration' not found in: " + originalName);
+ }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.
| String originalName = wizard.getWorkingCopy().getName(); | |
| String debugConfigName = originalName.substring(0, originalName.lastIndexOf("Configuration")) + " Debug"; //$NON-NLS-1$ //$NON-NLS-2$ | |
| wizard.getWorkingCopy().copy(debugConfigName).doSave(); | |
| String originalName = wizard.getWorkingCopy().getName(); | |
| if (originalName.contains("Configuration")) { | |
| String debugConfigName = originalName.substring(0, originalName.lastIndexOf("Configuration")) + " Debug"; //$NON-NLS-1$ //$NON-NLS-2$ | |
| wizard.getWorkingCopy().copy(debugConfigName).doSave(); | |
| } else { | |
| Logger.log("Expected substring 'Configuration' not found in: " + originalName); | |
| } |
|
@sigmaaa hi ! Tested under: Can't see icon: |
| @@ -39,7 +39,7 @@ | |||
| <extension point="org.eclipse.debug.ui.launchConfigurationTypeImages"> | |||
| <launchConfigurationTypeImage | |||
| configTypeID="com.espressif.idf.debug.gdbjtag.openocd.launchConfigurationType" | |||
| icon="icons/obj16/ESP-IDF GDB OpenOCD Debugging.png" | |||
| icon="platform:/plugin/org.eclipse.debug.ui/icons/full/eview16/debug_persp.png" | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Icon file does not exist
The icon file debug_persp.png specified in the path "platform:/plugin/org.eclipse.debug.ui/icons/full/eview16/debug_persp.png" does not exist in the repository. Please verify the path or ensure that the file is added to the repository.
- File:
bundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xml - Line: 42
Analysis chain
Verify the visibility of the new icon.
The icon path has been updated to "platform:/plugin/org.eclipse.debug.ui/icons/full/eview16/debug_persp.png". However, there is a user comment indicating that the icon is not visible. Please verify the visibility of the new icon and ensure the path is correct.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the visibility of the new icon.
# Test: Check if the icon file exists in the specified path.
icon_path="platform:/plugin/org.eclipse.debug.ui/icons/full/eview16/debug_persp.png"
if [ -f "$icon_path" ]; then
echo "Icon file exists."
else
echo "Icon file does not exist."
fi
Length of output: 231
Script:
#!/bin/bash
# Description: Search for the debug_persp.png file within the org.eclipse.debug.ui plugin directory.
# Locate the directory containing the org.eclipse.debug.ui plugin
plugin_dir=$(fd -t d -g "org.eclipse.debug.ui")
# Search for the debug_persp.png file within the located plugin directory
if [ -n "$plugin_dir" ]; then
fd "debug_persp.png" "$plugin_dir"
else
echo "org.eclipse.debug.ui plugin directory not found."
fi
Length of output: 171
Script:
#!/bin/bash
# Description: Search for the debug_persp.png file in the entire repository.
# Search for the debug_persp.png file in the entire repository
fd "debug_persp.png"
Length of output: 20
|
@sigmaaa hi ! Able to see icon. Do, you think we should change it to make it more uniformly ? |
There was a problem hiding this comment.
Hi @sigmaaa Thanks for the PR. Changes looks great. There are few UX issues. Please check the comments.
| wizard.performFinish(); | ||
|
|
||
| try | ||
| { | ||
| wizard.getWorkingCopy().doSave(); | ||
| String originalName = wizard.getWorkingCopy().getName(); | ||
| String debugConfigName = originalName.substring(0, originalName.lastIndexOf("Configuration")) + " Debug"; //$NON-NLS-1$ //$NON-NLS-2$ |
There was a problem hiding this comment.
We can remove the space before the " Debug" here as this will add the double space considering originalName.substring(0, originalName.lastIndexOf("Configuration")) will still return the project name ends with space after removing "Configuration" from the originalName
| wizard.getWorkingCopy().doSave(); | ||
| String originalName = wizard.getWorkingCopy().getName(); | ||
| String debugConfigName = originalName.substring(0, originalName.lastIndexOf("Configuration")) + " Debug"; //$NON-NLS-1$ //$NON-NLS-2$ | ||
| wizard.getWorkingCopy().copy(debugConfigName).doSave(); |
There was a problem hiding this comment.
After rename, you might need to refresh the project to avoid sync issues with file system project state.
Here is the case:
- Create a project
- Delete a project
- Choose to delete all configurations in the popup window
- Still you will see "project Debug" configurations in the configuration drop-down
There was a problem hiding this comment.
do not see this issue during test.
We can address this as well. |
I haven't found an easy way to change the default name for the additional new debug configuration (when we creating it manually). Dependency classes that we don't have access to correspond to the default configuration name. |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (2)
177-178: Remove the space before "Debug".This will avoid adding double spaces if the original name ends with a space after removing "Configuration".
- String debugConfigName = originalName.substring(0, originalName.lastIndexOf("Configuration")) + "Debug"; + String debugConfigName = originalName.substring(0, originalName.lastIndexOf("Configuration")).trim() + "Debug";
179-179: Refresh the project after renaming to avoid sync issues.After renaming, you might need to refresh the project to avoid sync issues with the file system project state.
+ wizard.getWorkingCopy().copy(debugConfigName).doSave(); + project.refreshLocal(IProject.DEPTH_INFINITE, null);
| String originalName = wizard.getWorkingCopy().getName(); | ||
| int configPartIndex = originalName.lastIndexOf("Configuration"); //$NON-NLS-1$ | ||
| String debugConfigName = configPartIndex != -1 ? originalName.substring(0, configPartIndex) + "Debug" //$NON-NLS-1$ | ||
| : originalName; | ||
| wizard.getWorkingCopy().copy(debugConfigName).doSave(); |
There was a problem hiding this comment.
Improve reliability of debug configuration name creation.
The method assumes the presence of the substring "Configuration" in the original name. This could lead to errors if "Configuration" is not part of the original name. Adding a check before performing the substring operation would enhance the robustness of this method.
- String debugConfigName = originalName.substring(0, originalName.lastIndexOf("Configuration")) + "Debug";
+ if (originalName.contains("Configuration")) {
+ String debugConfigName = originalName.substring(0, originalName.lastIndexOf("Configuration")) + "Debug";
+ wizard.getWorkingCopy().copy(debugConfigName).doSave();
+ } else {
+ Logger.log("Expected substring 'Configuration' not found in: " + originalName);
+ }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.
| String originalName = wizard.getWorkingCopy().getName(); | |
| int configPartIndex = originalName.lastIndexOf("Configuration"); //$NON-NLS-1$ | |
| String debugConfigName = configPartIndex != -1 ? originalName.substring(0, configPartIndex) + "Debug" //$NON-NLS-1$ | |
| : originalName; | |
| wizard.getWorkingCopy().copy(debugConfigName).doSave(); | |
| String originalName = wizard.getWorkingCopy().getName(); | |
| int configPartIndex = originalName.lastIndexOf("Configuration"); //$NON-NLS-1$ | |
| if (originalName.contains("Configuration")) { | |
| String debugConfigName = originalName.substring(0, configPartIndex) + "Debug"; //$NON-NLS-1$ | |
| wizard.getWorkingCopy().copy(debugConfigName).doSave(); | |
| } else { | |
| Logger.log("Expected substring 'Configuration' not found in: " + originalName); | |
| } |
| private void updateClangdFile(IProject project) | ||
| { | ||
| try { | ||
| try | ||
| { | ||
| new ClangdConfigFileHandler().update(project); | ||
| } catch (Exception e) { | ||
| } | ||
| catch (Exception e) | ||
| { |
There was a problem hiding this comment.
Refine exception handling in updateClangdFile method.
Currently, the method catches a generic Exception which might not be the best practice as it can mask other unexpected issues. Consider catching more specific exceptions related to the operations being performed.
- catch (Exception e)
+ catch (IOException | CoreException 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 updateClangdFile(IProject project) | |
| { | |
| try { | |
| try | |
| { | |
| new ClangdConfigFileHandler().update(project); | |
| } catch (Exception e) { | |
| } | |
| catch (Exception e) | |
| { | |
| private void updateClangdFile(IProject project) | |
| { | |
| try | |
| { | |
| new ClangdConfigFileHandler().update(project); | |
| } | |
| catch (IOException | CoreException e) | |
| { |
|
@sigmaaa hi ! LGTM 👍 |
Description
Changed the default debug config name and icon.
Fixes # (IEP-1204)
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
Enhancements
Improvements