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

Ensure visualizer operator override #1889

Merged
merged 7 commits into from
Jul 10, 2024
Merged

Conversation

glopesdev
Copy link
Member

@glopesdev glopesdev commented Jul 6, 2024

The implementation in #1751 introduced a regression whereby Visualizer operators found themselves overruled by the new propagation semantics for Sink. This happened because Visualizer inherits its internal Process method from the Sink base class, and therefore the following check succeeded for both Sink and Visualizer since DeclaringType would be Sink in both cases:

methodCall.Method.DeclaringType == typeof(Reactive.Sink)) &&

This PR introduces a new internal virtual method to allow the Process method to be redeclared in derived classes. This allows Visualizer to "override" the method with a new static function which just calls the same base class implementation but which now will cause a change in the DeclaringType of the method. The above check will now fail for Visualizer types, which restores the behavior of the operator.

Due to the subtle nature of visualizer propagation semantics and the likelihood of further internal implementation reviews, we also added regression tests for the issue, together with a refactoring of the testing framework towards a more flexible combinator-based API for building workflow graphs (this could almost go into its own PR as it can in principle be used for everything, but I prefer to leave it close to its original motivation and we can see where it goes from there).

Fixes #1860

@glopesdev glopesdev added the fix Pull request that fixes an issue label Jul 6, 2024
@glopesdev glopesdev added this to the 2.8.4 milestone Jul 6, 2024
Copy link
Member

@PathogenDavid PathogenDavid left a comment

Choose a reason for hiding this comment

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

Implementation looks good, new tests catch intentionally regressing either issue as appropriate. Had some comments on the tests and new test infrastructure.

I didn't verify in-editor because lazy I assume Bruno will double-check it solves things on his end 😁

Bonsai.Core.Tests/InspectBuilderTests.cs Show resolved Hide resolved
Bonsai.Core.Tests/InspectBuilderTests.cs Outdated Show resolved Hide resolved
Bonsai.Core.Tests/InspectBuilderTests.cs Outdated Show resolved Hide resolved
Bonsai.Core.Tests/InspectBuilderTests.cs Outdated Show resolved Hide resolved
Bonsai.Core.Tests/Workflow.cs Outdated Show resolved Hide resolved
Bonsai.Core.Tests/Workflow.cs Outdated Show resolved Hide resolved
Bonsai.Core.Tests/Workflow.cs Outdated Show resolved Hide resolved
Bonsai.Core.Tests/Workflow.cs Outdated Show resolved Hide resolved
Bonsai.Core.Tests/Workflow.cs Outdated Show resolved Hide resolved
Bonsai.Core.Tests/Workflow.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@bruno-f-cruz bruno-f-cruz left a comment

Choose a reason for hiding this comment

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

Working as intended on my end! Thanks

Copy link
Member

@PathogenDavid PathogenDavid left a comment

Choose a reason for hiding this comment

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

:shipit:

@glopesdev glopesdev merged commit 9a6e884 into bonsai-rx:main Jul 10, 2024
9 checks passed
@glopesdev glopesdev deleted the issue-1860 branch July 10, 2024 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Pull request that fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visualizer operator does not propagate internal workflow visualizer
3 participants