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.

Sign up for GitHub

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

Jump to bottom

Add evaluation status bar item #480

Closed
wants to merge 1 commit into from
Closed

Add evaluation status bar item #480

wants to merge 1 commit into from

Conversation

stefan-toubia
Copy link
Contributor

What has Changed?

  • Add a new status bar item for evaluation status

I've been bitten more than a few times because I missed the fact that I encountered an evaluation error, either because the bottom of my output channel was not visible or the output panel was collapsed. This adds a simple status indicator to make it idiot-proof.

No evaluation

image

Evaluating (animated)

image image image

Success

image

Error

image

Action

Clicking the icon will open the output panel. I couldn't get it to do more than this, I didn't put too much effort into it, it would be nice to make it toggle and ensure that the Calva Says output is shown.

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Made sure I am directing this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I am changed the default PR base branch, so that it is not master. (Sorry for the nagging.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.) You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test. (For now you'll need to opt in to the CircleCI New Experience UI to see the Artifacts tab, because bug.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • Referenced the issue I am fixing/addressing in a commit message for the pull request.
    • If I am fixing the issue, I have used GitHub's fixes/closes syntax
    • If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
  • Created the issue I am fixing/addressing, if it was not present.
  • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

  • Made sure the PR is directed at the dev branch (unless reasons).
  • Read the source changes.
  • Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @cfehse

stefan-toubia
stefan-toubia commented Nov 22, 2019
View reviewed changes
src/evaluate.ts
chan.appendLine("No results from file evaluation.");
}
}).catch((e) => {
chan.appendLine(`Evaluation of file ${fileName} failed: ${e}`);
setEvalStatus(EvaluationStatus.error);
});
}
if (callback) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was recently added, I'm not familiar with the purpose. Would it make sense for this to also be represented in the status bar indicator?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this might be from @cfehse 's fix for the load file callback. And yes, would probably be material for a status indicator.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose was solely to show that an error occurred instead of ignoring the error condition completely.

@PEZ
Copy link
Collaborator

PEZ commented Nov 22, 2019

I think this PR addresses a big and important issue with Calva. We should really do this. However, I don't think adding more status bar items is the way to do it. How about we instead:

  • use the clj indicator
  • Make it red when there are errors
    • Put the error message in the hover.
    • Add an action to it that shows the stack trace in the REPL window.
  • animate its color during evaluation, say pulsate between grey and white (if possible)

This indicator sometimes is a button already. For toggling cljc repl between clj and cljs. So for the stack trace action we will need to show a menu offering both actions. I think we should then change it so that it always shows a menu. We can move the actions for opening the REPL windows there.

@stefan-toubia
Copy link
Contributor Author

stefan-toubia commented Nov 22, 2019 •
edited
Loading

@PEZ fair enough about the status bar items, there are enough of them already. I like the plan of using the clj indicator. This should be a quick change over, I will do some refactoring to the status bar update function to clean this up a bit.

The status bar doesn't seem to support rapid updates, the most you can get is ~200ms updates, so no ability to really 'pulse`, I'll test some options out.

Add an action to it that shows the stack trace in the REPL window.

I think what would make sense here is to implement this as a separate PR after #471 is finished, which implements some changes to outputting messages and stacktrace to the REPL window, which should make this very straightforward to implement.

@PEZ
Copy link
Collaborator

PEZ commented Nov 22, 2019

Add an action to it that shows the stack trace in the REPL window.

I think what would make sense here is to implement this as a separate PR after #471 is finished, which implements some changes to outputting messages and stacktrace to the REPL window, which should make this very straightforward to implement.

Makes sense. It was that PR that got me to think about it. Let's just have the error message in the tooltip then for now?

This was referenced Nov 24, 2019
Open
Open
@stefan-toubia
Copy link
Contributor Author

I'm closing this PR since it is stale, will re-open a new one when I have made more progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfehse cfehse cfehse left review comments

@PEZ PEZ PEZ left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stefan-toubia @PEZ @cfehse