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

[Sprite Lab] Workspace alert for execution errors #45055

Merged
merged 3 commits into from
Mar 1, 2022

Conversation

mikeharv
Copy link
Contributor

Context and summary: If a behavior or function is undefined in a student's Blockly code, calling it results in an execution error. There is no user-facing console in Sprite Lab so all feedback is hidden. Because execution errors cause Sprite Lab to crash, these issues are often misdiagnosed by users as "disappearing sprites". Ideally, we want to prevent student programs from getting into this kind of state. In the meantime, we can alert the user if they do get into this state.
This change utilizes WorkspaceAlert which we also use across various labs for other purposes, such as when warning the student about code changes while the program is running.

Function Example:
image
Behavior Example:
image
Animation to show non-immediate error:
2022-02-25 15-46-50 2022-02-25 15_48_11

Links

Testing story

This change would not be used in Game Lab, because there is a Debug Console already available:
image

This change would be easy to add to Poetry, but is likely not needed. Behaviors are only selectable by drop-down and cannot be edited or deleted. Functions are also not available to students.
image

Deployment strategy

Follow-up work

STAR-121: [Sprite Lab] Prevent broken behaviors from causing a crash

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@mikeharv mikeharv requested review from KylieModen and a team February 25, 2022 20:58
@mikeharv mikeharv requested a review from a team as a code owner February 25, 2022 20:58
Copy link

@epeach epeach left a comment

Choose a reason for hiding this comment

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

This looks good! Do we want to cover this with a test in SpriteLabTest.js?

* If there is an executionError, alert the user. In Sprite Lab there is
* no user-facing console, so we instead create a WorkspaceAlert.
*/
this.alertStudent(this.JSInterpreter.executionError?.message);
Copy link
Contributor

Choose a reason for hiding this comment

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

naming nit: this function makes an assumption about what it should do ("if i hit an execution error, alert the student"). there are a few problems with this assumption:

  • the user won't always be a student
  • changing how we react to execution errors would require changing the same of this function (what if, in the future, instead of alerting, we decide to log to the debug console?)
  • this code shouldn't really care how the error is handled, but rather that consumers who do care can handle it however they want

@@ -89,6 +91,22 @@ export default class SpriteLab extends P5Lab {
}
}

alertStudent(msg) {
if (msg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: always prefer returning early to reduce nesting (which improves readability) over wrapping all functionality in a conditional

if (!msg) {
  return;
}

// now everything else can be at this indentation level


/**
* If there is an executionError, alert the user. In Sprite Lab there is
* no user-facing console, so we instead create a WorkspaceAlert.
Copy link
Contributor

Choose a reason for hiding this comment

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

this context is useful, but it should probably live in SpriteLab.js

Copy link
Contributor

@KylieModen KylieModen left a comment

Choose a reason for hiding this comment

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

LGTM - workspace alert looks great, ahead of ship let's notify the curriculum team of the change in their slack channel

Copy link
Contributor

@maddiedierker maddiedierker left a comment

Choose a reason for hiding this comment

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

🎉 great work, mike -- diving into the JSInterpreter code was really complicated and you did a great job navigating it!

@mikeharv mikeharv merged commit dcb7eb2 into staging Mar 1, 2022
@mikeharv mikeharv deleted the alert-for-undefined-functions branch March 1, 2022 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants