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

Fire breakpoint events only if breakpoint metadata changes #12183

Merged
merged 3 commits into from
Feb 23, 2023
Merged

Fire breakpoint events only if breakpoint metadata changes #12183

merged 3 commits into from
Feb 23, 2023

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Feb 15, 2023 •
edited
Loading

What it does

Fixes #11878.

This PR fixes the stack overflow component of #11878 by ensuring that debug editor models provide the ID of breakpoints they believe have moved rather than retrieving the modified breakpoint from the breakpoint manager (whose state may have been updated in the meantime by a different model for the same editor) using location data and by ensuring that the breakpoint manager doesn't fire events when setMarkers is called but no marker - breakpoint - metadata changes.

How to test

  1. Add a breakpoint to an editor.
  2. Split that editor so that two (or more) editor instances are opened for the file.
  3. Perform a text edit in one of the editor instances that moves the breakpoint up or down.
  4. The breakpoint should move, the UI should remain responsive, and the console should not show any stack overflow errors.
  5. Other breakpoint actions (adding, removing, modifying with conditions, etc.) should work as before.

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work added the debug issues that related to debug functionality label Feb 15, 2023
@colin-grant-work colin-grant-work force-pushed the bugfix/redundant-breakpoint-events branch from d8e83e3 to 27c5d5c Compare February 22, 2023 22:08
@colin-grant-work colin-grant-work force-pushed the bugfix/redundant-breakpoint-events branch from d6a881b to 7cd7c32 Compare February 22, 2023 22:15
vince-fugnitto
vince-fugnitto approved these changes Feb 23, 2023
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.

I can confirm that the changes work as expected, and the errors no longer exist when a split editor is modified with breakpoints 👍

@colin-grant-work colin-grant-work merged commit cd74848 into eclipse-theia:master Feb 23, 2023
@colin-grant-work colin-grant-work deleted the bugfix/redundant-breakpoint-events branch February 23, 2023 16:46
@colin-grant-work colin-grant-work added this to the 1.35.0 milestone Feb 23, 2023
@colin-grant-work colin-grant-work mentioned this pull request Mar 1, 2023
Merged
1 task
@pisv pisv mentioned this pull request May 22, 2023
Closed
pisv added a commit to pisv/theia that referenced this pull request Oct 8, 2023
@pisv
Fix eclipse-theia#12546: UI not updated upon editing breakpoint condi…
f74ae7b 
…tion

Fixes eclipse-theia#12546, which is a regression introduced by eclipse-theia#12183, by ensuring that
`BreakpointManager.setMarkers` fires a `SourceBreakpointsChangeEvent` when
`oldMarker === newMarker`, as there is no way to actually detect a change
in this case.
@pisv pisv mentioned this pull request Oct 8, 2023
Merged
1 task
pisv added a commit to pisv/theia that referenced this pull request Nov 3, 2023
@pisv
Fix eclipse-theia#12546: UI not updated upon editing breakpoint condi…
5964e6c 
…tion

Fixes eclipse-theia#12546, which is a regression introduced by eclipse-theia#12183, by ensuring that
`BreakpointManager.setMarkers` fires a `SourceBreakpointsChangeEvent` when
`oldMarker === newMarker`, as there is no way to actually detect a change
in this case.
tsmaeder pushed a commit that referenced this pull request Nov 3, 2023
@pisv
Fix #12546: UI not updated upon editing breakpoint condition (#12980)
aab4102 
Fixes #12546, which is a regression introduced by #12183, by ensuring that
`BreakpointManager.setMarkers` fires a `SourceBreakpointsChangeEvent` when
`oldMarker === newMarker`, as there is no way to actually detect a change
in this case.
This was referenced Mar 22, 2024
Open
Open
Open
Open
Open
Open
Open
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 approved these changes

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

Successfully merging this pull request may close these issues.

Multiple DebugEditorModels for same URI lead to stack overflow
2 participants
@colin-grant-work @vince-fugnitto