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

Fixes #2789. StatusItem should have a disabled attribute if it can't execute. #2790

Merged
merged 3 commits into from
Aug 11, 2023

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Aug 6, 2023

Fixes #2789 - Adding CanExecute property allow leveraging the power of conditional executions.

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@BDisp BDisp requested a review from tig as a code owner August 6, 2023 22:53
@tig
Copy link
Collaborator

tig commented Aug 7, 2023

Why is this targetting v1? We agreed we will not add new capabilities to v1, but only v2.

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 7, 2023

Why is this targetting v1? We agreed we will not add new capabilities to v1, but only v2.

@tig I consider this a compatible change for v1 with no breaking change and I'll also submit a PR targeting to the v2 as well. The first release for v2 will take still a long time and we need to keep v1 pleasant as possible. I really hope you understand my observation.

@tig
Copy link
Collaborator

tig commented Aug 7, 2023

Why is this targetting v1? We agreed we will not add new capabilities to v1, but only v2.

@tig I consider this a compatible change for v1 with no breaking change and I'll also submit a PR targeting to the v2 as well. The first release for v2 will take still a long time and we need to keep v1 pleasant as possible. I really hope you understand my observation.

But we agreed we were not adding anything new to v1. Every bit of focus we put into v1 delays v2.

I think we're pretty close to getting v2 done. We just need to focus on it!

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 7, 2023

But we agreed we were not adding anything new to v1. Every bit of focus we put into v1 delays v2.

I think we're pretty close to getting v2 done. We just need to focus on it!

@tig we agree not adding new features but not nothing about improving the existing one. I'm focused on v2 but actually I felling some like blocked without know where to go for now. What I did on StatusItem was something that was omitted before. I think constantly breaking only some improvements that do not compromise the existing code at all, it is not healthy to continue to keep any enthusiasm to really collaborate. Sorry but that's what I really feel. Please tell me what the code contained in this PR, breaks any compatibility with the code that the user currently uses. If you show me that this is the case I immediately close this PR. Also, as we know better v1, these improvements help us also implement v2 with more refined improvements. I think we should keep users as satisfied as well as possible with v1 to keep them and are more anxious for the official release of v2.

@tig
Copy link
Collaborator

tig commented Aug 7, 2023

I'm focused on v2 but actually I felling some like blocked without know where to go for now.

The Project has tons of things that need working on. https://github.com/orgs/gui-cs/projects/1/views/1

image

I think we should keep users as satisfied as well as possible with v1 to keep them and are more anxious for the official release of v2.

I do not see where a real customer has asked for this functionality. Did I miss that?

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 7, 2023

I do not see where a real customer has asked for this functionality. Did I miss that?

I'm a contributor. Doesn't I haven't that right as well, for god sake. No more comments. Finished.

Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

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

Please also submit a v2 PR for this. Thanks.

@BDisp
Copy link
Collaborator Author

BDisp commented Aug 11, 2023

Please also submit a v2 PR for this. Thanks.

Rest assured. All the PR's you ask or asked to submit to v2 I will do it. Don't forget to merge this one, please :-)

@tig tig merged commit 74aedd2 into gui-cs:develop Aug 11, 2023
1 check passed
@BDisp BDisp deleted the v1_statusitem-can-execute-fix_2789 branch August 11, 2023 18:53
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.

StatusItem should have a disabled attribute if it can't execute.
2 participants