-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IEP-1275: Install tools action triggered even after installing tools through offline installer #1031
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self Review
WalkthroughThe recent updates streamline the tool activation process within the system by replacing the previous Changes
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 Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (6 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java
Additional comments not posted (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java (3)
64-64
: Introduction ofIDF_TOOLS_PATH_KEY
is appropriate.The constant
IDF_TOOLS_PATH_KEY
is correctly defined and used to retrieve the tools path from the JSON configuration file.
167-183
: Streamlined tool activation process.The direct instantiation of
IDFToolSet
and scheduling ofToolsActivationJob
simplifies the tool activation process. Ensure that theToolsActivationJob
handles the tool activation as expected.
172-181
: Preference handling and error logging are well-managed.Preferences are updated correctly, and errors during preference flushing are logged appropriately. Ensure that the preference node ID is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Tested on windows
Description
Updated to improve the offline installer behavior with IDE, now using the installed tools in the offline installer instead of creating a new set. Improved handling of IDF_TOOLS_PATH to ensure that
Fixes # (IEP-1275)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please verify this with offline installer's esp_idf.json file to make sure that the tools are not installed but are rather used from the offline installer's directory.
Everything else including the ESP-IDF Manager should stay as is if new bugs are found please report them here and create a new ticket for them.
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Refactor
Chores