Skip to content
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-908: Adding the code to support the default workspace directory #736

Closed
wants to merge 3 commits into from

Conversation

alirana01
Copy link
Collaborator

@alirana01 alirana01 commented Apr 4, 2023

Description

The issue with installer where it sometimes references to the idf toolchain directory for workspace should be resolved now the default workspace is now either in the user home directory or is pointed by the defaultWorkspace property in the esp_idf.json file

Fixes # (IEP-908)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Can be a breaking change but code reviews should be able to identify them

How has this been tested?

Test 1:

  • Launch the espressif ide
  • For the first launch the workspace should now point to (USER_HOME/workspace) directory in the workspace selection dialog
    Test 2:
  • Add the esp_idf.json file if trying without the installer in the espressif ide root directory
  • Check to see if the defaultWorkspace property is present if not add yourself and point to a path
  • Launch the espressif ide the default workspace on the first launch should now point to the directory defined in previous step.
    Test3:
    Also make sure to change the workspace to some other directory and try to relaunch the IDE the workspace selection dialog on second start should always point to the last used directory.

Test Configuration:

  • ESP-IDF Version: Any
  • OS (Windows,Linux and macOS): All

Checklist

  • PR Self Reviewed
  • Applied Code formatting
  • Verified on all platforms - Windows,Linux and macOS

The issue with installer where it sometimes references to the idf toolchain directory for workspace should be resolved now the default workspace is now either in the user home directory or is pointed by the defaultWorkspace property in the esp_idf.json file
@alirana01 alirana01 self-assigned this Apr 4, 2023
Copy link
Collaborator Author

@alirana01 alirana01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overriding the default IDEApplication class and only the required methods that still required some internal code to be rewritten. I could not found a workaround to set the workspace via configs. Other methods only allow setting the default workspace but that also doesnt work at times.
I have also refactored and isolated some code that could be reused in multiple classes

@kolipakakondal kolipakakondal added this to the 2.9.2 milestone Apr 11, 2023
JSONParser parser = new JSONParser();
try
{
JSONObject jsonObj = (JSONObject) parser.parse(new FileReader(idf_json_file));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to close the FileReader with try-with-resources and provide an explicit character set to the FileReader constructor (e.g. StandardCharsets.UTF_8) to avoid dependency on OS code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sigmaaa resolved in the latest commit

Copy link
Collaborator

@sigmaaa sigmaaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alirana01, thanks for the PR, need to close FileReader, otherwise LGTM

Copy link
Collaborator

@sigmaaa sigmaaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kolipakakondal
Copy link
Collaborator

Setting the default workspace directory(/user-dir/workspace) in the espressif-ide should be resolved with #563

There was a bug earlier, and it was fixed with this

@kolipakakondal kolipakakondal removed this from the v2.9.2 milestone May 19, 2023
@kolipakakondal kolipakakondal deleted the IEP-908 branch November 4, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants