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

Breakpoints which appear in margin and injected text aren't factored into glyph margin width #180619

Closed
joyceerhl opened this issue Apr 22, 2023 · 0 comments · Fixed by #180620
Closed
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Milestone

Comments

@joyceerhl
Copy link
Contributor

  1. Have an active debug session
  2. Add a bookmark to one line
  3. Add a breakpoint to the same line where the breakpoint ends up being an inline breakpoint
  4. 🐛 the margin resizes so that the bookmark on the line in step 2 is also gone

Debugging this in devtools, this appears to be because the breakpoint does not appear in the result of getAllMarginDecorations. Therefore it's not taken into account when calculating the size of the margin.

const decorations = this.model.getAllMarginDecorations();

When we actually go to render the decorations, we use a different source of information ctx.getDecorationsInViewport, where the breakpoint does show up, so we end up rendering it even though no lane space was allocated for it.

const decorations = ctx.getDecorationsInViewport();

image

Those inline breakpoints appear by using injected text:

before: renderInline ? {
content: noBreakWhitespace,
inlineClassName: `debug-breakpoint-placeholder`,
inlineClassNameAffectsLetterSpacing: true
} : undefined,

I think the problem is that the fast path for getting just margin decorations doesn't search the injected text interval tree for margin decorations (because I didn't realize that margin decorations could appear in the injected text interval tree when implementing #179974).

private _search(filterOwnerId: number, filterOutValidation: boolean, overviewRulerOnly: boolean, cachedVersionId: number, onlyMarginDecorations: boolean): IntervalNode[] {
if (overviewRulerOnly) {
return this._decorationsTree1.search(filterOwnerId, filterOutValidation, cachedVersionId, onlyMarginDecorations);
} else if (onlyMarginDecorations) {
const r0 = this._decorationsTree0.search(filterOwnerId, filterOutValidation, cachedVersionId, onlyMarginDecorations);
const r1 = this._decorationsTree1.search(filterOwnerId, filterOutValidation, cachedVersionId, onlyMarginDecorations);
return r0.concat(r1);
} else {
const r0 = this._decorationsTree0.search(filterOwnerId, filterOutValidation, cachedVersionId, onlyMarginDecorations);
const r1 = this._decorationsTree1.search(filterOwnerId, filterOutValidation, cachedVersionId, onlyMarginDecorations);
const r2 = this._injectedTextDecorationsTree.search(filterOwnerId, filterOutValidation, cachedVersionId, onlyMarginDecorations);
return r0.concat(r1).concat(r2);
}
}

Only caught this now because inline breakpoints only show up when there are active debug sessions, and I didn't have a case where I set a breakpoint on a line with another decoration in a debug session till today :/

@joyceerhl joyceerhl added the bug Issue identified by VS Code Team member as probable bug label Apr 22, 2023
@joyceerhl joyceerhl added this to the April 2023 milestone Apr 22, 2023
@joyceerhl joyceerhl self-assigned this Apr 22, 2023
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Apr 23, 2023
@amunger amunger added the verified Verification succeeded label Apr 27, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants