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

Fix for issue #13493 #13518

Merged
merged 5 commits into from
Nov 19, 2023
Merged

Fix for issue #13493 #13518

merged 5 commits into from
Nov 19, 2023

Conversation

laolarou726
Copy link
Contributor

@laolarou726 laolarou726 commented Nov 7, 2023

What does the pull request do?

This PR fixed an issue mentioned in #13493
For function UpdateContent(bool withTransition), this PR ensures that the Updated content is not in the last presented ContentPresenter, otherwise it will raise the exception mentioned in #13493

What is the current behavior?

Currently, in some cases, the new content is the content that TCC just presented. An exception will be raised.

What is the updated/expected behavior with this PR?

Now there will be no such issue.

Checklist

Fixed issues

@maxkatz6
Copy link
Member

maxkatz6 commented Nov 7, 2023

Can you add a test for this scenario? In TransitioningContentControlTests file.
This PR also broken some other tests there.

@laolarou726
Copy link
Contributor Author

Can you add a test for this scenario? In TransitioningContentControlTests file. This PR also broken some other tests there.

I just fixed and now all the tests are passed. Also, I added one more test to verify the check is working. Could you please review it again? Thx!

@laolarou726
Copy link
Contributor Author

Can any one help me to confirm that this PR is able to merge into the master? Thx.

@maxkatz6 maxkatz6 requested a review from grokys November 8, 2023 04:31
Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

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

Hi @laolarou726 - thanks for the PR! However looking at the test, it seems to pass even without the modified code in TransitioningContentControl. In addition when I run your repro (https://github.com/laolarou726/TCCExceptionTest) I don't see any exception there either.

The fix looks reasonable but I'd like to be able to confirm the problem first and I'm not sure why I'm unable to. Does the unit test fail for you if you remove the changes to TransitioningContentControl?

@laolarou726
Copy link
Contributor Author

Hi @laolarou726 - thanks for the PR! However looking at the test, it seems to pass even without the modified code in TransitioningContentControl. In addition when I run your repro (https://github.com/laolarou726/TCCExceptionTest) I don't see any exception there either.

The fix looks reasonable but I'd like to be able to confirm the problem first and I'm not sure why I'm unable to. Does the unit test fail for you if you remove the changes to TransitioningContentControl?

Ohh sorry for the confusion, in the repo I replace the transitioningcontentcontrol to the fixed implementation "TCC". If you change all the TCC back to the TransitioningContentControl, the exception will occur. I'll just update the repo now.

@laolarou726
Copy link
Contributor Author

@grokys I just updated the repro, and now the exception will shown. Please click the "Start" and wait for a few seconds. Then the exception will be thrown. https://github.com/laolarou726/TCCExceptionTest.

For the test, do you think it's ok to just remove it?

@laolarou726
Copy link
Contributor Author

laolarou726 commented Nov 9, 2023

Hi @laolarou726 - thanks for the PR! However looking at the test, it seems to pass even without the modified code in TransitioningContentControl. In addition when I run your repro (https://github.com/laolarou726/TCCExceptionTest) I don't see any exception there either.

The fix looks reasonable but I'd like to be able to confirm the problem first and I'm not sure why I'm unable to. Does the unit test fail for you if you remove the changes to TransitioningContentControl?

@grokys Could you please check the repo again? Thx!

@laolarou726
Copy link
Contributor Author

@grokys Could you please review the repro again? Thanks!

@maxkatz6 maxkatz6 merged commit c6c12ce into AvaloniaUI:master Nov 19, 2023
6 checks passed
@maxkatz6 maxkatz6 added the backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch label Nov 28, 2023
maxkatz6 pushed a commit that referenced this pull request Dec 5, 2023
* Update OpenGlControlBase.cs

* Update TransitioningContentControl.cs

* bug fix and add test
@maxkatz6 maxkatz6 added backported-11.0.x and removed backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch labels Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TransitioningContentControl issue
3 participants