Implement breakpoint feature

[inguin]

Breakpoints work like in pretty much any IDE: Clicking a line number toggles a breakpoint, execution pauses before that instruction. The feature should probably be mentioned in some tutorial email, but I'm not sure where it might fit best.

Tiny issue: It's currently not possible to enable or disable breakpoints when the code tab is locked, i.e. while the program is running.

[inguin]

Just noticed there is an open feature request for this (#227). Looping in @yancouto.

[rilifon]

Sorry for the delay in reviewing inguin, I usually only work on Marvinc on mondays.

So I added your fork to a remote branch to test it out, and there are some issues I wanted you to clear.

First, I don't think your changes to highlighting previous or current instruction are helping. The box around the current line is always in par with the indicating arrow, both pointing to the next instruction, and not the current one. I believe your intentions were to make the box around the current instruction (the one that just played) and the arrow pointing to the next. I think this could be useful and I approve the change, just need to fix the behaviour. Also, I think visually I still prefer the line below the current instruction instead of a box, but this can be easily changed later.

For breakpoints, everything seems fine after I tested a little bit on my end. I have only two issues: First, change the effect visually. Instead of a red box around the line number, just make a red circle on the left of the number. The second one is making it possible to toggle the breakpoints during a running code, since this will greatly improve the usage.

Both of these request I don't mind working on after merging your branch, but since its your code you may want to do it yourself ^^

Anyway, if you can fix this behaviour on the highlighting issue, I can merge your branch right away. Let me know if you want to work on these other things, or else I'll do them myself :)

Thanks for the PR!

[inguin]

Hi Rilifon! No, my intention was not to highlight both the previous and the next instruction at the same time. I checked a few IDEs (Eclipse and Visual Studio Code), and they both highlight the next line to be executed.

However, just changing the highlighting logic made the horizontal line a bit misleading because it would now be between the next instruction and the one after it. I tried removing the line, but the triangle alone felt a bit too subtle, especially on short lines, so I finally settled for the box. It shouldn't be hard to mark both the last and next line, but I'm not really convinced it would be useful.

Marking breakpoints with a red dot would definitely be more customary. I avoided it for now because it would require a small layout change (some more space left or right of the line numbers), but I'm sure I can get that working. Maybe I could even implement some hover effect to hint at the fact that clicking a line number sets a breakpoint.

Last but not least, toggling breakpoints while the program is running would be absolutely useful, but I wasn't sure how to best implement it. It would somehow require to lock the text area so that line number mouse clicks are still accepted but keypresses or other mouse clicks are ignored.

[rilifon]

No, my intention was not to highlight both the previous and the next instruction at the same time.

Hmm I see. I can understand why those IDEs would highlight in such fashion, but I believe for the purpose of MarvInc trying to be at least somewhat accessible for people new to programming, highlighting only the next line would seem confusing. A think I prefer to stay with the line + arrow indicating current instruction for now, and maybe in the future I'll implement some better indication for what will be the next instruction.

For now lets just focus on your breakpoint feature. If you can, please undo your changes regarding highlight, and leave the breakpoint feature as it is. I can work to improve the toggling during code, and changing the visual aspect. Haven't thought about longer instruction numbers, but I think I can fit a small red circle on the top left corner. The hover effect as you mentioned will prove useful.

Also, I need to remember to include this feature explanation somewhere in the game. Probably some tutorial email.

[yancouto]

Thanks @inguin. I like this feature a lot. Sorry I don't have a lot of time to review and edit this right now.

About highlighting, it's one of those things that there seems to be no perfect solution .(I found myself in sort of the same problems when dealing with cursors in the text editor too.) So any one @rilifon decides is fine by me.

A red circle indicator would be great. Whoever is implementing that, remember to test with two digit lines, as it may overlap.

Lastly, for being able to click on breakpoints during execution, a quick and dirty solution may be to add some behavior before this line (please check if it really makes sense). Possible problems with that solution: it may be possible to toggle breakpoints when a PopUp is showing, or in other situations that should not be possible, but I don't think so. Be also careful with when you're typing to change the name of a register.

[inguin]

@rilifon: You say you want the "current" instruction to be highlighted, but there is no such thing. That's actually the root cause for this discussion. Execution is always paused between instructions; there's either the last one that has been completed, or the one that will be executed next.

For a user trying to follow the program flow the most important question is usually: What will the "robot" do when I hit the Step button? I completed all puzzles with the existing single-stepping feature and found it utterly confusing. My expectation is obviously shaped by my professional experience, but I don't see why a newbie would find the current solution more natural.

@yancouto: Yeah, that's exactly where the change will have to go. I will need to split the mousePressed method into one that checks for line number clicks and one that checks for text area clicks. Popups shouldn't be a problem because they set the TABS_LOCK flag checked three lines above.

What are the cursor issues you're referring to? The only confusion for me was that the selection and the cursor look to similar, so it always seems like the cursor position is part of the selected word. That should be easy to fix by drawing the cursor as a vertical line or hollow box in multi-select mode.

[rilifon]

By current instruction I mean the one that just played. It makes more sense for me and people I've seen playing, but as @yancouto mentioned, there is no perfect solution. I believe the line below the instruction that just occured makes a good reference that is "between" instructions, and I still prefere it instead of your modifications. I believe one solution could be the arrow pointing to a instruction while it is being played, such as 'walk', and when the instruction is over, it goes to the space between instructions before starting the next.

Anyway, this is mostly nitpicking, and I'm sorry it made single-stepping so confusing for you. I hope breakpoints can make debugging less troublesome.

Em Ter, 5 de jun de 2018 02:43, Ingo van Lil notifications@github.com escreveu:

@rilifon https://github.com/rilifon: You say you want the "current" instruction to be highlighted, but there is no such thing. That's actually the root cause for this discussion. Execution is always paused between instructions; there's either the last one that has been completed, or the one that will be executed next.

For a user trying to follow the program flow the most important question is usually: What will the "robot" do when I hit the Step button? I completed all puzzles with the existing single-stepping feature and found it utterly confusing. My expectation is obviously shaped by my professional experience, but I don't see why a newbie would find the current solution more natural.

@yancouto https://github.com/yancouto: Yeah, that's exactly where the change will have to go. I will need to split the mousePressed method into one that checks for line number clicks and one that checks for text area clicks. Popups shouldn't be a problem because they set the TABS_LOCK flag checked three lines above.

What are the cursor issues you're referring to? The only confusion for me was that the selection and the cursor look to similar, so it always seems like the cursor position is part of the selected word. That should be easy to fix by drawing the cursor as a vertical line or hollow box in multi-select mode.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MarvellousSoft/MarvInc/pull/246#issuecomment-394588599, or mute the thread https://github.com/notifications/unsubscribe-auth/ADwvcOMNW8jXxUlUaDIIk2rJZ7mmlNQEks5t5hqEgaJpZM4USLVf .

[inguin]

All done:

1. Dropped the controversial highlighting change.
2. Mark breakpoints with a red circle. I made space for this by removing the colon characters next to the line numbers; they felt a bit redundant with the vertical separator line.
3. Hovering over a line number draws a semi-transparent breakpoint marker.
4. Breakpoints can be toggled while the program is running.

[rilifon]

Ok, so I made some experiments with the line highlight since it really got weird using breakpoints and the way we highlighted lines. For now, it draws a box when the code is running, and switches to a line before a breakpoint. This should make it more clear when debugging what will be the next instruction. This still doesn't work solely on single stepping, but we can improve that when we have a better solution. Tell me what you think!

[inguin]

Not sure, it now feels pretty random when single-stepping. It might work when using only breakpoints in a more or less linear program, but otherwise it's not obvious why there sometimes is a line and sometimes a box,

I see one way to make the existing highlighting work better with breakpoints: A breakpoint could cause program execution to pause after the marked instruction. That way the breakpoint line would always be the one that's highlighted. A bit unconventional, but at least it would be consistent.

[rilifon]

Hmm interesting. But will this make it a bit harder to debug a program? Never used breakpoints like that in an IDE. I'll try to make some changes especifically to single-stepping, using a box when an instruction is running (middle of a walk for instance), and switching to a line when the instruction is over, maybe this will help?

[rilifon]

Actually always drawing like this could solve our dilema. But it could turn jumps confusing? I'll have to test it out.

[rilifon]

That way the breakpoint line would always be the one that's highlighted

Did you see the latest commit where I shifted the line a bit down so it would be in the middle of both lines?

[inguin]

How about this? Went a bit crazy on the Löve drawing primitives:

inguin@1b4330691fa1436f5b41bd2b1f3968cd8c13ccf1

[rilifon]

Thats exactly what I had in mind! Made some small visual adjustments, but merged your commit into dev. Thanks for the work inguin, hopefully this issue is forever closed ^^