Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add debug state to papyros editor #5366

Merged
merged 6 commits into from
Feb 21, 2024
Merged

Add debug state to papyros editor #5366

merged 6 commits into from
Feb 21, 2024

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Feb 16, 2024

This pull request adds a debug state to the papyros editor.

related papyros pr: dodona-edu/papyros#629

When a debug run is started, the editor becomes readonly.
All visualization of generated output, input used and active line is now based on the shown frame instead of the current phase of execution.
While the debug mode is active, the buttons to start a new execution are replaced by an end debug mode button.
Peek 2024-02-16 10-30

@jorg-vr jorg-vr added the enhancement A change that isn't substantial enough to be called a feature label Feb 16, 2024
@jorg-vr jorg-vr self-assigned this Feb 16, 2024
@jorg-vr jorg-vr added the deploy naos Request a deployment on naos label Feb 16, 2024
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Feb 16, 2024
@jorg-vr jorg-vr marked this pull request as ready for review February 16, 2024 10:26
@jorg-vr jorg-vr requested a review from a team as a code owner February 16, 2024 10:26
@jorg-vr jorg-vr requested review from bmesuere and niknetniko and removed request for a team February 16, 2024 10:26
Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nitpick: there are some unused imports now in coding_scratchpad.

@bmesuere bmesuere added the deploy naos Request a deployment on naos label Feb 17, 2024
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Feb 17, 2024
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, this solves a few problems we had before 👍

  • I was a little confused by the editor state which was read-only (expected by me while debugging), but this was not shown visually. In fact, you can click the code and a caret even appears which hints that you can edit, but you can't.

  • edit: another minor remark: If generating traces is fast (which it often is), the buttons flicker quite a bit between the original, stop and stop debugging. Is there a reason to go to the run state first?

Two smaller remarks not really related to the PR:

  • The gutter while debugging is quite big due to the large arrow tail and margin between arrow and line numbers. Maybe we can find a better arrow.
  • When clicking on debug, the debugger opens but it is not super clear where you need to click to start the debugging process. We do explain this in the info box at step 0, but even I always have to think for a second. Maybe we can tweak the style of the next step (>) button (make it primary? make it primary only at step 0?)

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Feb 19, 2024

The gutter while debugging is quite big due to the large arrow tail and margin between arrow and line numbers. Maybe we can find a better arrow.

In debug mode the lint gutter is now replaced by the debug gutter. This way the width doesn't increase. I also used an mdi icon instead of the font arrow so it should now look the same on all devices.

If generating traces is fast (which it often is), the buttons flicker quite a bit between the original, stop and stop debugging. Is there a reason to go to the run state first?

I am not exactly sure this is what you meant, but I changed it so the 'stop' button is shown while the state is 'loading' instead of the run/debug buttons. This mostly makes a difference the first time the debugger is opened (As it is in loading while papyros is fetched).

For fast, but not instant programs there might still be a flickering stop button appearing, before the 'stop debugging' shows up. But this is harder to avoid as I can't predict execution will be that fast.

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Feb 20, 2024

For fast, but not instant programs there might still be a flickering stop button appearing, before the 'stop debugging' shows up. But this is harder to avoid as I can't predict execution will be that fast.

@bmesuere I have added a 100ms delay on button changes which mostly solves the issue.

Now it is still possible to have a flicker of exactly 100ms of the last open button state. (I added the delay on the general button change function, thus it also happens when changing to 'stop debugger')
But as the buttons are showed at least one 100ms, the flicker feels less odd. Also most often the previous button will either be the 'stop debugger' button (If the debugger wasn't explicitly shut down before opening another case) or the run buttons. (But these are also blue so the flicker is less intrusive)

@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Feb 20, 2024
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Feb 20, 2024
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌
1 last nitpick, but I don't know if it's possible: the cursor is still the "text" cursor over the disabled input area

@jorg-vr jorg-vr merged commit e8626db into main Feb 21, 2024
13 checks passed
@jorg-vr jorg-vr deleted the enhance/improve-debug-state branch February 21, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A change that isn't substantial enough to be called a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants