Code Quality: Use SidebarContext and HomePageContext in OpenIn... RichCommands

[0x5bfa]

Summary

PR Checklist

• [ ] Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers. Closes #issue
• [ ] Did you build the app and test your changes?
• [ ] Did you check for accessibility? You can use Accessibility Insights for this.
• [ ] Did you remove any strings from the en-us resource file?
  • [ ] Did you search the solution to see if the string is still being used?
• [ ] Did you implement any design changes to an existing feature?
  • [ ] Was this change approved?
• [ ] Are there any other steps that were used to validate these changes? None

Screenshots

None

[0x5bfa]

@hishitetsu Does tho look good to you and do you happen to have another idea to concern?

[hishitetsu]

A reference to the IsExecutable property should not change any state, nor should it be assumed that the IsExecutable property is referenced prior to the ExecuteAsync call. Is there any inconvenience with the current implementation?

[0x5bfa]

Should call IsExecutable in the first line of ExecuteAsync?

Idk but it looks odd to me somewhat though it's my idea lol.

[hishitetsu]

It would be better to do the same conditional branching in ExecuteAsync as in GetIsExecutable. But in the first place, is there a need to use SidebarContext or HomePageContext here?

[0x5bfa]

I'm gonna use RichCommand for sidebar and widget card right click items to reduce duplications.

[0x5bfa]

It would be better to do the same conditional branching in ExecuteAsync as in GetIsExecutable.

But in the first place, is there a need to use SidebarContext or HomePageContext here?

In that case, calling GetIsExecutable before the actual execution in ExecuteAsync wouldn't work?

[hishitetsu]

Actions should be stateless. In other words, they shouldn't have any fields which change the process depending on the value. Control of processing should always be by reference to external properties.

[0x5bfa]

You are telling that there’s no guarantee of GetIsExecutable being called at all right?

As you may know we have discussion about Actions with multi-state. Why the actions should be stateless? ContentPageContext is already state and if user is viewing widgets page, ContentPageContext values are null. This means we determine the state already.

In this case, I think calling GetIsExecutable and returning the context type (also remove the field) at the first line of ExecuteAsync is the best idea. Besides, this ExecutableContextType is just getting executable context type literally determining whether values of each context are null or not.

Also, we are going to make ExecuteAsync() to be ExecuteAsync(object? parameter)

[hishitetsu]

You are telling that there’s no guarantee of GetIsExecutable being called at all right?

Yes.

Why the actions should be stateless?

More precisely, actions may refer to external contexts, but may not manage the state itself. Defining _executableContextType is not a good idea. The state when IsExecutable is referenced does not necessarily continue when ExecuteAsync is called, and a race condition may occur when the action is called from multiple locations at the same time. Therefore, actions should always refer to the latest state from external contexts.

Also, we are going to make ExecuteAsync() to be ExecuteAsync(object? parameter)

I'm not against it.

[0x5bfa]

I understand the point, then calling GetIsExecutable in ExecuteAsync and returning executable context might be a solution to this.