Add breakpoint from the debug console support #368
Conversation
omarArm
force-pushed
the
addBreakpointFeatures
branch
from
52a9e70 to
a28a790
Compare
There was a problem hiding this comment.
Please remove changes supposed to be in #366 only.
Also, please think about some useful tests for this.
omarArm
force-pushed
the
addBreakpointFeatures
branch
from
a28a790 to
6be3dc1
Compare
|
I cleaned the irrelevant changes. Sorry about that, I did something on my fork and it got reflected in the PR. I will think of some tests sure and add them in a separate PR |
There was a problem hiding this comment.
This overall looks good to me. I very much like that you have implemented this feature.
I have a few requests, but I'll leave it to @jreineckearm for a final review on this:
- Can you please add some tests for the
isBreakpointInRightLocationrelated logic 2. Can you add a test for user inserting breakpoint and deleting breakpoint via CLI - Can you document UpdateBreakpointView somewhere? Perhaps a new method called sendUpdateBreakpointView that all there locations call and that new method can have some doc?
|
Thanks, @jonahgraham, for the feedback! @omarArm is working on the tests but off today. We'll make sure to cover your feedback when he's back. |
Author: Omar Elkhouly <omar.elkhouly@arm.com> Date: Tues May 13 Co-authored-by: Jens Reinecke <jens.reinecke@arm.com>
omarArm
force-pushed
the
addBreakpointFeatures
branch
from
6a51657 to
ecb09d1
Compare
|
Thanks @jonahgraham for the feedback, I added two extra tests now and refactored my code a bit. The tests are getting longer and longer unfortunately though |
|
That's great @omarArm - let me know when all the tests are passing and want me to re-review this. Let me know if you have any questions on how to diagnose test failures on the buildmachine. If you haven't already seen it, please have a look at this part of the readme: https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/blob/main/src/integration-tests/README.md#test-logs-and-test-reports |
There was a problem hiding this comment.
Awesome work on this, @omarArm ! I know this turned out to be more complex than I'd initially thought. Thanks for staying on it.
Good to merge, will do so in the morning as I have to call it a day now. This gives also Jonah a chance to have another look if he fancies.
* Added support for breakpoint setting from the debug console * Adding tests for changes in the handleGDBNotify function * Adding tests for better setBreakpointRequest coverage * skipping breakpoint tests when gdb is not in async mode Author: Omar Elkhouly <omar.elkhouly@arm.com> Date: Tues May 13 --------- Co-authored-by: Jens Reinecke <jens.reinecke@arm.com>
3 participants
Previous behaviour:
Users were able to add breakpoints from the GUI of vscode itself. However, if a user set a breakpoint from the debug console, the breakpoint would not get reflected on the GUI with a red dot, even though the breakpoint was set by GDB in the backend.
Changes:
Users can now set breakpoints from the debug console and the behaviour will be reflected on the GUI. The GDB breakpoint notifications (breakpoint-created, breakpoint-modified, breakpoint-deleted) are now handled on the adapter's side.
Problems:
VScode has a bug in which standard Breakpoint events do not trigger a re-rendering of the breakpoint GUI. Breakpoints are visible in the breakpoints list on the side panel, but red dots are not visible on the source code. Therefore, I created a custom event that would trigger a re-rendering for vscode, so that red dots can appear on the source file
VScode