Fix debugger code highlighting while stepping, enable Game Lab breakpoints for project level

[cpirich]

• Enable breakpoints and debug buttons (pause/continue, step in, step over, step out) for the Gamelab project level in preparation to make it available once again on curriculum levels
• Fix debugger bug (impacts Applab): If you hit a breakpoint and then started pressing "Step Over" in our debugger, we would never highlight the current code in the block or text editor. So you had no idea where you were in the program. This regressed in Feb 2016: see ac9ed40#diff-0e77e03de590cbade1162606bd287623
  • We had an optimization that said: If we are hiding source or running at max speed and we're not paused, then just get the user code line and don't select the code in the editor. In Feb 2016, we removed the and we're paused from that line of code. (paused means that we are stopped or in one of the stepping states). Rather than restore that code exactly, I've set reachedBreak = true when we complete a step operation (which was already occurring for "step in"). This targeted change avoids a perf issue when you try to "Step Out" from the draw() loop in Gamelab - since we can't really step out of that function.

[caleybrock]

We pulled your branch and have been playing around. Here's some scenarios we came across.

• Step in while program is not running does not work. This still works in App Lab.
• Stepping over each line of a program that draws sprites gets stuck in drawSprites and doesn't draw anything. Example program: https://studio.code.org/projects/gamelab/6pRUiMiQZJQUZQCI5wAY9Q/edit
• When there is not draw loop, but drawing functions like background and ellipse it doesn't draw until the end when stepping through. Is this expected? When we add the same code (no sprites) to a draw loop, it works as expected.

There is a lot of cool functionality here, and there is a lot that works, but my concern is that the last two of these examples might cause more confusion for students than good.

[islemaster]

It seems like the intended functionality is that stepping over the last statement in draw() runs until the next line of user code (which is usually the first line of draw()). However, in local testing it seems like this case often skips the first line of draw() and breaks on the second line, resulting in surprising behavior.

Example:

1 function draw() {
2   background('pink');
3   rect(10, 10, 10, 10);
4   rect(20, 10, 10, 10);
5 }

If I put a breakpoint on line 2 and click "Run" everything behaves as expected:

Action	Highlight line	Playspace
Run	2
Step over	3
Step over	4
Step over	2
Step over	3

...and so on, 2, 3, 4, 2, 3, 4.

However, if I put my breakpoint on line 4, I'll never stop on line 2:

Action	Highlight line	Playspace
Run	4
Step over	3
Step over	4
Step over	3

The cycle continues 4, 3, 4, 3... Where I'd expect to see the same cycle, just starting at a different step.

This explains the "stuck on drawSprites" issue, because for a simple program:

function draw() {
  background('white');
  drawSprites();
}

...it's just skipping the first line each time, making it look like drawSprites stays highlighted.

[cpirich]

Yes, I noticed that we never stop on the first line inside draw() when stepping "out and around" - that's been that way forever. I'll log that. We have always had a general issue related to how to handle stepping when exiting from a function that it outside of user code. It's essentially the same issue - but I'll log that as well.

Personally, I think if your step will take you out of user code (essentially, out of draw()), you should probably go back to "running". But we need to try that and see how it feels.

I'll look at the 3rd issue: (no draw() loop).

[cpirich]

@caleybrock The issue you found is very interesting. It has to do with how we run this code with respect to p5.play's notion of the preload and setup phase. I have an idea of how to fix this so it will work in the way that you might expect.

[islemaster]

I like the idea of going back to "running" when you step out of user code. We were kind of enjoying the loop back to the top of draw (), but that's easy to recreate with a breakpoint, and there are weird cases where the next user code is a queued callback instead of the draw loop.

…

On Jan 10, 2018 4:52 PM, "Chris Pirich" ***@***.***> wrote: @caleybrock <https://github.com/caleybrock> The issue you found is very interesting. It has to do with how we run this code with respect to p5.play's notion of the preload and setup phase. I have an idea of how to fix this so it will work in the way that you might expect. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#19918 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABinkayiN6Pz40WAK8BMX9cIo3gkljwdks5tJVtigaJpZM4RaDd9> .

