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 animation behavior hot reload #2288

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

BretJohnson
Copy link
Contributor

Description of Change

Fix https://developercommunity.visualstudio.com/t/Hot-reload-stops-working-when-using-Comm/10707756

This is a follow up to an earlier attempt to fix this issue with #2174, which we decided not to take.

See that PR for more context. Here's the update since then:

Ideally, this would be fixed on the MAUI tooling side, making the VS Hot Reload code respect the
special rules around readonly OneWayToSource properties (which aren't supported in WPF but
are in MAUI). Evgeny and I discussed fixing there & started to scope, but we ran out of time to
get a fix in for VS 17.12 before it froze (and before we both went on vacation). I'm not sure
we'll be able to get a fix in for 17.13 either.

Given all that, it seems best to go back to applying this workaround on the CommunityToolkit side
to make Hot Reload work - with a public setter, it works. Updating the CommunityToolkit
also has the advantage that it works in all VS versions, including older ones.

I added a comment to the code to make it clearer that apps shouldn't use the setter directly, instead supplying a binding - the setter is only needed as a workaround for Hot Reload. @bijington - please review text of that comment.

Also, earlier we thought that making the setter internal may work, with an InternalsVisibleTo. But Evgeny clarified that

Internal will not work. InternalsVisibleTo should include target assembly. For example, If you edit MyPage.xaml which is part of MyApp then CommynityToolkit should have [assembly, InternalsVisibleTo("MyApp")]. Which is clearly impossible

So internal in a no go & public with comment seems like the best option to get this working with XAML Hot Reload.

Linked Issues

PR Checklist

Additional information

Fix https://developercommunity.visualstudio.com/t/Hot-reload-stops-working-when-using-Comm/10707756

Control properties should have both a getter and setter
so that Hot Reload (and C# code) can update them.
AnimateCommand was missing the setter - likely just
an oversight. Hot Reload now works with this change.
@bijington
Copy link
Contributor

Thanks for the follow up @BretJohnson I have just tried marking the setter [Obsolete] in the hope it will make it clearer to a developer. Let's see how it builds :)

Copy link
Contributor

@bijington bijington left a comment

Choose a reason for hiding this comment

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

Two more quick tests

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.

2 participants