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

add java-debug extension based on vscode-java-debug #3613

Merged
merged 10 commits into from
Dec 6, 2018
Merged

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Nov 23, 2018

TODO:

  • implement provide initial configurations
  • implement resolve configurations
  • hot replacement
  • user notifications
  • debug code lenses
  • user settings: https://github.com/Microsoft/vscode-java-debug#user-settings
  • reconnection testing
  • backend profiling with page reloading
  • java breakpoints are not rendered on session stop -> because of a cyclic call to session.dispose
  • add copyrights
  • open CQs

Debugging:
java-debug

HCR:
hotcode

Code lenses:
screen shot 2018-11-28 at 18 00 00

@akosyakov akosyakov changed the title add java-debug extension based on vscode-java-debug WIP add java-debug extension based on vscode-java-debug Nov 23, 2018
@akosyakov akosyakov force-pushed the ak/java_debug branch 9 times, most recently from db66fe4 to f355b4f Compare November 29, 2018 11:36
@akosyakov akosyakov changed the title WIP add java-debug extension based on vscode-java-debug add java-debug extension based on vscode-java-debug Nov 29, 2018
@akosyakov
Copy link
Member Author

It is ready for the review.

I've tested with: https://github.com/kasecato/vscode-javadebug-sample

@kittaakos
Copy link
Contributor

I am reviewing it now... (I am checking the browser version only: #3693)

Copy link
Contributor

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

Awesome! worked nicely for me.

@kittaakos
Copy link
Contributor

I cannot run on Windows:

:3000/#/c:/Users/kit…s/dev/tmp-java-ws:1 Uncaught (in promise) 
{success: false, message: "Illegal char <:> at index 2: /c:/Users/kittaakos/dev/tmp-java-ws", request_seq: 2, command: "launch", body: {…}, …}
body: {error: {…}}
command: "launch"
message: "Illegal char <:> at index 2: /c:/Users/kittaakos/dev/tmp-java-ws"
request_seq: 2
seq: 2
success: false
type: "response"
__proto__: Object

@kittaakos
Copy link
Contributor

I cannot run on Windows:

Although it is just a guess, the problem could be here: https://github.com/theia-ide/theia/pull/3613/files#diff-70a46c1aaadf49045db1097bc9e1f699R89

child.bind(MessagingContainer).toConstantValue(container);
child.bind(MessagingContribution).toSelf();
return child.get(MessagingContribution);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

should be singleton

@@ -54,7 +54,7 @@
"name": "Launch Backend",
"program": "${workspaceRoot}/examples/browser/src-gen/backend/main.js",
"args": [
"--log-level=debug",
"--hostname=0.0.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

hello, I don't see how this change it is related to this feature ?

Copy link
Member Author

@akosyakov akosyakov Nov 30, 2018

Choose a reason for hiding this comment

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

it is related because I needed it to debug extensions

@@ -1,5 +1,6 @@
{
"rules": {
"deprecation": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

hello, I don't see how this change it is related to this feature ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've deprecated some APIs and want other developers to notice it.

Copy link
Contributor

Choose a reason for hiding this comment

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

my point is why deprecated notice is not discussed first
and why it should be deprecated

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, let's discuss deprecation on Tuesday

btw it is just warning, it won't break builds and so on. There is also no APIs broken in core and developers can stop using moved APIs over time.

@@ -94,7 +91,7 @@ export class MessageClient {
* To be implemented by an extension, e.g. by the messages extension.
*/
showProgress(progressId: string, message: ProgressMessage, cancellationToken: CancellationToken): Promise<string | undefined> {
this.logger.info(message.text);
console.info(message.text);
Copy link
Contributor

Choose a reason for hiding this comment

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

why ?

Copy link
Member Author

Choose a reason for hiding this comment

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

all console calls are implemented by logger already, there is no need to inject Logger

@benoitf
Copy link
Contributor

benoitf commented Nov 30, 2018

hello, why it is not in a separate repository ?
I thought all java stuff has to be moved out of main theia repository

@svenefftinge
Copy link
Contributor

True, but we should move everything at once.

@akosyakov
Copy link
Member Author

akosyakov commented Dec 3, 2018

@benoitf @tsmaeder @tolusha I've just double checked and not found any usages of QuickPickService in the plugin extension, it is not affected at all by this PR

@benoitf
Copy link
Contributor

benoitf commented Dec 3, 2018

@akosyakov AFAIK plug-in extension is only using MonacoQuickOpenService yes

@akosyakov
Copy link
Member Author

@benoitf ok, it was not changed

@akosyakov akosyakov force-pushed the ak/java_debug branch 3 times, most recently from fbbad1c to 47692a4 Compare December 3, 2018 13:58
@akosyakov
Copy link
Member Author

@kittaakos I've addressed all your comments.

@kittaakos
Copy link
Contributor

I've addressed all your comments.

I am OK proceeding with this. I will take care of the Windows path issue (#3709) and for the time being, I remove the debugging extensions (#3693) from the electron example.

@akosyakov
Copy link
Member Author

@kittaakos thank you

@akosyakov
Copy link
Member Author

@marcdumais-work
Copy link
Contributor

@marcdumais-work CQs:

The CQs looks good, thanks Anton.

We now need to wait until the Eclipse Foundation has the time to look at them. In many cases they will give "parallel IP" check-in permission, if, after a summary look, the Foundation thinks the CQ does not raise red flags. When we have that permission for both CQs, we can merge.

@marcdumais-work
Copy link
Contributor

Update on the CQs: no longer blocking the merge of this PR:

18453: license_certified
18454: permission to merge through parallel IP process.

@marcdumais-work marcdumais-work removed the CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) label Dec 5, 2018
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
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.

None yet

7 participants