fix: keep macro prompt open for events older than 100

[ammmze]

Description

Previously we were only looking for a series of prompt action events within the last 100 events. However, if things are happening in the background while the prompt is still open, those events can keep racking up and eventually the prompt window will close (though klipperscreen still displays it).

I've now refactored this code so that it will internally keep an array of the prompt events and we will process through all the events at first, but we will set a checkpoint at the most recent processed event so that we don't go back and re-process ones that were already done. Effectively this means on first load sure we iterated through potentially a lot of events, but then after that we're really only processing through at most a few events. It is certainly less concise than the previous implementation, but I believe it is a more robust solution and perhaps a bit more performant. Previously every time we asked for this.macroPromptEvents it would iterate through the last 100 messages doing its filters and map. And that would get called numerous times in the scope of a single render. With this new process each time we call it (aside from the first load), it typically will only have at most a few items to filter through.

In addition, the part of the code that would parse out the message, because it would replace all double quotes, it meant that I could not do this:

RESPOND TYPE=command MSG="action:prompt_begin \"Question of the day\""
RESPOND TYPE=command MSG="action:prompt_button Foo Bar|RESPOND MSG=\"FOO BAR\""
RESPOND TYPE=command MSG="action:prompt_show"

If I tried to click that button I would get the following error:

Malformed command 'RESPOND MSG=FOO BAR'

I've updated the message parsing to effectively just strip off matching open and closing single or double quotes from the message. For example, all of the following would result in a message of Question of the day:

RESPOND TYPE=command MSG="action:prompt_begin \"Question of the day\""
RESPOND TYPE=command MSG="action:prompt_begin 'Question of the day'"
RESPOND TYPE=command MSG="action:prompt_begin Question of the day"

Here's a quick couple macros that demonstrate the bug. Execute the MAINSAIL_BUG_TEST macro and then click the button to Send 100. This will then execute 100 respond macros and the prompt dialog in mainsail will close without the user intending to close it.

[gcode_macro MAINSAIL_BUG_TEST]
gcode:
  RESPOND TYPE=command MSG="action:prompt_begin Question of the day"
  RESPOND TYPE=command MSG="action:prompt_button Send 100|MAINSAIL_BUG_TEST_MSG LOOP=100"
  RESPOND TYPE=command MSG="action:prompt_button Send 10|MAINSAIL_BUG_TEST_MSG LOOP=10"
  RESPOND TYPE=command MSG="action:prompt_button Send 1|MAINSAIL_BUG_TEST_MSG LOOP=1"
  RESPOND TYPE=command MSG="action:prompt_show"

[gcode_macro MAINSAIL_BUG_TEST_MSG]
gcode:
  {% set loop  = params.LOOP|default(10)|int %}
  {% for i in range(loop) %}
    RESPOND MSG="Iteration {i}"
  {% endfor %}

Related Tickets & Documents

None found

Mobile & Desktop Screenshots/Recordings

[optional] Are there any post-deployment tasks we need to perform?

[meteyou]

To be honest, while displaying a Macro prompt, there should not be any traffic in the console, because klippy should be in pause. i have to double-check your code how performant it works, because "the last 100 entries" was just a performance thing...

[ammmze]

To be honest, while displaying a Macro prompt, there should not be any traffic in the console, because klippy should be in pause. i have to double-check your code how performant it works, because "the last 100 entries" was just a performance thing...

Thank you for taking the time to review. While my example macro prompt was pretty simplified, I would counter and say that there are perfectly valid ways that klippy would not be paused while a prompt is displayed. Another example, FLAP (haven't used it, but just came across it), will trigger prompts for things like runout, and insert events. Once those prompts are displayed, klippy technically is not paused. They can then have those buttons run whatever macros they want and choose whether or not the prompt should close after the button is pressed. And to further explain what I am working on where I encountered this is in Happy Hare v3, i'm working on an interactive calibration procress that will walk the user through a series of prompts that are driven from a separate thread, which makes it easier to collect context along the way and make the the workflow more dynamic. Preview of that work can be found here.

Perhaps a compromise would be to add the slice back in, but with a higher value (5-10k). I just think 100 too little. That way there is a defined cap to the number of events processed on initial load. But I would still suggest keeping the rest of this refactor because it will reduce the number of events we iterate over in each render significantly.

[pedrolamas]

I just noticed this PR, the only comment I have is why should we truncate the double quotes? If you don't want to use them, then don't add it?

[ammmze]

I just noticed this PR, the only comment I have is why should we truncate the double quotes? If you don't want to use them, then don't add it?

Thanks for the question! The quotes are needed in the command in order to properly group the value and assign it to the parameter. For example, if I omit the quotes and try to send the following:

RESPOND TYPE=command MSG=action:prompt_button Foo Bar|RESPOND MSG=FOO

I receive the following error:

Malformed command 'RESPOND TYPE=command MSG=action:prompt_button Foo Bar|RESPOND MSG=FOO'

And similarly if I omit the quotes in the "nested" command like this:

RESPOND TYPE=command MSG="action:prompt_button Foo Bar|RESPOND MSG=FOO BAR"

I get the error:

Malformed command 'RESPOND MSG=FOO BAR'

And FWIW we stripped the quotes before this PR, the difference is previously we stripped all the quotes in the command, which made it so nested commands would lose their quotes. The change I made allows the nested commands to keep the quotes.

[pedrolamas]

Thanks for the reply @ammmze , but I was refering to this bit here:

I've updated the message parsing to effectively just strip off matching open and closing single or double quotes from the message. For example, all of the following would result in a message of Question of the day:

Why strip the quotes here? If the user added quotes, then we should use them and show them.

I'm not saying that this was working correctly before, but I just don't see the point in truncating these quotes on this specific spot.

[pedrolamas]

Thanks for the reply @ammmze , but I was refering to this bit here:

I've updated the message parsing to effectively just strip off matching open and closing single or double quotes from the message. For example, all of the following would result in a message of Question of the day:

What strip the quotes here? If the user added quotes, then we should use them and show them.

I just don't see the point in truncating these quotes on this specific spot.

RESPOND TYPE=command MSG="action:prompt_begin \"Question of the day\""
RESPOND TYPE=command MSG="action:prompt_begin 'Question of the day'"

To me this should show ”Question of the day" and 'Question of the day' respectively, just as the user put it (as the quotes here are superfluous as we are using | as separator)

[ammmze]

I just don't see the point in truncating these quotes on this specific spot.

RESPOND TYPE=command MSG="action:prompt_begin \"Question of the day\""
RESPOND TYPE=command MSG="action:prompt_begin 'Question of the day'"

To me this should show ”Question of the day" and 'Question of the day' respectively, just as the user put it (as the quotes here are superfluous as we are using | as separator)

Oh right, that part. Yea that makes sense. We were already stripping the double quotes, so I figured it was intended. But now that you bring it up, I also looked at KlipperScreen. My screen must not be big enough or something, so it doesn't display the title for me, but looking at it's code and observing the logs, they are NOT stripping those outer quotes. So I agree with you that we should leave those quotes in there to be consistent with KlipperScreen. I will work on an update to make that behavior consistent and open a new PR in the next few days. Thank you for bringing this up!

[pedrolamas]

No problem! It also makes it consistent with Fluidd, hence why I mentioned this!

[meteyou]

I have just read up on your conversation and I think it only makes sense if we now fix it in Mainsail like Fluidd + KlipperScreen does, so that all GUIs are simply consistent.

Thank you @ammmze for making a PR for this!