frontend icon indicating copy to clipboard operation
frontend copied to clipboard

Published 20 hours ago • verified publisher icon source-academy

Playground breakpoints: removal of last breakpoint leads to spurious "Run" action

Open martin-henz opened this issue 5 years ago • 2 comments

Here is the sequence. Notice the missing 4 in the last screen shot: Removing the last breakpoint triggered a "Run".

Screenshot 2020-08-05 at 11 40 18 AM Screenshot 2020-08-05 at 11 40 26 AM Screenshot 2020-08-05 at 11 40 32 AM Screenshot 2020-08-05 at 11 40 49 AM

martin-henz avatar Aug 05 '20 03:08 martin-henz

This bug is still there. Just tried. Here is the program: https://share.sourceacademy.nus.edu.sg/1ijs1

martin-henz avatar May 27 '21 05:05 martin-henz

This bug is still there.

https://share.sourceacademy.nus.edu.sg/5ya5k

The bug shows up in Source §3 but also in Source §1.

martin-henz avatar Nov 02 '21 11:11 martin-henz

When the number of breakpoints goes from 1 to 0, line 763 is run which clears the REPL output: https://github.com/source-academy/frontend/blob/5391c8a76bc51822cac281d29b54186c385a8cc4/src/pages/playground/Playground.tsx#L744-L770

The reason this is done is because playground.debuggerContext.result.value is an array of objects used by the stepper (at least in Source 1) when the breakpoint is set: image

Whereas without breakpoints, the playground.debuggerContext.result.value is an actual value that the REPL can display: image

As such, we are forced to clear the REPL output because if the stepper's playground.debuggerContext.result.value is parsed by the normal REPL, the frontend crashes. @martin-henz It would seem that playground.debuggerContext.result.value is being used for 2 different purposes, and should be refactored into separate variables if we want to fix this bug.

ianyong avatar Feb 15 '23 17:02 ianyong

The stepper's use of breakpoints is quite lame right now: It switches to the stepper when a breakpoint is set, but it doesn't do anything with the breakpoints. It seems like the use of breakpoints by stepper wasn't well designed. Would it be a lot of trouble removing breakpoints from the stepper entirely? The loss would be minimal. Then, once we have a clear idea how to make use of breakpoints in the stepper, we can revisit this, hopefully with a better solution.

martin-henz avatar Feb 15 '23 22:02 martin-henz

The stepper's use of breakpoints is quite lame right now: It switches to the stepper when a breakpoint is set, but it doesn't do anything with the breakpoints. It seems like the use of breakpoints by stepper wasn't well designed. Would it be a lot of trouble removing breakpoints from the stepper entirely? The loss would be minimal. Then, once we have a clear idea how to make use of breakpoints in the stepper, we can revisit this, hopefully with a better solution.

It seems that the refactoring might not be as simple as removing breakpoints from the stepper entirely since the debuggerContext.result.value is used by the stepper. In fact, the current design has led to previous bugs such as the one fixed in #2305. There is an issue #2306 that is tracking the same refactoring that is necessary to solve this issue.

ianyong avatar Feb 17 '23 04:02 ianyong