IEP-1255: Adding option to remove all versions #996
Conversation
WalkthroughThe recent updates enhance the ESP-IDF tool management functionality by improving UI components and backend logic for environment and toolchain cleanup. Key updates include renaming UI refresh methods, adding a "Remove All" button in the tool manager page, updating necessary string constants, and handling user confirmations for mass operations. The changes aim to streamline user interactions and ensure proper resource management in the IDE. Changes
Poem
Warning Review ran into problemsProblems (1)
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 (6)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolSetConfigurationManager.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJobListener.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (9 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/install/IDFNewToolsWizard.java
Additional comments not posted (9)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJobListener.java (1)
52-52: Refactored method call fromrefreshTable()torefreshEditorUI()aligns with UI consistency improvements.#!/bin/bash # Description: Verify that all instances of `refreshTable()` have been replaced with `refreshEditorUI()`. # Test: Search for the old function usage. Expect: No occurrences of the old function. rg --type java $'refreshTable'bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.java (1)
119-124: Added string constants for 'Remove All' functionality are appropriately named and support the new feature.#!/bin/bash # Description: Verify that new string constants are used consistently in the UI. # Test: Search for the new constants usage. Expect: Occurrences in UI related files. rg --type java $'EspIdfManagerRemoveAllBtn|EspIdfManagerMessageBoxDeleteAllConfirmMessage|EspIdfManagerMessageBoxDeleteAllConfirmMessageTitle'bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties (1)
121-126: Updated and added properties inmessages.propertiescorrectly support the new 'Remove All' functionality.#!/bin/bash # Description: Verify that the updated properties are used consistently and correctly throughout the application. # Test: Search for the updated properties usage. Expect: Occurrences in UI related files. rg --type properties $'EspIdfManagerRemoveAllBtn|EspIdfManagerMessageBoxDeleteConfirmMessage|EspIdfManagerMessageBoxDeleteConfirmMessageTitle|EspIdfManagerMessageBoxDeleteAllConfirmMessage|EspIdfManagerMessageBoxDeleteAllConfirmMessageTitle'bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (6)
39-39: Added import forLaunchBarListenerseems appropriate for the new UI functionalities introduced.
63-64: Introduction of new class variablesidfToolSetsandremoveAllButtonto manage the state and control of the toolsets and their UI representation.
73-73: Initialization ofidfToolSetsin thecreatePagemethod aligns with the new functionality to manage toolsets dynamically.
Line range hint
82-114: RefactoredrefreshEditorUImethod to handle dynamic UI updates more efficiently, especially with the disposal of UI elements to prevent memory leaks.
120-127: Logic to enable/disable theremoveAllButtonbased on the presence of toolsets is clear and correctly implemented.
187-208: Ensure that theremoveAllButtonis appropriately tested, especially the scenario where the deletion of one toolset fails but others might still need to be processed.#!/bin/bash # Description: Verify `removeAllButton` functionality with partial failure scenarios. # Test: Simulate partial failure in deletion and check for correct handling. # Expect: Remaining toolsets should still be attempted for deletion. echo "Simulate and verify partial failure scenario."
| removeAllButton = new Button(buttonComposite, SWT.PUSH); | ||
| removeAllButton.setText(Messages.EspIdfManagerRemoveAllBtn); | ||
| removeAllButton.addSelectionListener(new SelectionAdapter() | ||
| { | ||
| @Override | ||
| public void widgetSelected(SelectionEvent e) | ||
| { | ||
| MessageBox messageBox = new MessageBox(Display.getDefault().getActiveShell(), | ||
| SWT.ICON_WARNING | SWT.YES | SWT.NO); | ||
| messageBox.setMessage(Messages.EspIdfManagerMessageBoxDeleteAllConfirmMessage); | ||
| messageBox.setText(Messages.EspIdfManagerMessageBoxDeleteAllConfirmMessageTitle); | ||
| int response = messageBox.open(); | ||
| if (response == SWT.YES) | ||
| { | ||
| for(IDFToolSet idfToolSet : idfToolSets) | ||
| { | ||
| toolSetConfigurationManager.delete(idfToolSet); | ||
| } | ||
| refreshEditorUI(); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Added functionality for the removeAllButton to handle the removal of all toolsets. The use of a confirmation dialog box is a good user experience practice. However, consider handling potential exceptions during the deletion process to improve robustness.
+ try {
+ for(IDFToolSet idfToolSet : idfToolSets) {
+ toolSetConfigurationManager.delete(idfToolSet);
+ }
+ refreshEditorUI();
+ } catch (Exception e) {
+ // Log error or inform user
+ }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.
| removeAllButton = new Button(buttonComposite, SWT.PUSH); | |
| removeAllButton.setText(Messages.EspIdfManagerRemoveAllBtn); | |
| removeAllButton.addSelectionListener(new SelectionAdapter() | |
| { | |
| @Override | |
| public void widgetSelected(SelectionEvent e) | |
| { | |
| MessageBox messageBox = new MessageBox(Display.getDefault().getActiveShell(), | |
| SWT.ICON_WARNING | SWT.YES | SWT.NO); | |
| messageBox.setMessage(Messages.EspIdfManagerMessageBoxDeleteAllConfirmMessage); | |
| messageBox.setText(Messages.EspIdfManagerMessageBoxDeleteAllConfirmMessageTitle); | |
| int response = messageBox.open(); | |
| if (response == SWT.YES) | |
| { | |
| for(IDFToolSet idfToolSet : idfToolSets) | |
| { | |
| toolSetConfigurationManager.delete(idfToolSet); | |
| } | |
| refreshEditorUI(); | |
| } | |
| } | |
| }); | |
| removeAllButton = new Button(buttonComposite, SWT.PUSH); | |
| removeAllButton.setText(Messages.EspIdfManagerRemoveAllBtn); | |
| removeAllButton.addSelectionListener(new SelectionAdapter() | |
| { | |
| @Override | |
| public void widgetSelected(SelectionEvent e) | |
| { | |
| MessageBox messageBox = new MessageBox(Display.getDefault().getActiveShell(), | |
| SWT.ICON_WARNING | SWT.YES | SWT.NO); | |
| messageBox.setMessage(Messages.EspIdfManagerMessageBoxDeleteAllConfirmMessage); | |
| messageBox.setText(Messages.EspIdfManagerMessageBoxDeleteAllConfirmMessageTitle); | |
| int response = messageBox.open(); | |
| if (response == SWT.YES) | |
| { | |
| try { | |
| for(IDFToolSet idfToolSet : idfToolSets) { | |
| toolSetConfigurationManager.delete(idfToolSet); | |
| } | |
| refreshEditorUI(); | |
| } catch (Exception e) { | |
| // Log error or inform user | |
| } | |
| } | |
| } | |
| }); |
| // cleanup the env and toolchains | ||
| IDFEnvironmentVariables idfEnvironmentVariables = new IDFEnvironmentVariables(); | ||
| idfEnvironmentVariables.removeAllEnvVariables(); | ||
| try | ||
| { | ||
| ESPToolChainManager espToolChainManager = new ESPToolChainManager(); | ||
| espToolChainManager.removeCmakeToolChains(); | ||
| espToolChainManager.removeStdToolChains(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Logger.log(e); | ||
| } |
There was a problem hiding this comment.
Enhanced cleanup logic in the delete method ensures that environment variables and toolchains are properly removed. Consider adding more detailed logging to improve traceability.
// Suggested logging enhancement
Logger.log("Starting cleanup for toolset: " + idfToolSet.getId());
try
{
ESPToolChainManager espToolChainManager = new ESPToolChainManager();
espToolChainManager.removeCmakeToolChains();
espToolChainManager.removeStdToolChains();
}
catch (Exception e)
{
Logger.log("Failed to clean up toolchains for: " + idfToolSet.getId(), e);
}
Logger.log("Completed cleanup for toolset: " + idfToolSet.getId());
alirana01
changed the title
|
@alirana01 hi ! Tested under: Install tools -> create project -> build -> delete all tools using "Remove All" button -> restart IDE -> clean "Project Full Clean" -> try to build project -> it is still building project with empty Tool Manager.
|
Can you try restarting IDE the toolchains at times dont disappear until restart |
Please, read my comment above. |
Description
Added an option to remove all the ESP-IDF version in the manager view also now you can delete the active version.
Fixes # (IEP-1255)
Type of change
Please delete options that are not relevant.
How has this been tested?
Install multiple versions and try using Remove All button which should remove every version from the IDE. Also test removing the active version. Verify that the remove all button stays disabled when there are no versions in the IDE.
Test Configuration:
Checklist
Summary by CodeRabbit
New Features
Enhancements
refreshTable()torefreshEditorUI()for better clarity and functionality.Bug Fixes