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

Engine should send notification about node status #3729

Merged
merged 23 commits into from
Sep 28, 2022

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Sep 22, 2022

Pull Request Description

When nodes get invalidated in the cache, they have to be recomputed. Let the IDE know which of the nodes are pending by sending Api.ExpressionUpdate.Payload.Pending message.

Important Notes

This PR introduces new Api.ExpressionUpdate.Payload.Pending message. This message is delivered before re-computation of nodes. Later Api.ExpressionUpdate.Payload.Value or other is sent to notify the IDE that a value for given node is available.

Trivial implementation of of the Api.ExpressionUpdate.Payload.Pending message in the IDE is provided by this PR to (improperly) visualize pending node status - further improvements needed in follow up PRs.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@JaroslavTulach JaroslavTulach self-assigned this Sep 22, 2022
@JaroslavTulach JaroslavTulach requested a review from 4e6 as a code owner September 22, 2022 06:47
@JaroslavTulach JaroslavTulach marked this pull request as draft September 22, 2022 06:47
@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/NodeStatusNotifications_182818735 branch from 285dd16 to 2c7652d Compare September 26, 2022 09:45
@JaroslavTulach JaroslavTulach marked this pull request as ready for review September 26, 2022 09:51
@JaroslavTulach
Copy link
Member Author

I still need to write some unit tests for the new message, but otherwise I believe this PR is ready for review. Thanks in advance for your opinions.

Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

Checked the rust part.

app/gui/src/presenter/graph/state.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@4e6 4e6 left a comment

Choose a reason for hiding this comment

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

Looks ok. Things that can be improved here:

  • sending message with a list of pending IDs would be more efficient, than a bunch of messages for each ID
  • The type and methodCall of the new ExpressionUpdate message are always null (another reason for sending pending IDs in a batch). It should be at least documented in the protocol description

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Sep 28, 2022
@mergify mergify bot merged commit 835ac05 into develop Sep 28, 2022
@mergify mergify bot deleted the wip/jtulach/NodeStatusNotifications_182818735 branch September 28, 2022 12:35
@unfurl-links unfurl-links bot mentioned this pull request May 18, 2023
2 tasks
hubertp added a commit that referenced this pull request May 18, 2023
The change adds an additional field to `ExpressionUpdates` messages sent
by `ProgramExecutionSupport` to indicate if the type of value (or its
method pointer) has changed and therefore would potentially require a
suggestions' update.

Prior to #3729 that check was done during the instrumentation. However
we still want to continue to support "pending expression" functionality
therefore `SuggestionsHandler` will use the additional information to
filter only the required expression updates.

Most of the changes are related to adapting our tests to the new field.

Closes #6707.
mergify bot pushed a commit that referenced this pull request May 18, 2023
The change adds an additional field to `ExpressionUpdates` messages sent by `ProgramExecutionSupport` to indicate if the type of value (or its method pointer) has changed and therefore would potentially require a suggestions' update.

Prior to #3729 that check was done during the instrumentation. However we still want to continue to support "pending expression" functionality therefore `SuggestionsHandler` will use the additional information to filter only the required expression updates.

Most of the changes are related to adapting our tests to the new field.

Closes #6706.

# Important Notes
The associated project now loads and navigates smoothly.
Also attaching a screenshot from the project that illustrates that pending functionality continues to work:
[Kazam_screencast_00006.webm](https://github.com/enso-org/enso/assets/292128/35918841-f84f-4e1c-b1b0-40e45d97e111)
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.

5 participants