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

Honor changing view controller type. #7814

Merged
merged 10 commits into from
May 23, 2023

Conversation

hkfb
Copy link
Contributor

@hkfb hkfb commented Apr 4, 2023

Background

When replacing existing Views with changed Controller types, if the new Views happen to have the same ids, then the new Controller types are not honored, and instead the old, possibly incompatible Controllers are used instead.

Change List

  • Create new Controller if type has changed

When replacing existing Views with changed Controller types, if
the new Views happen to have the same ids, then the new Controller
types are not honored, and instead the old, possibly incompatible
Controllers are used instead.
Copy link
Collaborator

@felixpalmer felixpalmer left a comment

Choose a reason for hiding this comment

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

This doesn't check if the view has changed, as the original TODO mentions. Could you also add that? Please also add some tests that demonstrate that this works

@hkfb
Copy link
Contributor Author

hkfb commented Apr 5, 2023

This doesn't check if the view has changed, as the original TODO mentions. Could you also add that? Please also add some tests that demonstrate that this works

I think it's the view / controller combination that is the intention of the TODO comment. The change of the View is handled elsewhere.

I added a test that demonstrates the issue. It fails without the changes in this PR.

@hkfb hkfb requested a review from felixpalmer April 6, 2023 06:33
@hkfb
Copy link
Contributor Author

hkfb commented Apr 12, 2023

@felixpalmer would you mind taking another look?

@coveralls
Copy link

Coverage Status

Coverage: 89.831% (+0.0005%) from 89.83% when pulling 3237e90 on hkfb:invalidate_controller into 96ce058 on visgl:master.

@felixpalmer felixpalmer merged commit 98c1677 into visgl:master May 23, 2023
2 checks passed
@hkfb hkfb deleted the invalidate_controller branch May 23, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants