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: ICommadSource Implemetation #15496

Conversation

workgroupengineering
Copy link
Contributor

@workgroupengineering workgroupengineering commented Apr 24, 2024

What does the pull request do?

Ensure CanExecute and Execute consistence.

What is the current behavior?

The current implementation of ICommandSource does not guarantee that CanExecute and Execute occur on the same command.

What is the updated/expected behavior with this PR?

How was the solution implemented (if it's not obvious)?

  • CanExecuteChangedHandler caching to avoid multiple allocation of delegates at subscribe/unsubscribe
  • Copy Commad and CommandParameter references to local variables before calling CanExecute and Execute to ensure consistency

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0047659-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@robloo
Copy link
Contributor

robloo commented Apr 24, 2024

Note that whatever is decided here also must be done to SplitButton. SplitButton was based on Button code in this area. (It doesn't inherit from Button since WinUI doesn't do that either. Although it was debated we left that like upstream at least for 11.0).

Copy link
Member

@maxkatz6 maxkatz6 left a comment

Choose a reason for hiding this comment

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

These changes make sense.

If we want to define this as an expected behavior, it should also be unit tested (i.e., changing Command inside of CanExecute handler or something obscure like that).

Also, SplitButton, yes.

@maxkatz6 maxkatz6 added the bug label Apr 25, 2024
@workgroupengineering workgroupengineering marked this pull request as draft April 25, 2024 11:02
@jmacato
Copy link
Member

jmacato commented Apr 25, 2024

@cla-avalonia recheck

@cla-avalonia
Copy link
Collaborator

cla-avalonia commented Apr 25, 2024

  • All contributors have signed the CLA.

@workgroupengineering
Copy link
Contributor Author

@cla-avalonia agree

@workgroupengineering workgroupengineering force-pushed the fixes/Controls/ICommadSource_Implemetation branch 2 times, most recently from b3b7533 to 99af069 Compare April 26, 2024 10:40
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0047790-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@workgroupengineering workgroupengineering force-pushed the fixes/Controls/ICommadSource_Implemetation branch from 5e8b374 to fe7c841 Compare April 26, 2024 12:12
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0047796-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@workgroupengineering workgroupengineering marked this pull request as ready for review April 26, 2024 13:07
@maxkatz6 maxkatz6 added the backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch label Apr 30, 2024
@maxkatz6 maxkatz6 merged commit 81d2e7d into AvaloniaUI:master Apr 30, 2024
11 checks passed
@workgroupengineering workgroupengineering deleted the fixes/Controls/ICommadSource_Implemetation branch April 30, 2024 06:11
maxkatz6 pushed a commit that referenced this pull request May 8, 2024
* test: CommandParameter does not change between CanExecute and Execute

* feat: CommandParameter does not change between CanExecute and Execute

* test: update
@maxkatz6 maxkatz6 added backported-11.1.x and removed backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels May 8, 2024
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.

6 participants