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.

Sign up for GitHub

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

Jump to bottom

9012-vscode-java-debug-error #9035

Closed
wants to merge 1 commit into from
Closed

9012-vscode-java-debug-error #9035

wants to merge 1 commit into from

Conversation

danarad05
Copy link
Contributor

@danarad05 danarad05 commented Feb 7, 2021 •
edited
Loading

Signed-off-by: Dan Arad dan.arad@sap.com

What it does

(a) Fixes #9012
(b) this PR solves:

  1. vscode-java-debug invokes vscode.window.registerTerminalLinkProvider upon extension activation. This method was not implemented in Theia - added it but implementation itself seems like an entire PR and don't know if it is wanted at all.
  2. after crossing this first problem, second problem was that JDTUtils threw exception when receiving file changed event where URI scheme was user_storage - illegal schema name (contains "_") - therefore I changed user_storage to user-storage scheme name

How to test

  1. Open in Theia java project
  2. After load you can run or debug project
    image

image

Review checklist

Reminder for reviewers

@danarad05
9012-vscode-java-debug-error:
7c7ca6d 
Issues this PR solves:
1) vscode-java-debug invokes vscode.window.registerTerminalLinkProvider upon extension activation. This method was not implemented in Theia - added it but implementation itself seems like an entire PR and don't know if it is wanted in this PR.
2) after crossing issue #1, second problem was that JDTUtils threw exception when receiving file changed event where URI scheme was user_storage - illegal schema name (_)

Signed-off-by: Dan Arad <dan.arad@sap.com>
@danarad05 danarad05 mentioned this pull request Feb 7, 2021
Closed
vince-fugnitto
vince-fugnitto reviewed Feb 8, 2021
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@danarad05 thank you for the pull-request, can you please update both the commit and pull-request description by including a proper link to the issue (not #1). It seems the link provided for the user-storage update is incorrect.

In addition, it might be better to separate the pull-request into two commits (stubbing terminal-link and updating user-storage). The commit title "9012-vscode-java-debug-error" does not help to accurately reflect the changes when viewing the git history.

packages/plugin/src/theia.d.ts
* A provider that enables detection and handling of links within terminals.
*/
// copied from https://github.com/microsoft/vscode/blob/bd20a720fba05f87ddb53c11c6af66936fd88a55/src/vs/vscode.d.ts#L5578-L5597

Copy link
Member

Choose a reason for hiding this comment

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

minor: remove unnecessary line.

Copy link
Contributor Author

@danarad05 danarad05 Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed. please see #9048

packages/preferences/src/browser/preferences-json-schema-contribution.ts
@@ -23,7 +23,7 @@ import { PreferenceConfigurations } from '@theia/core/lib/browser/preferences/pr
import { PreferenceScope } from '@theia/core/lib/browser';

const PREFERENCE_URI_PREFIX = 'vscode://schemas/settings/';
const USER_STORAGE_PREFIX = 'user_storage:/';
const USER_STORAGE_PREFIX = 'user-storage:/';
Copy link
Member

Choose a reason for hiding this comment

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

I understand that _ is not valid according to RFC 3986, but I'm not sure what the implications or side effects of changing the scheme are (it should most definitely be reported in the changelog).

For instance we should confirm that:

  • existing user preferences, extensions, keymaps can still be loaded from the user's home
  • any other potential use-cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vince-fugnitto
added remark in changelog - please see #9049 for that.
tested prefs, exts, keymaps and seems that scheme change works fine.

@vince-fugnitto vince-fugnitto added java issues related to the java language user storage issues related to user storage vscode issues related to VSCode compatibility labels Feb 8, 2021
@danarad05
Copy link
Contributor Author

@danarad05 thank you for the pull-request, can you please update both the commit and pull-request description by including a proper link to the issue (not #1). It seems the link provided for the user-storage update is incorrect.

In addition, it might be better to separate the pull-request into two commits (stubbing terminal-link and updating user-storage). The commit title "9012-vscode-java-debug-error" does not help to accurately reflect the changes when viewing the git history.

@vince-fugnitto

  1. Initial Project Setup #1 was referring to first problem, not issue no. 1. anyway, that is fixed.
  2. Split this PR to 2: 9012-registerTerminalLinkProvider-stub: for vscode-java-debug #9048 9012-updating-userstorageuri-scheme #9049

@danarad05
Copy link
Contributor Author

Split this PR to two (as per @vince-fugnitto's request) - #9048 #9049. Therefore closing this PR.

@danarad05 danarad05 closed this Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto left review comments

Assignees
No one assigned
Labels
java issues related to the java language user storage issues related to user storage vscode issues related to VSCode compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vscode-java-debug Error: It is not possible to provide debug adapter executable.
2 participants
@danarad05 @vince-fugnitto