-
Notifications
You must be signed in to change notification settings - Fork 128
IEP-1516: Automatic update to IDE with filewatcher #1210
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
base: release/v4.0.0
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
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.
Self Review
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.
from the code perspective LGTM
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. it would be great if we can incorpate the feedback.
@@ -7,6 +7,6 @@ EspIdfManagerReloadBtnToolTip=Reload from disk | |||
IDFToolsHandler_ToolsManagerConsole=Espressif IDF Tools Console | |||
IDFGuideLinkLabel_Text=Select the version of ESP-IDF you want to use. Click the radio button in Active column next to the version you want. For help in choosing the correct version, visit <a>ESP-IDF Version Guide</a>. | |||
EimJsonChangedMsgTitle=ESP-IDF Installation Changed | |||
EimJsonChangedMsgDetail=It looks like the ESP-IDF tools are modified. The Espressif IDE cannot guarantee the consistency. Kindly refresh the installation via ESP-IDF Manager. | |||
EimJsonChangedMsgDetail=It looks like the ESP-IDF tools are modified. The Espressif IDE cannot guarantee the consistency. Do you want to refresh installation? |
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.
Can we maintain a positive tone in this message(especially this -The Espressif IDE cannot guarantee the consistency)?
It seems the ESP-IDF tools have been modified. To maintain consistency, we recommend refreshing your installation. Would you like to proceed with the update?
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.
Done
display.syncExec(() -> { | ||
Shell shell = display.getActiveShell(); | ||
if (shell == null) | ||
{ | ||
shell = new Shell(display); | ||
} |
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.
Display.getDefault().getActiveShell() should work here considering that workbench has been already initilized, do you see any issue here?
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.
You can also use EclipseUtil.getShell()
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.
@kolipakakondal yes since the handler can be called from early startup class it causes NPE because the shell is not available at times
MessageBox messageBox = new MessageBox(shell, SWT.ICON_WARNING | SWT.YES | SWT.NO); | ||
messageBox.setText(Messages.EimJsonChangedMsgTitle); | ||
messageBox.setMessage(Messages.EimJsonChangedMsgDetail); | ||
response[0] = messageBox.open(); |
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.
Can we use MessageDialog over MessageBox? MessageDialogs are better, more modern, and JFace-based.
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.
Done
Display.getDefault().asyncExec(() -> { | ||
IWorkbenchWindow activeww = EclipseHandler.getActiveWorkbenchWindow(); | ||
if (activeww != null && activeww.getActivePage() != null) | ||
{ | ||
try | ||
{ | ||
IDE.openEditor(activeww.getActivePage(), new EimEditorInput(eimJson), | ||
ESPIDFManagerEditor.EDITOR_ID, true); | ||
} | ||
catch (PartInitException e) | ||
{ | ||
Logger.log("Failed to open ESP-IDF Manager Editor."); | ||
Logger.log(e); | ||
} | ||
} | ||
else | ||
{ | ||
Logger.log("Cannot open ESP-IDF Manager Editor. No active workbench window yet."); | ||
} | ||
}); |
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.
Avoid nested blocks
If certain criteria doesn't match, verify and return immedialy
if (activeww == null || activeww.getActivePage() == null) {
Logger.log("Cannot open ESP-IDF Manager Editor. No active workbench window or page.");
return;
}
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.
Done
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 after the code review comments resolution
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
Hi @AndriiFilippov Please check |
@alirana01 hi! Tested under: The filewatcher been added only if tools\eim_idf.json exists. So, if user haven't used our eim installer before he runs IDE, the filewatcher won't be added without restart. Reproduce: delete your tools\eim_idf.json file -> run IDE -> run eim.installer and install ESP-IDF -> the installed version will appears in IDE Installation Manager -> but no filewatcher added. To add filewatcher - restart IDE. |
@alirana01 hi! Tested under: During testing, I made a mistake by manually editing the eim_idf.json file (extra comma) and received an error in the Installation Manager. Perhaps we should consider providing a more informative error message in this case.
|
OS - Windows 11 ESP-IDF Manager shell (window) does not update after changes to eim_idf.json file. |
Description
We can update the IDE environment if there is a change to the eim json file. We will update the IDE only when there is one entry in json file. For multiple we will open the ESP-IDF Manager
Fixes # (IEP-1516)
Type of change
Please delete options that are not relevant.
How has this been tested?
Try to make changes to eim_idf.json file the IDE should be able to capture that and show a proper message to user to update their environment for only one entry it will update automatically for multiple ones we will launch ESP-IDF Manager.
Test Configuration:
Checklist