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

Fix exception widget covering code lines #8344

Closed
wants to merge 1 commit into from
Closed

Fix exception widget covering code lines #8344

wants to merge 1 commit into from

Conversation

DoroNahari
Copy link
Contributor

@DoroNahari DoroNahari commented Aug 10, 2020 •
edited
Loading

Signed-off-by: Doron Nahari doron.nahari@sap.com

What it does

Fix exception widget overrides code lines
Fixes one issue in #8225

How to test

Create a.js script:

var a = 5;
console.log(a);
if (a>2) {
    throw new Error("Error");
}
console.log("End");

Create node launcher:

{
    "type": "node",
    "request": "launch",
    "name": "Launch Program",
    "program": "${workspaceFolder}/a.js",
    "runtimeExecutable": "node"
}

From debug pane set "Uncought Exceptions" breakpoint.
Launch "Launch Program"

Before this pr the exception widget was covering the code
After, the code pushed down:
ezgif com-video-to-gif

Review checklist

Reminder for reviewers

@DoroNahari DoroNahari changed the title Fix exception widget overrides code lines Fix exception widget covering code lines Aug 10, 2020
@DoroNahari
Fix exception widget overrides code lines
a783624 
Signed-off-by: Doron Nahari <doron.nahari@sap.com>
amiramw
amiramw approved these changes Aug 12, 2020
View reviewed changes
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.

Tested behavior and it seems good to me. Same as vs code.

@akosyakov can you review also the code changes? I believe most of it is yours.

@amiramw amiramw requested a review from akosyakov August 12, 2020 08:18
@akosyakov
Copy link
Member

I will have a look tomorrow morning.

@akosyakov akosyakov added the debug issues that related to debug functionality label Aug 12, 2020
akosyakov
akosyakov reviewed Aug 13, 2020
View reviewed changes
packages/debug/src/browser/style/index.css
@@ -375,6 +375,7 @@
/** Breakpoint Widget */
.theia-debug-breakpoint-widget {
display: flex;
height: calc(100% - 4px);
Copy link
Member

Choose a reason for hiding this comment

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

what for is 4px? we should avoid using hardcoded numbers, is it padding then it should be derived from var(--theia-ui-padding)

akosyakov
akosyakov reviewed Aug 13, 2020
View reviewed changes
packages/debug/src/browser/style/index.css
@@ -411,6 +412,7 @@
white-space: pre-wrap;
user-select: text;
overflow: hidden;
height: auto;
Copy link
Member

Choose a reason for hiding this comment

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

if there are other widgets based on zone widgets do they need to do the same now? Or it is only relevant for the exception widget?

@akosyakov
Copy link
Member

It would be good if PR does not require adding height property. It seems to be a breaking change for all downstreams using the zone widget. Is it feasible to avoid changing styles?

akosyakov
akosyakov reviewed Aug 13, 2020
View reviewed changes
packages/debug/src/browser/editor/debug-exception-widget.tsx
<div className='title'>{info.id ? `Exception has occurred: ${info.id}` : 'Exception has occurred.'}</div>
{info.description && <div className='description'>{info.description}</div>}
{stackTrace && <div className='stack-trace'>{stackTrace}</div>}
</React.Fragment>,
Copy link
Member

Choose a reason for hiding this comment

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

I assume it was to resize on each rendering? Why do we need to remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That callback was actually did nothing (as far as i saw).
While this component is rendering the containerNode.offsetHeight is 0 (i assume because it is not rendered yet in the zoneNode) and therefore heightInLines is 0.
Getting the containerNode.offsetHeight on onComputedHeight after the viewZone has rendered, worked for me.
https://github.com/eclipse-theia/theia/pull/8344/files#diff-e36030c15530769fcd2fe88693fd3cedR139

Copy link
Member

Choose a reason for hiding this comment

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

ok, it seems react semantic has changed and it is matter of rendering exception after zone

akosyakov
akosyakov reviewed Aug 13, 2020
View reviewed changes
packages/monaco/src/browser/monaco-editor-zone-widget.ts
@@ -41,9 +41,12 @@ export class MonacoEditorZoneWidget implements Disposable {
this.toHide
);

readonly editorLineHeight: number;
Copy link
Member

Choose a reason for hiding this comment

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

i don't think it should be like that, we already capture height in zone object and options are always available from preferences.

@akosyakov
Copy link
Member

not invasive PR is here: #8382

@akosyakov
Copy link
Member

superseded by #8382

@akosyakov akosyakov closed this Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akosyakov akosyakov akosyakov left review comments

@amiramw amiramw amiramw approved these changes

Assignees
No one assigned
Labels
debug issues that related to debug functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DoroNahari @akosyakov @amiramw