IEP-1110 NPE when trying to create new launch configuration #852
Conversation
WalkthroughThe changes primarily focus on enhancing the robustness of the code in various Java classes within the Changes
Poem
TipsChat with CodeRabbit Bot (
|
![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: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java (1 hunks)
- bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java (2 hunks)
| String selectedTarget = StringUtil.EMPTY; | ||
| try | ||
| { | ||
| selectedTarget = launchBarManager.getActiveLaunchTarget().getAttribute(IDFLaunchConstants.ATTR_IDF_TARGET, | ||
| ""); //$NON-NLS-1$ | ||
| if (launchBarManager.getActiveLaunchConfiguration() != null) | ||
| { | ||
| selectedTarget = launchBarManager.getActiveLaunchTarget() | ||
| .getAttribute(IDFLaunchConstants.ATTR_IDF_TARGET, StringUtil.EMPTY); | ||
| } | ||
|
|
There was a problem hiding this comment.
The null check for launchBarManager.getActiveLaunchConfiguration() is good for preventing Null Pointer Exceptions. However, it would be better to also check if launchBarManager.getActiveLaunchTarget() is not null before calling getAttribute on it. This will further enhance the robustness of the code.
- if (launchBarManager.getActiveLaunchConfiguration() != null)
+ if (launchBarManager.getActiveLaunchConfiguration() != null && launchBarManager.getActiveLaunchTarget() != null)Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| String selectedTarget = StringUtil.EMPTY; | |
| try | |
| { | |
| selectedTarget = launchBarManager.getActiveLaunchTarget().getAttribute(IDFLaunchConstants.ATTR_IDF_TARGET, | |
| ""); //$NON-NLS-1$ | |
| if (launchBarManager.getActiveLaunchConfiguration() != null) | |
| { | |
| selectedTarget = launchBarManager.getActiveLaunchTarget() | |
| .getAttribute(IDFLaunchConstants.ATTR_IDF_TARGET, StringUtil.EMPTY); | |
| } | |
| String selectedTarget = StringUtil.EMPTY; | |
| try | |
| { | |
| if (launchBarManager.getActiveLaunchConfiguration() != null && launchBarManager.getActiveLaunchTarget() != null) | |
| { | |
| selectedTarget = launchBarManager.getActiveLaunchTarget() | |
| .getAttribute(IDFLaunchConstants.ATTR_IDF_TARGET, StringUtil.EMPTY); | |
| } |
...ssif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java
Show resolved
Hide resolved
|
@sigmaaa hi ! Tested under: Test 1: 1) Close project -> try to create new Launch Config -> get error -> close window -> restart -> try to create new Launch Config -> clear error message. 👍 Test 2: create project -> build project -> Close project -> try to create new Launch Config -> get error -> close window -> restart -> try to create new Launch Config -> get error -> restart -> try to create new Launch Config -> get error -> Open Project -> try to create new Launch Config -> get error: STILL PERSIST. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabMain.java (1 hunks)
- bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java (3 hunks)
Additional comments: 2
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabMain.java (1)
- 78-84: The null check for
projDesandprojDes.getActiveConfiguration()is a good practice to prevent potential Null Pointer Exceptions. This change enhances the robustness of the code.bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java (1)
- 495-498: The null checks for
projDesandprojDes.getActiveConfiguration()are good for preventing NullPointerExceptions. No changes needed.
| if (launchBarManager.getActiveLaunchConfiguration() != null) { | ||
| return t.contentEquals(launchBarManager.getActiveLaunchTarget().getId()); | ||
| } |
There was a problem hiding this comment.
The null check for getActiveLaunchConfiguration() is good for preventing NullPointerExceptions. However, consider storing the result of getActiveLaunchTarget() in a local variable to avoid calling it twice, which could potentially improve performance.
- if (launchBarManager.getActiveLaunchConfiguration() != null) {
- return t.contentEquals(launchBarManager.getActiveLaunchTarget().getId());
- }
+ LaunchTarget activeLaunchTarget = launchBarManager.getActiveLaunchTarget();
+ if (activeLaunchTarget != null) {
+ return t.contentEquals(activeLaunchTarget.getId());
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| if (launchBarManager.getActiveLaunchConfiguration() != null) { | |
| return t.contentEquals(launchBarManager.getActiveLaunchTarget().getId()); | |
| } | |
| LaunchTarget activeLaunchTarget = launchBarManager.getActiveLaunchTarget(); | |
| if (activeLaunchTarget != null) { | |
| return t.contentEquals(activeLaunchTarget.getId()); | |
| } |
| if (launchBarManager.getActiveLaunchConfiguration() != null) { | ||
| selectedTarget = launchBarManager.getActiveLaunchTarget() | ||
| .getAttribute(IDFLaunchConstants.ATTR_IDF_TARGET, StringUtil.EMPTY); | ||
| } |
There was a problem hiding this comment.
Similar to the first comment, consider storing the result of getActiveLaunchTarget() in a local variable to avoid calling it twice.
- if (launchBarManager.getActiveLaunchConfiguration() != null) {
- selectedTarget = launchBarManager.getActiveLaunchTarget()
- .getAttribute(IDFLaunchConstants.ATTR_IDF_TARGET, StringUtil.EMPTY);
- }
+ LaunchTarget activeLaunchTarget = launchBarManager.getActiveLaunchTarget();
+ if (activeLaunchTarget != null) {
+ selectedTarget = activeLaunchTarget.getAttribute(IDFLaunchConstants.ATTR_IDF_TARGET, StringUtil.EMPTY);
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| if (launchBarManager.getActiveLaunchConfiguration() != null) { | |
| selectedTarget = launchBarManager.getActiveLaunchTarget() | |
| .getAttribute(IDFLaunchConstants.ATTR_IDF_TARGET, StringUtil.EMPTY); | |
| } | |
| LaunchTarget activeLaunchTarget = launchBarManager.getActiveLaunchTarget(); | |
| if (activeLaunchTarget != null) { | |
| selectedTarget = activeLaunchTarget.getAttribute(IDFLaunchConstants.ATTR_IDF_TARGET, StringUtil.EMPTY); | |
| } |
Hi @AndriiFilippov, pushed a commit, which should fix the test 2 |
|
@sigmaaa hi ! Test 2: create project -> build project -> Close project -> try to create new Launch Config -> get error -> close window -> restart -> try to create new Launch Config -> get error -> restart -> try to create new Launch Config -> get error -> Open Project -> try to create new Launch Config -> click "Cancel" -> Launch Bar settings disappear. |
|
@sigmaaa, tested. LGTM 👍 |
Description
To reproduce: create project -> build project -> Close project -> try to create new Launch Config -> get error -> close window -> restart -> try to create new Launch Config -> get error -> restart -> try to create new Launch Config -> get error -> Open Project -> try to create new Launch Config -> get error:
Fixes # (IEP-1110)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test 1:
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit