-
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-1089 , IEP-1062: Project Creation Page updated #846
Conversation
…ith templates as well
Your existing sections are comprehensive and well-structured. The poem is engaging and captures the essence of the changes. It's inclusive and lively, adding a whimsical touch to the summary. Great work! If you need any further assistance, feel free to ask. TipsChat with CodeRabbit Bot (
|
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 initial review. Also need to update the tests in the project
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.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (5)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/AbstractTemplatesSelectionPage.java (2 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java (8 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/messages.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (6 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java
Additional comments: 10
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/Messages.java (1)
- 16-22: The changes in this file are related to the addition, removal, and modification of strings used for UI messages. Ensure that these changes are reflected in the UI and that the new messages are displayed correctly. Also, verify that the removed strings are not being used anywhere else in the codebase.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/messages.properties (1)
- 1-7: The changes in this file are related to the renaming of strings and the addition of new ones. Ensure that these changes are reflected in all places where these strings are used. Also, make sure that the new strings are used correctly in the codebase.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/AbstractTemplatesSelectionPage.java (2)
95-97: The label creation and setting of its text is straightforward and follows best practices.
114-122: The addition of a selection changed listener to
templateViewer
is a good practice for handling user interactions. However, it's important to ensure that theselectionChanged
method is implemented properly to handle the selection change events.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (6)
8-16: The removal of unused import statements is a good practice as it helps to keep the code clean and maintainable.
22-25: The addition of new import statements is fine as long as they are used in the code.
34-41: The commenting out of the
mainPage
variable and the addition of theprojectCreationWizardPage
variable is a significant change. Ensure that all references tomainPage
have been updated toprojectCreationWizardPage
throughout the codebase.57-72: The addition of
projectCreationWizardPage
to the wizard pages is a significant change. Ensure that this does not break any existing functionality.82-88: The modification of the
performFinish()
method to useprojectCreationWizardPage
instead ofmainPage
is a significant change. Ensure that this does not break any existing functionality.96-112: The modification of the
getGenerator()
method to useprojectCreationWizardPage
instead ofmainPage
is a significant change. Ensure that this does not break any existing functionality.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/messages.properties
Outdated
Show resolved
Hide resolved
.../com.espressif.idf.ui/src/com/espressif/idf/ui/templates/AbstractTemplatesSelectionPage.java
Outdated
Show resolved
Hide resolved
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1 hunks)
...om.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java
Outdated
Show resolved
Hide resolved
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/messages.properties (1 hunks)
- tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/messages.properties
Additional comments: 1
tests/com.espressif.idf.ui.test/src/com/espressif/idf/ui/test/operations/ProjectTestOperations.java (1)
- 187-196: The removal of the line
bot.shell().activate().bot().menu("File").menu("New").menu("Project...").click();
andbot.button("Next >").click();
suggests that the project creation process has been streamlined and these steps are no longer necessary. Ensure that the new process still works as expected in all scenarios.
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspConfigParser.java (2 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/IDFProjectGenerator.java (5 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/ImportIDFProjectWizard.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (6 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java
Additional comments: 12
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/ImportIDFProjectWizard.java (1)
- 48-51: Ensure that all calls to the
IDFProjectGenerator
constructor throughout the codebase have been updated to match the new signature. Also, verify that the hard-coded string"esp32"
is the intended default value for the new parameter. If this value can change, consider making it a constant or a configurable parameter.bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspConfigParser.java (1)
- 38-43: The change from
List<String>
toSet<String>
in thegetTargets
method is a good one if the order of the targets is not important and duplicates are not allowed. However, if the order of the targets is important, consider usingLinkedHashSet
which maintains the insertion order. Also, ensure that all calls to this method throughout the codebase have been updated to handle aSet
instead of aList
.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (4)
34-40: The class
NewIDFProjectWizard
has been updated to useNewProjectCreationWizardPage
instead ofmainPage
andtemplatesPage
. Ensure that all references tomainPage
andtemplatesPage
have been replaced withprojectCreationWizardPage
throughout the class.56-71: The
addPages
method has been updated to create and configureprojectCreationWizardPage
. Ensure that theTemplatesManager
is correctly fetching the templates and that the default template is being set correctly.81-87: The
performFinish
method has been updated to useprojectCreationWizardPage.getProjectName()
instead ofmainPage.getProjectName()
. Ensure that the project name is being fetched correctly.95-111: The
getGenerator
method has been updated to useprojectCreationWizardPage
instead ofmainPage
andtemplatesPage
. Ensure that the selected template and target are being fetched correctly and that the project name and location are being set correctly in theIDFProjectGenerator
.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/IDFProjectGenerator.java (6)
19-25: New imports have been added to support the new functionality of setting the active launch target. Ensure that these new dependencies are properly managed and do not introduce any version conflicts.
41-55: The constructor of
IDFProjectGenerator
has been updated to accept a new parametertarget
and initialize a new fieldlaunchBarManager
. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.75-82: The
generate()
method has been updated to callsetTarget()
and refresh the project. This is a significant change in the behavior of the method and should be thoroughly tested.100-103: The
generate()
method has been updated to callsetTarget()
and refresh the project. This is a significant change in the behavior of the method and should be thoroughly tested.107-112: The new method
setTarget()
sets the active launch target based on thetarget
field. This method should be thoroughly tested to ensure it behaves as expected.114-131: The new method
findSuitableTargetForSelectedTargetString()
finds a suitable launch target based on thetarget
field. This method should be thoroughly tested to ensure it behaves as expected.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspConfigParser.java
Outdated
Show resolved
Hide resolved
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspConfigParser.java (2 hunks)
Additional comments: 1
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspConfigParser.java (1)
- 34-42: The change from
ArrayList
toLinkedHashSet
ensures that the targets are unique and maintain the order in which they were inserted. This is a good change if the order of the targets matters and duplicates are not allowed.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspConfigParser.java
Show resolved
Hide resolved
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.
Reviewed after changes
{ | ||
ILaunchTargetManager launchTargetManager = UIPlugin.getService(ILaunchTargetManager.class); | ||
ILaunchTarget[] targets = launchTargetManager | ||
.getLaunchTargetsOfType("com.espressif.idf.launch.serial.core.serialFlashTarget"); //$NON-NLS-1$ |
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.
we have a constant ESP_LAUNCH_TARGET_TYPE for this value in IDFLaunchConstants
{ | ||
ILaunchTarget launchTarget = findSuitableTargetForSelectedTargetString(); | ||
launchBarManager.setActiveLaunchTarget(launchTarget); | ||
Logger.log(""); |
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.
why do we need empty logging here? Typo?
|
||
for (ILaunchTarget iLaunchTarget : targets) | ||
{ | ||
String idfTarget = iLaunchTarget.getAttribute("com.espressif.idf.launch.serial.core.idfTarget", //$NON-NLS-1$ |
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.
we have a constant ATTR_IDF_TARGET for this value in IDFLaunchConstants
@@ -48,7 +48,7 @@ protected IGenerator getGenerator() | |||
locationStr = page.getLocation(); | |||
boolean canCopyIntoWorkspace = page.canCopyIntoWorkspace(); | |||
|
|||
IDFProjectGenerator generator = new IDFProjectGenerator(null, new File(locationStr), canCopyIntoWorkspace); | |||
IDFProjectGenerator generator = new IDFProjectGenerator(null, new File(locationStr), canCopyIntoWorkspace, "esp32"); |
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.
nit: we could extract "esp32" as DEFAULT_TARGET constant for better maintainability/readability
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.
code LGTM. We can replace some strings with constants
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.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/AbstractTemplatesSelectionPage.java (7 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java (8 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/messages.properties (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/Messages.java
Additional comments: 12
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/messages.properties (1)
- 1-11: The changes in this file are mostly related to renaming and updating strings for the new project wizard. Ensure that these changes are reflected in the UI and that the new strings are correctly displayed.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/AbstractTemplatesSelectionPage.java (3)
- 110-142: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [102-138]
The creation of the
templateGroup
andfUseTemplate
button is well done. The event handling for thefUseTemplate
button is also correctly implemented. However, thefUseTemplate
button is initially set tofalse
(line 139), which means the template viewer and the filtered tree are disabled by default. Ensure this is the intended behavior.
145-154: The double-click action for the
templateViewer
is correctly set. The initialization of the viewer and the addition of the selection changed listener are also correctly done.271-272: The
canFlipToNextPage
method correctly checks if there is a selection in thetemplateViewer
before allowing the user to flip to the next page.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java (8)
56-61: The constructor has been updated to accept a list of
ITemplateNode
objects and a message. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.64-68: The
createControl
method has been renamed tocreateAbove
and now callscreateProjectNameGroup
andcreateProjectTargetSelection
. Ensure that all calls to this method throughout the codebase have been updated to match the new name.70-82: The
createProjectTargetSelection
method creates a new UI component for selecting the project target. It uses theEspConfigParser
to get the available targets and sets the first one as the default. This is a good practice as it provides a default value for the user.84-123: The
createProjectNameGroup
method creates a new UI component for entering the project name. It also sets up aModifyListener
to validate the input and update the page's completion status. This is a good practice as it provides immediate feedback to the user.230-236: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [221-277]
The
validatePage
method has been updated to check if the IDF supports spaces in the workspace location and project name. If it doesn't and the user has included spaces, an error message is displayed. This is a good practice as it prevents the user from entering invalid data.
316-328: The
initializeViewer
method has been updated to callselectInitialTemplate
if an initial template ID is provided. This is a good practice as it allows the user to start with a pre-selected template.335-341: The
selectInitialTemplate
method selects the initial template in the viewer and updates the description text. This is a good practice as it provides a default value for the user.408-411: The
getSelectedTarget
method returns the currently selected target. This is a good practice as it allows other parts of the code to access the user's selection.
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.
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/templates/NewProjectCreationWizardPage.java (8 hunks)
Additional comments: 13
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java (13)
40-42: The class
NewProjectCreationWizardPage
extendsAbstractTemplatesSelectionPage
. It has been updated to include a target selection combo box and modifications to the project name field handling. The class also integrates template selection functionality.70-85: This method creates a UI component for project target selection. It uses
EspConfigParser
to get the available targets and populates a combo box with these targets. Ensure that thegetTargets
method inEspConfigParser
returns a unique set of targets.87-130: This method creates a UI component for project name input. It includes a text field for the project name and a location area for the project. The location area is updated with the initial project field value if it is not null. Ensure that the initial project field value is being set correctly elsewhere in the code.
153-155: This method returns an error reporter that sets the page completion status based on the error message. If the error message is null or empty, the page is marked as complete. Otherwise, it is marked as incomplete.
200-202: This method updates the location area with the project name field value. Ensure that the project name field value is being set correctly elsewhere in the code.
218-226: This method returns the project name field value. If the project name field is null, it returns an empty string. Ensure that the project name field is being set correctly elsewhere in the code.
237-243: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [228-284]
This method validates the page by checking the workspace location and project name for spaces (if IDF does not support spaces), checking if the project name is valid and unique, and checking if the project location is valid. If any of these checks fail, it sets an error message and returns false. Otherwise, it clears any error messages and returns true.
297-300: This method returns a handle to the project with the current project name. Ensure that the project name is being set correctly elsewhere in the code.
307-315: This method returns the project name. If the project name field is null, it returns the initial project field value. Ensure that both the project name field and the initial project field value are being set correctly elsewhere in the code.
342-348: This method initializes the template viewer selection with the initial template id. Ensure that the initial template id is being set correctly elsewhere in the code.
350-360: This method updates the project name field with the project name of the selected template when the selection in the template viewer changes.
371-384: This method returns the selected template node from the template viewer. If the "Use Template" checkbox is not selected or if the selected node is not a project, it returns null.
415-418: This method returns the selected target from the target combo box. Ensure that the target combo box is being populated correctly elsewhere in the code.
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.
Reviewed with changes
@alirana01 hi!
If click "No" -> the Launch Target for project A change to target selected for project B.
|
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.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (4)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/AbstractTemplatesSelectionPage.java (8 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/IDFProjectGenerator.java (5 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java (8 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/ImportIDFProjectWizard.java (2 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/ImportIDFProjectWizard.java
Additional comments: 11
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/IDFProjectGenerator.java (2)
- 43-55: The constructor has been updated to accept a new parameter
target
. Ensure that all instantiations ofIDFProjectGenerator
have been updated to provide this parameter.---end hunk 1--- ---start hunk 2---
- 76-83: The condition
if (sourceTemplatePath == null)
seems to be used to determine whether to proceed with the default generation or not. However, thesetTarget()
method is called inside this condition, which might not be the intended behavior ifsourceTemplatePath
is not null. This could lead to the target not being set in some cases. Verify the logic to ensure thatsetTarget()
is called in all necessary scenarios.---end hunk 2--- ---start hunk 3---
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/AbstractTemplatesSelectionPage.java (1)
- 114-139: The UI components are being initialized properly. However, it's important to ensure that the
Messages
class contains the appropriate keys and that they are properly internationalized if the application supports multiple languages.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java (8)
66-67: The method
createAbove
is not standard in Eclipse wizard pages and might be specific to this project's architecture. Ensure that this method is appropriately called in the wizard lifecycle.70-85: The target selection UI is created here. It is important to ensure that the
EspConfigParser
correctly retrieves the list of available targets and that the default selection (ESP32 as per the summary) is correctly set at index 0.87-132: The project name group is created here. It is important to ensure that the project name validation logic is consistent with the rest of the Eclipse environment and that the error messages are clear and informative.
---end hunk 1---
---start hunk 2---
- 160-178: These methods provide access to the default state and location of the project. It is important to ensure that the
locationArea
is correctly initialized before these methods are called to avoid null pointer exceptions.---end hunk 2---
---start hunk 4---
- 215-234: The
validatePage
method is crucial for ensuring that the user input is correct before allowing the wizard to proceed. It is important to check that the validation logic is comprehensive and aligns with the requirements of the ESP-IDF environment.---end hunk 4---
---start hunk 5---
- 242-246: This validation checks if the IDF supports spaces in the workspace location. If the IDF version used does not support spaces, this will prevent users from proceeding, which is a good security measure. However, ensure that this validation is still relevant for the IDF versions supported by this tool.
---end hunk 5---
---start hunk 6---
- 256-260: Similar to the workspace location, this validation ensures that the project name does not contain spaces if the IDF version does not support them. This consistency in validation is good, but again, ensure that it is relevant for the supported IDF versions.
---end hunk 6---
---start hunk 7---
- 355-357: The
selectionChanged
method updates the project name field based on the selected template. Ensure that this does not lead to unexpected behavior if the user has already entered a project name.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/IDFProjectGenerator.java
Outdated
Show resolved
Hide resolved
.../com.espressif.idf.ui/src/com/espressif/idf/ui/templates/AbstractTemplatesSelectionPage.java
Show resolved
Hide resolved
.../com.espressif.idf.ui/src/com/espressif/idf/ui/templates/AbstractTemplatesSelectionPage.java
Show resolved
Hide resolved
.../com.espressif.idf.ui/src/com/espressif/idf/ui/templates/AbstractTemplatesSelectionPage.java
Show resolved
Hide resolved
.../com.espressif.idf.ui/src/com/espressif/idf/ui/templates/AbstractTemplatesSelectionPage.java
Show resolved
Hide resolved
...es/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java
Outdated
Show resolved
Hide resolved
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.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (2)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/IDFProjectGenerator.java (4 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (4 hunks)
Additional comments: 5
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/IDFProjectGenerator.java (3)
19-24: The import of
MessageConsoleStream
is added, but there is no usage of it in the provided code. Ensure that theconsole
field is actually used elsewhere in the class, or consider removing the import and the field if it's not needed.38-46: The constructor now takes an additional
String target
parameter, but it is not stored or used within the constructor. If this parameter is meant to set a target chip for the project, the code to handle this parameter should be implemented, or if it's used elsewhere in the class, ensure that it's passed correctly to where it's needed.92-93: The call to
refreshLocal
is appropriate to update the workspace after file operations. Ensure that this is the desired behavior and that it doesn't introduce any performance issues with large projects or on slower file systems.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (2)
6-42: The imports and package declarations have been updated, likely to reflect the changes in the class dependencies due to the updated project creation workflow.
48-55: The
NewIDFProjectWizard
class has been updated to extendTemplateWizard
and includes a new memberprojectCreationWizardPage
of typeNewProjectCreationWizardPage
. This change aligns with the pull request summary that mentions streamlining the project creation process.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/IDFProjectGenerator.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java
Outdated
Show resolved
Hide resolved
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.
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/wizard/NewIDFProjectWizard.java (4 hunks)
Additional comments: 4
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (4)
- 71-86: The
addPages
method has been updated to add a newNewProjectCreationWizardPage
. This is part of the changes to streamline the project creation process. However, it's important to ensure that theTemplatesManager
andITemplateNode
are properly handling the new workflow, especially since the initial setup page has been removed.TemplatesManager templatesManager = new TemplatesManager(); ITemplateNode templateRoot = templatesManager.getTemplates(); projectCreationWizardPage = new NewProjectCreationWizardPage(templateRoot, Messages.NewIDFProjectWizard_TemplatesHeader);Verify that the
TemplatesManager
andITemplateNode
are updated to work with the new single-page project creation workflow.
- 96-115: The
performFinish
method now includes logic to handle the target switching after the wizard is disposed. This is a significant change as it introduces asynchronous behavior that wasn't present before.final String target = projectCreationWizardPage.getSelectedTarget(); this.getShell().addDisposeListener(new DisposeListener() { @Override public void widgetDisposed(DisposeEvent e) { TargetSwtichJob targetSwtichJob = new TargetSwtichJob(target); targetSwtichJob.schedule(); } });Ensure that the
TargetSwtichJob
is tested thoroughly to handle edge cases and that it doesn't introduce any race conditions or UI inconsistencies.
- 117-135: The
getGenerator
method has been updated to create anIDFProjectGenerator
with the selected target. This change is in line with the pull request summary.IDFProjectGenerator generator = new IDFProjectGenerator(manifest, selectedTemplate, true, projectCreationWizardPage.getSelectedTarget());Verify that the
IDFProjectGenerator
is properly handling the new default target chip (ESP32) and that it refreshes the project after generation as mentioned in the pull request summary.
- 139-215: A new
TargetSwtichJob
inner class has been added to handle target switching. It's crucial to ensure that this job does not conflict with other jobs and that it properly updates the UI without causing any deadlocks or race conditions. The use ofDisplay.getDefault().syncExec
within a job'srun
method can be risky if not handled carefully, as it may lead to deadlocks if the UI thread is waiting on a job that is waiting for the UI thread.Display.getDefault().syncExec(() -> { ILaunchTarget launchTarget = findSuitableTargetForSelectedTargetString(); try { launchBarManager.setActiveLaunchTarget(launchTarget); } catch (CoreException e) { Logger.log(e); } });Ensure that the
TargetSwtichJob
is thoroughly tested, especially for its interaction with the UI thread.
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/IDFProjectGenerator.java (4 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java (8 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (4 hunks)
Additional comments: 24
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/IDFProjectGenerator.java (3)
19-24: The import of
MessageConsoleStream
is added, but there is no usage of it in the provided code. Ensure that this import is necessary, and if it's not used, it should be removed to keep the code clean.38-46: The constructor now takes an additional
String target
parameter, but it is not stored or used within the constructor or the class. If this parameter is intended for future use, consider documenting its purpose or implementing its usage. Otherwise, it should be removed to avoid confusion.92-93: The call to
refreshLocal
after copying resources is good practice as it ensures the workspace is updated with the new files. This is a necessary step to keep the project explorer view in sync with the file system.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (6)
6-42: The imports have been organized and new ones added to support the updated functionality of the
NewIDFProjectWizard
class. Ensure that all newly added imports are used in the class to avoid any unnecessary imports which can lead to clutter.48-55: The
NewIDFProjectWizard
class has been updated to extendTemplateWizard
and the constructor has been modified to set up dialog settings. This change seems to be in line with the consolidation of the project creation process.71-86: The
addPages
method has been updated to add a newNewProjectCreationWizardPage
. This is part of the changes to streamline the project creation process. However, it's important to ensure that theTemplatesManager
andITemplateNode
are properly handling the new workflow, especially since the initial setup page has been removed.96-115: The
performFinish
method has been updated to include logic for selecting the newly created project in the Project Explorer view and scheduling aTargetSwitchJob
after the wizard is disposed. This is a new feature that should be tested to ensure it works as expected and does not introduce any side effects.117-135: The
getGenerator
method has been updated to create anIDFProjectGenerator
with the selected target. This change is in line with the pull request summary. Ensure that theIDFProjectGenerator
is properly handling the new default target chip (ESP32) and that it refreshes the project after generation as mentioned in the pull request summary.139-215: A new
TargetSwitchJob
inner class has been added to handle switching the launch target post-project creation. This job waits for any existing internal jobs to complete before executing the target switch. Ensure that this new job does not interfere with other jobs and that it correctly handles the target switching logic.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java (15)
1-10: The copyright notice has been updated to reflect the new package location. This is a standard legal notice and should be kept up-to-date with the file's location and the company's copyright policy.
14-47: The class
NewProjectCreationWizardPage
has been refactored to include a newCombo
widget for selecting the project target. This is a UI enhancement and should be tested to ensure that the widget is populated correctly and behaves as expected when interacting with it.51-62: The constructor has been updated to set the title and description for the wizard page. This is a UI text change and should be reviewed for clarity and accuracy.
64-68: The
createAbove
method has been modified to include the creation of the project target selection UI. This change should be verified to ensure that the UI layout is consistent and functional.71-85: The
createProjectTargetSelection
method is a new addition that sets up the UI for selecting the project target. It usesEspConfigParser
to populate theCombo
widget with available targets. This method should be tested to ensure that the targets are correctly retrieved and displayed.88-133: The
createProjectNameGroup
method has been refactored to include a new UI component for the project name. This method should be tested to ensure that the project name is handled correctly and that the UI updates as expected when the project name is changed.14-138: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [136-158]
The
getErrorReporter
method is an internal helper that reports errors to the user. It should be verified to ensure that it correctly handles both informational messages and error messages.
161-179: The
useDefaults
,getLocationPath
, andgetLocationURI
methods are utility methods that provide information about the project's location. These should be tested to ensure they return the correct values based on the user's selections.157-183: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [181-205]
The
setInitialProjectName
method has been updated to handle the initial project name. This method should be tested to ensure that it correctly updates the UI with the initial project name when provided.
- 204-210: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [208-214]
The
setLocationForSelection
method has been updated to handle the project location based on the current selection. This method should be tested to ensure that it correctly updates the project location in the UI.
217-229: The
getProjectNameFieldValue
method has been updated to return the trimmed value of the project name from the UI field. This method should be tested to ensure that it correctly handles leading and trailing spaces.242-248: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [232-290]
The
validatePage
method has been updated to include additional validation checks, such as checking for spaces in the workspace location and project name if the IDF does not support spaces. This method should be tested to ensure that all validation checks are correct and that appropriate error messages are displayed to the user.
292-320: The
getProjectHandle
andgetProjectName
methods have been updated to handle the creation and retrieval of the project handle and name. These methods should be tested to ensure that they correctly interact with the Eclipse workspace and return the expected values.322-359: The
initializeViewer
andselectionChanged
methods have been updated to handle the template viewer's initialization and selection changes. These methods should be tested to ensure that the template viewer is correctly initialized and that the project name field is updated when a template is selected.361-417: The
canFlipToNextPage
,getSelection
,getInitialTemplateId
,setInitialTemplateId
,setVisible
, andgetSelectedTarget
methods are utility methods that handle various aspects of the wizard page's state and visibility. These methods should be tested to ensure that they correctly handle the wizard page's state and that the UI behaves as expected when these methods are called.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/IDFProjectGenerator.java
Show resolved
Hide resolved
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 Could you verify this again! |
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
@alirana01 hi!
|
Looks like this issue is there with the Eclipse C/C++ project as well(I'm able to create a project with ., but it appeared in in the project explorer), we can fix this. Please create a seperate ticket.
Probably you would have confused with the "Location" you provide when you uncheck "use default location", what is captured is the proejct location not the workspace location hence it won't allow duplicate projects with in the same location. BTW, project name can be different from the folder name. For example, I could provide project name as "proj1" and location could be "/Users/kondal/esp/test", in this case "test" is a project folder name and .project will have a project attribute as "proj1" |
@AndriiFilippov there can be a path starting with / on mac and linux so shouldn't be a problem |
@kolipakakondal @AndriiFilippov |
Description
Remove the first page for new project so we only have the second page with project templates/examples
Fixes # (IEP-1089)
(IEP-1062)
Type of change
Please delete options that are not relevant.
How has this been tested?
Try creating a new project from the project wizard. Create a project with and also without any templates. Also try to rename the examples folder to something else in esp-idf directory and see if you are still able to create a project.
Select a specific chip from the target selector in the project wizard the project should be created and the launchbar should show the target for that particular board.
Also verify that the default chip is always esp32
Test Configuration:
Checklist
Summary by CodeRabbit
The provided information contains a significant amount of technical details and code changes. To ensure confidentiality and focus on end-user impact, I'll need to summarize the changes based on the provided details.
Here's the revised summary:
New Features
Enhancements
Refactor
Bug Fixes
Documentation
Style
Tests
This summary focuses on the user-facing impact of the changes while maintaining confidentiality.