Skip to content

Fixes #2354. View.Redraw doesn't clear itself and PositionCursor doesn't ensure focus when a prior view was disabled.#2355

Merged
tig merged 3 commits intogui-cs:developfrom
BDisp:IgnoreHasFocusPropertyOnRedraw-property_2354
Feb 21, 2023
Merged

Fixes #2354. View.Redraw doesn't clear itself and PositionCursor doesn't ensure focus when a prior view was disabled.#2355
tig merged 3 commits intogui-cs:developfrom
BDisp:IgnoreHasFocusPropertyOnRedraw-property_2354

Conversation

@BDisp
Copy link
Copy Markdown
Collaborator

@BDisp BDisp commented Feb 19, 2023

Fixes #2354 - A simple view now can be used as container with normal and focus color if IgnoreHasFocusPropertyOnRedraw is false (default) or only with the normal color if it's true. Now if it's possible to focus a enable view this is ensured, otherwise the cursor will be invisible.

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

imagem

@tig
Copy link
Copy Markdown
Collaborator

tig commented Feb 20, 2023

Do we have a real user that has requested this?

In my opinion we should be very leery of adding new features (methods/properties) to v1 that are not critically needed.

Everything we add to v1 now becomes more legacy we need to support/port in v2.

I would much rather get focused on v2 for things like this.

However, if there is a real customer (developer external to this project) who is requesting this, and can't get their app to work without it, then I'll change my mind.

@BDisp
Copy link
Copy Markdown
Collaborator Author

BDisp commented Feb 20, 2023

Do we have a real user that has requested this?

We may not need the IgnoreHasFocusPropertyOnRedraw which is a new feature, but I think you didn't yet seen that this PR fixes three bugs:

  • Fixes the View.Redraw that doesn't clear itself before redraw his subviews.
  • Fixes the Enabled property if is true of a view that is being added to the superview that has the Enabled as false by not also setting the subview to false.
  • Fixes the PositionCursor where the cursor isn't placed on the available view that can focus, is visible and is enabled when a previous view that was previously focused and his Enabled property is set to false.

The GetFocusColor method is a helper to get the Focus or the Disabled color.

In my opinion we should be very leery of adding new features (methods/properties) to v1 that are not critically needed.

I agree. The only thing I can remove here is the IgnoreHasFocusPropertyOnRedraw property. The rest I think is useful for both versions.

Everything we add to v1 now becomes more legacy we need to support/port in v2.

I agree. Do you want I do remove the IgnoreHasFocusPropertyOnRedraw property, then?

I would much rather get focused on v2 for things like this.

I agree with the cases of adding new features, but not for bug fixes before the v2 is published.

However, if there is a real customer (developer external to this project) who is requesting this, and can't get their app to work without it, then I'll change my mind.

No there is no customer, that was only me that I detected some bugs and when I was fixing I thinking in a view to force the view to always using the normal color whether it's focused or not, if the IgnoreHasFocusPropertyOnRedraw property is true.

@BDisp BDisp force-pushed the IgnoreHasFocusPropertyOnRedraw-property_2354 branch from 6ff0049 to 3ccc782 Compare February 20, 2023 13:02
@BDisp BDisp changed the title Fixes #2354. View should have a IgnoreHasFocusPropertyOnRedraw property to prevent unnecessary derived class. Fixes #2354. View.Redraw doesn't clear itself and PositionCursor doesn't ensure focus when a prior view was disabled. Feb 20, 2023
@tig
Copy link
Copy Markdown
Collaborator

tig commented Feb 20, 2023

Do we have a real user that has requested this?

We may not need the IgnoreHasFocusPropertyOnRedraw which is a new feature, but I think you didn't yet seen that this PR fixes three bugs:

  • Fixes the View.Redraw that doesn't clear itself before redraw his subviews.
  • Fixes the Enabled property if is true of a view that is being added to the superview that has the Enabled as false by not also setting the subview to false.
  • Fixes the PositionCursor where the cursor isn't placed on the available view that can focus, is visible and is enabled when a previous view that was previously focused and his Enabled property is set to false.

I know it seems like more work (and I know I'm guilty of not always doing this right myself), but these should really each be documented as individual Issues and submitted as separate PRs.

For v1 develop (I think we can operate slightly more loosey-goosy in the v2 branch), I am wondering if we should start being super-hardcore about this. I'd be curious to know your reaction to me stating that.

I agree. Do you want I do remove the IgnoreHasFocusPropertyOnRedraw property, then?

Yes.

@BDisp
Copy link
Copy Markdown
Collaborator Author

BDisp commented Feb 20, 2023

I know it seems like more work (and I know I'm guilty of not always doing this right myself), but these should really each be documented as individual Issues and submitted as separate PRs.

Yes you are right. Sometimes I'm so lazy, sorry :-)

For v1 develop (I think we can operate slightly more loosey-goosy in the v2 branch), I am wondering if we should start being super-hardcore about this. I'd be curious to know your reaction to me stating that.

Yes I'm starting working more in the v2 and I propose that you move the View2 code to the View, so we can only be focusing in the v2. But I think that all the v1 bugs that are transversal to the v2 must be merge in the v1 and all the v2 PR must be merged with the develop branch before they are submitted, otherwise there will be a bunch of merge conflicts to resolve.

I agree. Do you want I do remove the IgnoreHasFocusPropertyOnRedraw property, then?

Yes.
I already removed and changed the issue and PR's titles.

@tig
Copy link
Copy Markdown
Collaborator

tig commented Feb 21, 2023

I know it seems like more work (and I know I'm guilty of not always doing this right myself), but these should really each be documented as individual Issues and submitted as separate PRs.

Yes you are right. Sometimes I'm so lazy, sorry :-)

No need to apologize. We're all in this for fun (but I think it's fun to sometimes pretend this all matters ;-).

For v1 develop (I think we can operate slightly more loosey-goosy in the v2 branch), I am wondering if we should start being super-hardcore about this. I'd be curious to know your reaction to me stating that.

Yes I'm starting working more in the v2 and I propose that you move the View2 code to the View, so we can only be focusing in the v2. But I think that all the v1 bugs that are transversal to the v2 must be merge in the v1 and all the v2 PR must be merged with the develop branch before they are submitted, otherwise there will be a bunch of merge conflicts to resolve.

I'm going to do this asap. As part of it, my plan is to remove a bunch of existing functionality that becomes redundant.

In some cases, I plan on focusing on v2 and then back-porting to v1. I think that either way we'll have some merge issues to deal with. Not fun.

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.

View.Redraw doesn't clear itself and PositionCursor doesn't ensure focus when a prior view was disabled.

2 participants