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

[martin-henz]

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

[martin-henz]

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

[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.

[ianyong]

When the number of breakpoints goes from 1 to 0, line 763 is run which clears the REPL output:

frontend/src/pages/playground/Playground.tsx

Lines 744 to 770 in 5391c8a

	const handleEditorUpdateBreakpoints = React.useCallback(
	(breakpoints: string[]) => {
	// get rid of holes in array
	const numberOfBreakpoints = breakpoints.filter(arrayItem => !!arrayItem).length;
	if (numberOfBreakpoints > 0) {
	setHasBreakpoints(true);
	if (propsRef.current.playgroundSourceChapter <= 2) {
	/**
	* There are breakpoints set on Source Chapter 2, so we set the
	* Redux state for the editor to evaluate to the substituter
	*/
	propsRef.current.handleUsingSubst(true);
	}
	}
	if (numberOfBreakpoints === 0) {
	setHasBreakpoints(false);
	if (selectedTab !== SideContentType.substVisualizer) {
	propsRef.current.handleReplOutputClear();
	propsRef.current.handleUsingSubst(false);
	}
	}
	propsRef.current.handleEditorUpdateBreakpoints(breakpoints);
	},
	[selectedTab]
	);

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:

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

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.

[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.

[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.

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.