Fix enable/disable of verified breakpoints if moved #9479
Conversation
Signed-off-by: thegecko <rob.moran@arm.com>
vince-fugnitto
added
bug
There was a problem hiding this comment.
I confirmed that it fixes the issue where if a breakpoint exists on a non-code line (comment), it moves to the next corresponding code line. I verified this with multiple breakpoints as well:
Like vscode, it also fails if we do something like the following :)
| @@ -50,7 +50,8 @@ export class DebugSourceBreakpoint extends DebugBreakpoint<SourceBreakpoint> imp | |||
| const { uri, raw } = this; | |||
| let shouldUpdate = false; | |||
| let breakpoints = raw && this.doRemove(this.origins.filter(origin => !(origin.raw.line === raw.line && origin.raw.column === raw.column))); | |||
| if (breakpoints) { | |||
| // Check for breakpoints array with at least one entry | |||
| if (breakpoints && breakpoints.length) { | |||
There was a problem hiding this comment.
OK, are you saying this re-introduces a regression?
There was a problem hiding this comment.
OK, are you saying this re-introduces a regression?
No :) Just saying that it doesn't fix #8222 entirely (only fixes it for single breakpoints).
There was a problem hiding this comment.
Aha! thanks. Just a little bit better, then. Shame
Is this specific to rhyming logpoints? 😄 I'll take a look later today. |
There was a problem hiding this comment.
I confirm that it fixes #8222, but only when dealing with one breakpoint. If I add more breakpoints and try to disable those, it will remove them until only one is left. Super weird.
I guess this change improves things a little bit. If you are happy with that I am not against approving and merging.
Yeah, its a step in the right direction, sorry it wasn't the golden bullet we'd hoped for. Shall we get it in? |
Signed-off-by: thegecko rob.moran@arm.com
What it does
This PR fixes the issue of not being able to enable/disable verified breakpoints if they have moved their location.
This is as mentioned in: #8222 (comment)
@marechal-p this may also fix the issue you've seen in #8222 , but I've been unable to verify that.
How to test
Review checklist
Reminder for reviewers