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

MVVM Toolkit - Make Command classes open for extension #3610

Conversation

thomasclaudiushuber
Copy link

Fixes

This PR fixes Issue #3609 that helps to make command classes extendable, which is needed to use existing command logic with WPF's CommandManager.

This change also helps to move issue #3571 forward.

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

Currently command classes are sealed and cannot be extended

What is the new behavior?

Command classes can be extended

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

@ghost
Copy link

ghost commented Dec 7, 2020

Thanks thomasclaudiushuber for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@thomasclaudiushuber thomasclaudiushuber changed the title Make Command classes open for extension MVVM Toolkit - Make Command classes open for extension Dec 7, 2020
Comment on lines 16 to +23
/// Notifies that the <see cref="ICommand.CanExecute"/> property has changed.
/// </summary>
void NotifyCanExecuteChanged();

/// <summary>
/// Gets a value indicating whether this command can always be executed. True if the command was created without execution status logic.
/// </summary>
bool CanAlwaysExecute { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Notifies that the <see cref="ICommand.CanExecute"/> property has changed.
/// </summary>
void NotifyCanExecuteChanged();
/// <summary>
/// Gets a value indicating whether this command can always be executed. True if the command was created without execution status logic.
/// </summary>
bool CanAlwaysExecute { get; }
/// Gets a value indicating whether this command can always be executed. True if the command was created without execution status logic.
/// </summary>
bool CanAlwaysExecute { get; }
/// <summary>
/// Notifies that the <see cref="ICommand.CanExecute"/> property has changed.
/// </summary>
void NotifyCanExecuteChanged();

Put property above method? I know StyleCop enforces this for classes, but I guess not for interfaces?

Choose a reason for hiding this comment

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

Yes, you're absolutely right. Should be like suggested by you if we would merge it. Thanks @michael-hawker

@michael-hawker
Copy link
Member

Thanks for jumping in here @thomasclaudiushuber and investigating this WPF world for us.

Is this PR ready to review or is the discussion in #3609 with @Sergio0694 still moving forward? Otherwise should this be a draft PR to help facilitate discussion?

@thomasclaudiushuber
Copy link
Author

Sorry @michael-hawker, saw your comment here just yet. Yes, the PR was meant to support the discussion in the issue #3609. I think after we have discussed all the points, we agreed that we don't make the command classes sealed, because the only reason we can think of why we should do that is the WPF CommandManager. So, in that case, a Command re-implementation for WPF might be the better idea.

Thank you.

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.

[Feature] MVVM Toolkit - Remove 'sealed' from the four Command Classes to allow extension
2 participants