Skip to content

Conversation

amartya4256
Copy link
Contributor

This contributes to adding the workspace address to the display data on the initilization of the workbench. This contribution allows further SWT components to utilize this information.

Contributes to eclipse-platform/eclipse.platform.swt#1013

Copy link
Contributor

github-actions bot commented Oct 21, 2024

Test Results

 1 821 files  ±0   1 821 suites  ±0   1h 50m 21s ⏱️ + 2m 23s
 7 714 tests ±0   7 485 ✅  - 1  228 💤 ±0  1 ❌ +1 
24 303 runs  ±0  23 555 ✅  - 1  747 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit adb78fa. ± Comparison against base commit 4fbbe1b.

♻️ This comment has been updated with latest results.

@amartya4256 amartya4256 force-pushed the add_workspace_info_to_display branch 2 times, most recently from 2831958 to 3373da8 Compare October 25, 2024 10:31
@fedejeanne fedejeanne force-pushed the add_workspace_info_to_display branch from 3373da8 to c277967 Compare October 28, 2024 11:42
@HeikoKlare HeikoKlare force-pushed the add_workspace_info_to_display branch from c277967 to 497f830 Compare October 28, 2024 13:52
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

I've updated the data key with the proper value added to SWT via:

I've restricted the additional data to Windows and added proper handling for workspace-free applications (see eclipse-platform/eclipse.platform.swt#1548 (comment)). I've also simplified the retrieval of the workspace directory path string using more "proper" conversion instead of toString-conversions.

@fedejeanne can you have a look please?

@HeikoKlare HeikoKlare changed the title Add Workspace Address to Display data Set Edge data directory to workspace on Windows Oct 28, 2024
Copy link
Member

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

@HeikoKlare the code looks fine. I only have 2 nit-picks and one question/suggestion regarding relative locations (if that's even a thing).

If the question is irrelevant then please consider this PR approved on my account :-)

@HeikoKlare HeikoKlare force-pushed the add_workspace_info_to_display branch from 497f830 to 61a5b9f Compare October 28, 2024 14:53
This contributes to adding the workspace address to the display data on
workbench initialization as the data directory used by Edge browser.

Contributes to
eclipse-platform/eclipse.platform.swt#1013
@HeikoKlare HeikoKlare force-pushed the add_workspace_info_to_display branch from 61a5b9f to adb78fa Compare October 29, 2024 12:39
@HeikoKlare
Copy link
Contributor

Failing test is unrelated and documented: #195

@HeikoKlare HeikoKlare merged commit 910b6fc into eclipse-platform:master Oct 29, 2024
15 of 17 checks passed
@sratz
Copy link
Member

sratz commented Oct 30, 2024

With this change we now have a directory named EBWebView very prominently lying around in the user's workspace next to all the projects:

eclipse_workspace\
    .metadata\
    asdf\
    EBWebView\
    foo_bar\
    my_demo_project\
    ...

Should we maybe put this into a separate . directory?

eclipse_workspace\
    .metadata\
    .swt_edge\
        EBWebView\
    asdf\
    foo_bar\
    my_demo_project\
    ...

Or move it into the plugin-specific instance-scoped directory?

eclipse_workspace\
    .metadata\
        .plugins\
            org.eclipse.swt\
                EBWebView\
    asdf\
    foo_bar\
    my_demo_project\
    ...

@HeikoKlare
Copy link
Contributor

HeikoKlare commented Oct 30, 2024

Or move it into the plugin-specific instance-scoped directory?

That seems to be a proper place for this folder. Makes sense to move it there instead of having it placed directly in the workspace.

Proposal: #2469

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.

4 participants