IEP-1073: Tools installation download progress now shows text based progress bar #860
Conversation
WalkthroughThe changes involve updates to the IDF console handling in the UI, including a new parameter for control character handling and progress bar updates. The GitHub Actions workflow has been modified to include a new test step, utilize a reverse RDP action, and update the ESP-IDF version reference. Additionally, a test method related to project copying and building has been removed, indicating a shift in testing or project structure. Changes
Poem
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
I added a new method to specifically set the control chars handle to true to make sure that the other areas using the getConsoleStream() don't break anything.
![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 (3)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/IDFConsole.java (2 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InputStreamConsoleThread.java (2 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/AbstractToolsHandler.java (2 hunks)
Additional comments: 7
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/IDFConsole.java (2)
36-44: The implementation of the new
getConsoleStreammethod with thehandleControlCharsparameter is correct and aligns with the summary provided. It sets thehandleControlCharactersandcarriageReturnAsControlCharacterproperties based on thehandleControlCharsvalue.61-63: The overloaded
getConsoleStreammethod without thehandleControlCharsparameter correctly defaults to calling the updated method withhandleControlCharsset tofalse.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InputStreamConsoleThread.java (2)
46-53: The regex used to match progress update lines seems appropriate for the described functionality. Ensure that all progress update lines strictly follow the format that ends with a percentage sign to avoid any mismatches.
74-100: The implementation of the
updateProgressBarmethod appears to be correct and should provide a visual progress bar in the console. Ensure that the console supports carriage return characters to update the progress bar in place.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/AbstractToolsHandler.java (3)
45-50: The addition of the
idfConsolefield and related console streams is consistent with the pull request's aim to enhance the user interface for the tools installation process.115-120: The changes to
activateIDFConsoleViewproperly initialize theidfConsoleand set up the console streams, aligning with the pull request's description.114-123: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [45-121]
No further issues detected in the provided code. Ensure that the new
idfConsolefield is properly utilized in all relevant parts of the codebase and that the changes do not introduce any unintended side effects.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InputStreamConsoleThread.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InputStreamConsoleThread.java
Show resolved
Hide resolved
There was a problem hiding this comment.
This class was only being used in context of tools installation so its safe to modify this to read the progress but it heavily relies on the idf_tools.py script to have the same output as it does now. Better way would be when we get a machine readable output of that script it will handle things much better
There was a problem hiding this comment.
I didn't understand the need to create two different console objects even when they are returning the same console. So I added a class level property for the IDFConsole and used the updated method in class to explicitly allow the handling of control chars like carriage return \r that I am using in the InputStreamConsoleThread to overwrite the existing progress when updated
alirana01
left a comment
alirana01
left a comment
There was a problem hiding this comment.
Self Review with explanations
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/ci_windows.yml (1 hunks)
Additional comments: 1
.github/workflows/ci_windows.yml (1)
- 50-50: Verify whether the
foregroundoption is intentionally commented out. If it's not needed, consider removing the line to avoid confusion.
.github/workflows/ci_windows.yml
Outdated
| - name: Test background | ||
| uses: NyaMisty/reverse-rdp-windows-github-actions-ng@master | ||
| with: | ||
| ngrok-token: ${{ secrets.NGROK_AUTH_TOKEN }} | ||
| password: "Aa123456" # You can also put the password in secrets as you wish. | ||
| #foreground: false |
There was a problem hiding this comment.
The hardcoded password "Aa123456" in the "Test background" step is a security risk. It's recommended to use repository secrets for sensitive data.
- password: "Aa123456" # You can also put the password in secrets as you wish.
+ password: ${{ secrets.RDP_PASSWORD }} # Use a secret for the passwordCommittable 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.
| - name: Test background | |
| uses: NyaMisty/reverse-rdp-windows-github-actions-ng@master | |
| with: | |
| ngrok-token: ${{ secrets.NGROK_AUTH_TOKEN }} | |
| password: "Aa123456" # You can also put the password in secrets as you wish. | |
| #foreground: false | |
| - name: Test background | |
| uses: NyaMisty/reverse-rdp-windows-github-actions-ng@master | |
| with: | |
| ngrok-token: ${{ secrets.NGROK_AUTH_TOKEN }} | |
| password: ${{ secrets.RDP_PASSWORD }} # Use a secret for the password | |
| #foreground: false |
.github/workflows/ci_windows.yml
Outdated
| - name: Test background | ||
| uses: NyaMisty/reverse-rdp-windows-github-actions-ng@master | ||
| with: | ||
| ngrok-token: ${{ secrets.NGROK_AUTH_TOKEN }} | ||
| password: "Aa123456" # You can also put the password in secrets as you wish. |
There was a problem hiding this comment.
The hardcoded password in the "Test background" step should be moved to GitHub secrets to avoid potential security risks.
- password: "Aa123456" # You can also put the password in secrets as you wish.
+ password: ${{ secrets.RDP_PASSWORD }} # Use GitHub secrets for passwordsCommittable 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.
| - name: Test background | |
| uses: NyaMisty/reverse-rdp-windows-github-actions-ng@master | |
| with: | |
| ngrok-token: ${{ secrets.NGROK_AUTH_TOKEN }} | |
| password: "Aa123456" # You can also put the password in secrets as you wish. | |
| - name: Test background | |
| uses: NyaMisty/reverse-rdp-windows-github-actions-ng@master | |
| with: | |
| ngrok-token: ${{ secrets.NGROK_AUTH_TOKEN }} | |
| password: ${{ secrets.RDP_PASSWORD }} # Use GitHub secrets for passwords |
.github/workflows/ci_windows.yml
Outdated
| uses: NyaMisty/reverse-rdp-windows-github-actions-ng@master | ||
| with: | ||
| ngrok-token: ${{ secrets.NGROK_AUTH_TOKEN }} | ||
| password: "Aa123456" # You can also put the password in secrets as you wish. |
There was a problem hiding this comment.
The comment suggests using secrets for the password, which is a good practice. It would be beneficial to implement this suggestion to enhance security.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/ci_windows.yml (1 hunks)
Additional comments: 2
.github/workflows/ci_windows.yml (2)
15-17: The
runs-onfield now includesself-hostedandali-test. Verify that both runners are properly configured and available for use, and that this change aligns with the intended CI infrastructure setup.12-20: The summary mentions a new "Test background" step utilizing
NyaMisty/reverse-rdp-windows-github-actions-ng@master, but this step is not visible in the provided hunk. Ensure that this step is correctly implemented in the workflow file.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/ci_windows.yml (2 hunks)
Additional comments: 3
.github/workflows/ci_windows.yml (3)
15-17: Verify that the
ali-testrunner is properly configured and available for use, and ensure that the use of multipleruns-onvalues aligns with the intended CI pipeline configuration.26-29: Ensure that the project and its dependencies are compatible with the updated ESP-IDF version
v5.1to prevent any build or runtime issues.12-20: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [12-32]
The summary mentions a new "Test background" step using
NyaMisty/reverse-rdp-windows-github-actions-ng@masteraction, but it is not present in the provided hunks. Verify if this step has been included elsewhere in the workflow file.
|
@alirana01 hi ! Tested: Able to install tools and the output from other commands in console looks good 👍 But during tool installation entire IDE glitches: any action responds with a big delay, like click on "Espressif" menu or expand installation window or any other action. Heap Status: |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- .github/workflows/ci_windows.yml (1 hunks)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java (1 hunks)
Additional comments: 2
.github/workflows/ci_windows.yml (1)
- 24-27: The update to
ref: v5.1for theesp-idfrepository submodule is consistent with the PR objectives and the provided summary. Ensure that all dependencies and workflows are compatible with this new version to prevent any breaking changes from affecting the build process.tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/executable/cases/project/NewEspressifIDFProjectTest.java (1)
- 119-124: The addition of the test method
givenNewIDFProjectIsDeletedWithAllRelatedConfigurationsis a positive change as it expands the test coverage to include project deletion and related configurations. This ensures that the deletion functionality works as expected and does not leave behind any artifacts that could affect subsequent operations.
Hi @AndriiFilippov I have verified this is not reproducible for me could be some local system issue |
Description
Tools installation was showing progress percentage only and that too on new line for every update scrolling the console output to the bottom making it not very user friendly. Now the output in console show the text based progress bar.
Fixes # (IEP-1073)
Type of change
Please delete options that are not relevant.
How has this been tested?
Remove everything in espressif home directory, and then use the install tools option to install the tools. The console should now show a text based progress bar for the tools that are being downloaded.
Also test other flows of the tools installation like download and configure not the installation wizard as that is not using the IDFConsole class.
Also verify the output from other commands in console to see that they are not affected in any way
Test Configuration:
Checklist
Summary by CodeRabbit
New Features
Improvements
Documentation
Continuous Integration
runs-onfield to include new environments for the build job.v4.4tov5.1to incorporate the latest changes and improvements.Tests