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

Display error message in status bar when execution failed #6918

Merged
merged 5 commits into from Jun 9, 2023

Conversation

vitvakatu
Copy link
Contributor

@vitvakatu vitvakatu commented Jun 1, 2023

Pull Request Description

Closes #6859

2023-06-01.13-38-50.mp4

Latest state:

Screenshot 2023-06-02 at 12 34 23

Important Notes

  • Is current message to the user correct? Do we need to say something more?

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

Comment on lines 31 to 32
/// Text that shows up in the statusbar when backend reports a failed execution.
pub const EXECUTION_FAILED_MESSAGE: &str = "Execution failed. Please try restarting IDE.";
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also display the message received from the engine, making debugging user problems easier.

The question is if the messages are not too long.

Copy link
Contributor

Choose a reason for hiding this comment

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

"Please report this problem at support@enso.org" as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The message from the engine seems to be too long. I added the email address as suggested

@wdanilo
Copy link
Member

wdanilo commented Jun 1, 2023

  1. Why what you entered in the searcher kills engine? It should not kill it. Is it a special mode for testing it?
  2. When the execution failed, after restarting the IDE, is the project re-opened with the current state or user is loosing their work? If the latter, we need to fix it somehow (probably outside of this PR).
  3. Can we have a button there, like? "Critical backend error, click here to restart Enso." ?
  4. After clicking it, before restarting, there should be a pop-up asking if you want to report the error with optional text field for expalanation what happened, as @hubertp suggested. However, such reporting has to be automatic, so after clicking "report and restart" the error should be reported automatically. Again, this might be out of scope of this PR.

@farmaazon
Copy link
Contributor

farmaazon commented Jun 2, 2023

  1. I think Ilya is triggering bug meta1 = meta1 triggers silent NPE #6821
  2. Failed execution should not prevent us from sending update, so no code is lost, perhaps the cached values only.
    1. Let's do small steps first. I would merge this (with an email address as @hubertp suggested) and make an epic issue for improving this further (and not only this but any potential bug, like panics as well).

@vitvakatu
Copy link
Contributor Author

  1. Correct, I just asked Hubert how I can trigger execution failure for testing.
  2. In this particular case, the state of the project is preserved (and the execution fails again). I think there might be cases when we don't save the latest changes, though. It needs to be checked.

@wdanilo
Copy link
Member

wdanilo commented Jun 5, 2023

@farmaazon
2. Ok, no code is lost, but you need to restart Enso, so the current project is not getting saved then, right? So in the end, after restarting Enso, you'd lose some part of your work, am I correct?

@farmaazon
Copy link
Contributor

farmaazon commented Jun 5, 2023

you need to restart Enso, so the current project is not getting saved

I don't understand this inference. You need to restart Enso because execution is failing for some reason. What does it have to do with saving projects?

Of course, it may happen if the engine is thoroughly broken, but this is not because of execution fail.

@farmaazon
Copy link
Contributor

QA report 🍌

The message is displayed as it should. However, when playing with it, I realized there are some ways of improvement:

  1. Having the new dashboard, there probably is no need for restarting IDE, but just the project: user could go back to dashboard, click "stop" butting and run project again. It also would make more sense in cloud (where closing the IDE does not stop the engine, so restarting it may be pointless).
  2. If the restarting Project would work, I think there are high odds that restarting the execution may help (just clicking play button).
  3. If the failure is due to failing code (as tested in this PR), the user might make it working again (or just undo changes). So I think I'll be safe to hide "Execution failed" message upon successful execution. But I'm not sure what notification would help us - the executionComplete seems to be sent always, also after failing executions. You may ask @4e6 for guidance.

From the above points, I think we could change the second sentence to "Please try restarting project or IDE and..." or - if we would implement the point 3 - "Please try to re-execute graph, restart project or IDE..."

@4e6
Copy link
Contributor

4e6 commented Jun 7, 2023

safe to hide "Execution failed" message upon successful execution

Currently, the engine sends executionComplete when the execution is finished, and executionFailed in case of failures. We can change the logic to indicate success & failure outcomes with these two messages. In other words, executionFailed will be sent in cases of failures (as before) and executionComplete otherwise. I.e. the IDE will receive only one of the two messages when the program execution finishes.

@vitvakatu
Copy link
Contributor Author

I adjusted the text on the label as suggested and created additional issues for improvements, @farmaazon please confirm if we can merge it now.

@wdanilo
Copy link
Member

wdanilo commented Jun 9, 2023

image

I started to wonder what is banana-like approval :P

@farmaazon
Copy link
Contributor

I started to wonder what is banana-like approval :P

It's a variation of 🟡 (not red, but also not entirely green).

@wdanilo
Copy link
Member

wdanilo commented Jun 9, 2023

I started to wonder what is banana-like approval :P

It's a variation of 🟡 (not red, but also not entirely green).

I know! It just made me laugh, that was a nice touch :D

@vitvakatu vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Jun 9, 2023
@mergify mergify bot merged commit 03d725b into develop Jun 9, 2023
25 checks passed
@mergify mergify bot deleted the wip/vitvakatu/execution-failed branch June 9, 2023 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let the user know that the engine has entered a 'bad state'
5 participants