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

🪲 Improve long program warning for sleep programs and increase time #5576

Merged
merged 12 commits into from
May 28, 2024

Conversation

Felienne
Copy link
Member

Fixes #5399

In #5277 we introduced a sleep prompt, but accidentally we now now longer detect sleeps in levels below 7, causing them to quickly stop (annoying!) Maybe that was even the reason people were reporting #5399 but I upped the limits a bit too.

@Felienne
Copy link
Member Author

Hi @Annelein!

I think I made the frontend fix correctly but I am not sure, can you check and approve if this makes sense (and improve otherwise 🤣 )

@Felienne Felienne changed the title Improve long program 🪲 Improve long program warning for sleep programs and increase time May 28, 2024
response['variables'] = transpile_result.roles_of_variables
except Exception:
pass

if level < 7:
Copy link
Member Author

@Felienne Felienne May 28, 2024

Choose a reason for hiding this comment

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

This if needs to go in any case, because otherwise has_sleep will never be set on programs above 7, but I am not sure what to do with the rest.

It is a bit of magic why we need to send the list of sleep lengths to the frontend. If needed, I can probably get them from the parser, but you don't always know upfront how long they will be (it a variable is involved)

I am not even sure we need all of this complex code for the sleep prompt?

I think we could do it a lot easier in the Skulpt execution? Because I am sure the interpreter knows when to sleep?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I prefer to ship this as we have it now, and ask Jesus (in due time) to solve this in the interpreter, because this is annoying people.

@Annelein
Copy link
Collaborator

@Felienne I think it looks good! But maybe because I did not know of a better/easier way to do this 😅, I don't think I am the most experienced for this PR? So maybe best if @jpelay has a look as well?

@Felienne
Copy link
Member Author

@Felienne I think it looks good! But maybe because I did not know of a better/easier way to do this 😅, I don't think I am the most experienced for this PR? So maybe best if @jpelay has a look as well?

Yeah I figured that! it was smartly done working around the parser like that!

If will you are ok with merging this, I will re-open the sleep prompt one and we will ask Jesus to do it with Skulpt!

@Annelein Annelein self-requested a review May 28, 2024 16:50
@Annelein Annelein merged commit 901d4a0 into main May 28, 2024
10 checks passed
@Annelein Annelein deleted the improve-long-program branch May 28, 2024 16:50
Copy link
Contributor

mergify bot commented May 28, 2024

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

🪲 Programs get flagged as 'too long' too quickly
2 participants