feat: improving UX during run/debug #900
Conversation
WalkthroughThe recent update enhances user interaction with debug and launch configurations in the Eclipse environment, focusing on the Espressif IDF UI. It introduces dialogs for selecting configurations, improves messages for better user guidance, and refines execution handling. The shift from asynchronous to synchronous execution in launch target changes ensures more reliable updates, while new classes and properties enhance the user interface and experience. Changes
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: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
bundles/com.espressif.idf.ui/plugin.xmlis excluded by:!**/*.xml
Files selected for processing (8)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java (2 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SelectDebugConfigDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SelectLaunchConfigDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/RunActionHandler.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/messages.properties (1 hunks)
Additional comments: 4
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/Messages.java (1)
- 23-27: LGTM. New string constants for messages are added correctly.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/messages.properties (1)
- 15-19: LGTM. New messages related to missing debug configurations and project selection are added correctly.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java (1)
- 31-36: LGTM. New string constants for debug and launch configuration dialogs are added correctly.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties (1)
- 24-29: LGTM. New dialog labels and texts for selecting debug and launch configurations are added correctly.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SelectDebugConfigDialog.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/SelectLaunchConfigDialog.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/RunActionHandler.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java
Outdated
Show resolved
Hide resolved
|
@sigmaaa hi ! Tested under: OS - Windows 10
🔴 Please, update text with Upper Case for those words and delete extra words.
🔴 found a bug related to Launch Config name validation. Create project A -> create Debug config "A config" -> Edit Debug Config -> Rename to "A" -> there is the warning message only BUT it is still possible to create one. If ignore it - in this particular PR it leads to the bug with empty list: |
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.ui/src/com/espressif/idf/ui/dialogs/messages.properties (1 hunks)
Additional comments: 2
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties (2)
- 24-26: The new dialog messages for selecting a debug configuration are clear and informative. However, ensure that the label text and instructions are consistent with the rest of the application's UI language and terminology.
- 27-29: The messages for the launch configuration dialog are well-defined. It's important to verify that these messages align with the user's expectations and the application's terminology. Additionally, consider localizing these messages for non-English speakers if the application supports internationalization.
Hi @AndriiFilippov, fixed messages in the latest commit. The empty list bug happens due to editing the configuration name to an existing one, which should not be allowed. It's a bug from Eclipse CDT, I guess and it will be easier to fix on the CDT side |
|
@sigmaaa hi ! OK, in this case everything LGTM 👍 |
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.ui/src/com/espressif/idf/ui/LaunchBarListener.java (1 hunks)
Additional comments: 3
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LaunchBarListener.java (3)
- 54-54: The switch from asynchronous to synchronous execution in the
activeLaunchTargetChangedmethod by usingDisplay.getDefault().syncExecis a significant change. This ensures that the UI thread is used to execute the code block, which is necessary for UI-related operations. However, it's crucial to ensure that this change does not introduce UI freezes or performance issues, especially if the operations performed within the synchronous block are time-consuming or involve network I/O, file I/O, or other blocking operations.Please verify that the operations performed within the
syncExecblock are quick and do not significantly impact the UI responsiveness. Consider moving longer operations to background threads if necessary.
- 51-57: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [76-76]
In the
updatemethod, the retrieval of the active launch configuration and project name is handled correctly. However, it's important to ensure that the logic for handling different scenarios, such as JTAG flashing and target mismatches, is thoroughly tested across various project configurations and launch targets.Please verify that the logic for handling JTAG flashing and target mismatches works as expected in all supported scenarios, including edge cases. Consider adding automated tests to cover these scenarios if not already present.
- 51-57: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [157-157]
The
deleteBuildFoldermethod uses anIWorkspaceRunnableto perform the deletion of the build folder and clean the SDK configuration. This approach is appropriate for ensuring that these operations are executed within the workspace's resource change notification mechanism, minimizing the risk of conflicts. However, it's crucial to handle exceptions properly to avoid leaving the workspace in an inconsistent state.Please ensure that all potential exceptions within the
IWorkspaceRunnableare handled appropriately, and consider logging or notifying the user in case of failures. This will help maintain the integrity of the workspace and provide better feedback to the user in case of issues.
* fix: improving UX during run/debug * fix: changing dialog messages --------- Co-authored-by: Kondal Kolipaka <kondal.kolipaka@espressif.com>
Description
This is an alternative to #806, since the approach with filtering run/debug configurations causes bugs and additional maintenance debt
Fixes # (IEP-957)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test 1:
Test 2
Test 3
Test 4
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Enhancements
Documentation