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

fix debug exception height computation #8382

Merged

Conversation

akosyakov
Copy link
Member

Signed-off-by: Anton Kosyakov anton.kosyakov@typefox.io

What it does

How to test

  • create a.js file with following content:
var a = 5;
console.log(a);
if (a>2) {
    throw new Error("Error");
}
console.log("End");
  • use Launch with Node.js debug configuration to debug it
  • enable exception breakpoint
  • rerun the debug configuration again
  • the exception breakpoint should not overflow editor content
  • it should stay the same when you modify a file
  • when you switch to another file the exception widget should not be visible
  • but should appear when you come back

Review checklist

Reminder for reviewers

@akosyakov akosyakov added the debug issues that related to debug functionality label Aug 14, 2020
@akosyakov akosyakov force-pushed the akosyakov/node-debug-exception-8225 branch from bd359d8 to 607bc1b Compare August 14, 2020 11:59
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.

The changes work very well:

master:

debug-2

pull-request:

debugging-pr

@danidoedel
Copy link

The margin at the bottom is now bigger than the one at the top. Is that intended behavior? According to the screenshot in the ticket it seems to be equal for VS Code. Just wondering 😉

@akosyakov
Copy link
Member Author

@danidoedel I've noticed it too. That's because zone (distance between lines) height is computed by Monaco, but content height is by browser. If it try to match it then with each edit content height grows by 2px which leads to overflow eventually. I guess it is the same issue in VS Code.

@DoroNahari
Copy link
Contributor

I tried this change on gitpod now and it doesn't worked well for me:
image

@akosyakov
Copy link
Member Author

@DoroNahari hm, I cannot reproduce it. Are sure that you are trying Theia from this PR, not Gitpod Theia? on the image below it is a tab without Gitpod logo:
Screenshot 2020-08-16 at 14 24 22

@vince-fugnitto @amiramw Could you try to reproduce?

@vince-fugnitto
Copy link
Member

@vince-fugnitto @amiramw Could you try to reproduce?

I verified again using Gitpod and it worked correctly:

Screen Shot 2020-08-16 at 11 43 32 AM

Copy link
Member

@amiramw amiramw left a comment

Choose a reason for hiding this comment

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

For me it tested successfully. Both these issues are solved now:

  • Stripe covering the code.
  • When moving to another opened tab the stripe stays.

(in #8344 the second issue was not solved)

There is some difference on behavior comparing to vs code but I think I'm fine with it:

when you resize the browser window to be smaller the stripe height stays the same and the bottom of the stack trace is disappearing. In vs code the stripe height is getting bigger on such resize.

@@ -175,7 +175,7 @@ export class DebugEditorModel implements Disposable {

protected async toggleExceptionWidget(): Promise<void> {
const { currentFrame } = this.sessions;
if (!currentFrame) {
if (!currentFrame || !currentFrame.source || currentFrame.source.uri.toString() !== this.uri.toString()) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe currentFrame?.source?.uri?.toString() !== this.uri.toString()

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov akosyakov force-pushed the akosyakov/node-debug-exception-8225 branch from 607bc1b to fee75c2 Compare August 17, 2020 07:28
@akosyakov
Copy link
Member Author

@amiramw Could you try again? resizing should be handled as well now

@amiramw
Copy link
Member

amiramw commented Aug 17, 2020

Resizing is responding well now. 👍

@akosyakov
Copy link
Member Author

@amiramw Could you check please with @DoroNahari as well? If you can do it offline. It is important for me that we don't overlook something.

@DoroNahari
Copy link
Contributor

LGTM

@akosyakov akosyakov merged commit 4fe3b12 into eclipse-theia:master Aug 17, 2020
@akosyakov akosyakov deleted the akosyakov/node-debug-exception-8225 branch August 17, 2020 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node debug exception breakpoint stripe issues
5 participants