[cpirich]

PR has been amended to fix @caleybrock 3rd issue discovered. You can now step through global code and see what it is doing. The "setup epilogue" is now broken into two phases. The first phase runs earlier, so the p5.play canvases are visible after we start running the user's code. But we still don't tell p5.play that "setup is really done" until after the global code is complete.

Also, new debugSprites() functionality has been added. It is not yet enabled in the UI, but it allows you to press a button and make all sprites have debug = true.

[cpirich]

@islemaster PR has been amended to cause the debugger to go back into a running state when you exit from a callback or event while stepping (step in/out/over). Verified in gamelab's draw() function and in an applab onEvent() handler. This behavior seems reasonable and expected.

[islemaster]

Awesome, I'll pull and try it out in a little bit.

[islemaster]

Gave this a try. I feel really good about the new behavior, with one exception: When I step out of user code and end up back in "running" state, the highlight of the last line I was broken on doesn't clear. @caleybrock @epeach are you okay with going back to "running" when we step out of draw()? I think it's actually the most consistent thing we can do.

[caleybrock]

why isn't this logic the same as line 283?

[cpirich]

I just made it match what it previously did (by reverting your PR from 1 year ago). My guess is that there was no need to check hideSource for enabling breakpoints, since you can't see the UI component needed to enable the breakpoints.

In its current form (without the addition hideSource check), we match the previous behavior - and we match the current applab behavior / source code.

Adding the additional check certainly wouldn't hurt anything, but it doesn't seem strictly necessary either.

[caleybrock]

I think this fixes the scenarios I was concerned with, with the exception of the last highlight at the end.

[islemaster]

I'd also like to see tests over this fixed behavior, because in the past we've broken debugger behavior in game lab without noticing right away. This might be a great job for gamelab level tests - if we can set up a small example program, set breakpoints, use the debug commands and verify that we stop on the expected lines, that would be great validation. Maybe Caley or Erin can help with this, they've written game lab level tests recently.

[caleybrock]

Here's the PR with Erin's tests: #19677

[cpirich]

@islemaster the last line of the debugger staying highlighted is another known issue that I had been tracking.

What do you think are next steps here? Should we merge and then resolve adding tests?

[islemaster]

I'm okay with tackling tests in a follow-up PR - they may be a good bit of work, especially if we try to detect what's been drawn at each step.

I've got mixed feelings about merging before the "last line staying highlighted" gets fixed. On one hand this is mostly working as expected. On the other hand, we disabled this in the first place because it was visually misleading, becoming more confusing than helpful to students. Still having that incorrect visual cue worries me a bit. If we merge this now, any sense of how long it will be before we address it?

Any thoughts on Caley's question in GameLab.js?

[caleybrock]

I'm fine with test as follow-on PR, and I don't have strong feelings on the last line being highlighted. We could give @poorvasingal a demo tomorrow for her to make a call.

[cpirich]

@islemaster the "last line stays highlighted" impacts Applab as well, where the debugger has always been enabled. Just hit a breakpoint, then press "Continue", and you'll see that the highlighting doesn't go away. Since no one has reported that as a problem there, I don't see it as a deal-breaker here (where it is only being enabled for /p/gamelab for now)

[islemaster]

Awesome. I'm satisfied. Thanks for doing the work to re-enable this! Let's merge, and follow up with some tests (which I'm happy to help with).

…

On Jan 12, 2018 8:55 AM, "Chris Pirich" ***@***.***> wrote: @islemaster <https://github.com/islemaster> the "last line stays highlighted" impacts Applab as well, where the debugger has stayed on for all this time. Just hit a breakpoint, then press "Continue". So, since no one has reported that as a problem there, I don't see it as a deal-breaker here (where it is only being enabled for /p/gamelab for now) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19918 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABinkbJJ_NKl7nPsV6aQgfehTQ7uSEYLks5tJ4t_gaJpZM4RaDd9> .