Fixes #2913 - remove Disposed views from Responder.instances when Debugging #2914

a-usr · 2023-10-16T08:15:59Z

Fixes #2913 - remove Disposed views from Responder.instances and remove Pos and Dim objects from View when disposing.

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

Please take a look at the code before accepting / rejecting. Just please

Made to assist this solution

tig

Nice. Thank you.

The Responder fix was already in v2, but no the cleaning up of the dim/pos in View.

tig

Unit tests fail tho. Can you fix?

* Ported to v2 * Fixed scneario - config * Lots of goodness * Lots of goodness * Code cleanup * Fixed mouse dragging * Removed Header and margin stuf * Removed Header and margin stuf * Removed Header and margin stuf * Fixed keyboard * upgraded xunit * Action -> EventHandler * Got Range working correctly * Added starter set of unit tests * Cleans up dim/pos objs in dispose. See #2914. * Fixed merge issues; renamed SliderOrientation and added changed event

a-usr · 2023-10-18T14:52:31Z

Unit tests fail tho. Can you fix?

I'll try, but the UnitTest that is failing seems to have no connection to the changes I made. I'll investigate

a-usr · 2023-10-18T15:19:19Z

On a fresh clone of the develop branch the Tests UnitTests/Terminal.Gui.Drivertests/Contents_Copies_From_OS_Clipboard, UnitTests/Terminal.Gui.Drivertests/Contents_Pastes_To_OS_Clipboard and UnitTests/Terminal.Gui.ViewTests/KeyBindings_Command fail, so it seems like the issue isnt caused by the changes i made on my branch. But I will still try to help, although i dont have that much time to work on my "own" projects.

a-usr · 2023-10-18T15:39:08Z

I have created a new Issue for this: #2923

Terminal.Gui/Core/Responder.cs

BDisp · 2023-10-18T20:06:59Z

On a fresh clone of the develop branch the Tests UnitTests/Terminal.Gui.Drivertests/Contents_Copies_From_OS_Clipboard, UnitTests/Terminal.Gui.Drivertests/Contents_Pastes_To_OS_Clipboard and UnitTests/Terminal.Gui.ViewTests/KeyBindings_Command fail, so it seems like the issue isnt caused by the changes i made on my branch. But I will still try to help, although i dont have that much time to work on my "own" projects.

None of this tests fail. Perhaps it's something missing on your development configuration.

V1 miniedit fix 2913

a-usr · 2023-10-19T05:07:23Z

On a fresh clone of the develop branch the Tests UnitTests/Terminal.Gui.Drivertests/Contents_Copies_From_OS_Clipboard, UnitTests/Terminal.Gui.Drivertests/Contents_Pastes_To_OS_Clipboard and UnitTests/Terminal.Gui.ViewTests/KeyBindings_Command fail, so it seems like the issue isnt caused by the changes i made on my branch. But I will still try to help, although i dont have that much time to work on my "own" projects.

None of this tests fail. Perhaps it's something missing on your development configuration.

The clipboard tests failed because I didn't have the ms store version of Powershell installed.

The KeyBindings_Command Test fails because the actuall string is separated differently than the expected string (/ / .).

I don't really have the necessary In-depth knowledge of the the inner workings of Terminal.Gui, but if I had to make an educated guess I'd say the separator is not hard-coded considering on my machine the test fails, but on yours it doesn't.

Maybe It has to do something with the OS?

Again, I don't have had a good enough look at the source code (mainly because it is so much) so I can't really say for sure

BDisp · 2023-10-19T13:00:49Z

The clipboard tests failed because I didn't have the ms store version of Powershell installed.

Install it from here.

The KeyBindings_Command Test fails because the actuall string is separated differently than the expected string (/ / .).

How you are running the tests? From command line, VS2022, VSCode, other? In what OS?

I don't really have the necessary In-depth knowledge of the the inner workings of Terminal.Gui, but if I had to make an educated guess I'd say the separator is not hard-coded considering on my machine the test fails, but on yours it doesn't.

The tests are running with "en-US" encoder, no mater OS localization you have.

Maybe It has to do something with the OS?

I don't think so. Mine is Portuguese, which separator and others formats are different from "En-US", but the tests run well.

Again, I don't have had a good enough look at the source code (mainly because it is so much) so I can't really say for sure

But in this case there is no problem with the code and the unit tests must run without fail. PAY ATENTION you can't copy and paste anything on the computer while the unit tests are running because when the clipboard tests are writing to and reading from clipboard the values may differ.

a-usr · 2023-10-23T06:11:23Z

The KeyBindings_Command Test fails because the actuall string is separated differently than the expected string (/ / .).

How you are running the tests? From command line, VS2022, VSCode, other? In what OS?

From VS2022 in Windows

I don't really have the necessary In-depth knowledge of the the inner workings of Terminal.Gui, but if I had to make an educated guess I'd say the separator is not hard-coded considering on my machine the test fails, but on yours it doesn't.

The tests are running with "en-US" encoder, no mater OS localization you have.

Are you sure? I modified the build workflow to run on my branch, and all tests are passing now.
However on my machine they do not. I suppose xUnit is supposed to force the localization?
My machine has German Localization, wich is why the date is seperated with . on my end.

Im closing #2923 for now, as this seems to be an issue with xUnit not being able to force the localization on some machines, and because the tests run perfectly fine now with your fix,

a-usr · 2023-10-23T06:17:25Z

Also, @tig, you could try to merge now, unittests complete without errors now thanks to @BDisp's fix.

Minor format change to trigger new Unittest execution

BDisp · 2023-10-23T09:49:21Z

Are you sure? I modified the build workflow to run on my branch, and all tests are passing now. However on my machine they do not. I suppose xUnit is supposed to force the localization? My machine has German Localization, wich is why the date is seperated with . on my end.

Im closing #2923 for now, as this seems to be an issue with xUnit not being able to force the localization on some machines, and because the tests run perfectly fine now with your fix,

Why do you not try to run the modified workflow on your machine and if it all pass, commit and push to this PR.

a-usr · 2023-10-23T11:21:32Z

Why do you not try to run the modified workflow on your machine and if it all pass, commit and push to this PR.

as I already said, the one test that actually is failing for me (UnitTests/Terminal.Gui.ViewTests/DateFieldTests/KeyBindings_Command) is likely failing because xUnit for some reason can't force the localization properly. This is either a problem with xUnit itself, or a problem with the custom windows environment on my machine, set by my employer.
The Github Action however is working and doesn't produce any errors. Thats why I already pushed it to this PR.

a-usr · 2023-10-23T11:27:08Z

why do you not try to run the modified workflow on your machine and if it all pass, commit and push to this PR.

Did you maybe think that i modified the workflow to make all tests work? Because i did not. I only enabled a push to my branch to trigger the execution of this workflow, nothing else. I know a merge to the develop branch on my repo would've done the exact same thing, but I am afraid of breaking code that is not mine, especially if i dont understand it completely.

BDisp · 2023-10-23T11:33:23Z

Also, @tig, you could try to merge now, unittests complete without errors now thanks to @BDisp's fix.

@a-usr don't forget to merge my PR https://github.com/a-usr/Terminal.Gui-FORK/pull/4 first. Thanks.

Fixes Pos/Dim static fields not being disposing.

BDisp · 2023-10-23T12:37:12Z

Also, @tig, you could try to merge now, unittests complete without errors now thanks to @BDisp's fix.

@a-usr don't forget to merge my PR a-usr#4 first. Thanks.

I also submit the PR #2929 for the v2.

a-usr · 2023-10-23T12:37:47Z

@a-usr don't forget to merge my PR a-usr#4 first. Thanks.

I actually didnt get a notification for that haha. I merged it now, thanks for your effort!

BDisp

I already review this but only @tig can activate the CI and merge. We need to wait.

UnitTests/Application/RunStateTests.cs

a-usr · 2023-10-23T13:33:08Z

I didnt know that. Tbh this is my first time doing collaborative work over GitHub

tig · 2023-10-25T12:47:24Z

@BDisp I'm worried about taking the removal of the static stuff from Pos/Dim into v1.

Strictly speaking that change is:

not needed to fix this particular issue
a breaking change

I think there's a significant risk of regressions to v1 if we take https://github.com/a-usr/Terminal.Gui-FORK/pull/4 as part of the is PR.

Do you really think we should? I'll defer to you, but it makes me nervous.

BDisp · 2023-10-25T12:56:02Z

Do you really think we should? I'll defer to you, but it makes me nervous.

@tig the fix really work perfectly without any unit tests failures on both versions and there isn't any breaking change. But a static field shouldn't store object instances that already was disposed. Don't you agree?

tig · 2023-10-25T13:02:17Z

Do you really think we should? I'll defer to you, but it makes me nervous.

@tig the fix really work perfectly without any unit tests failures on both versions and there isn't any breaking change. But a static field shouldn't store object instances that already was disposed. Don't you agree?

I see now. I had it in my mind this removed static from public members. But I obv. misunderstood. Thanks!

tig

Nice!

John Züchler added 2 commits October 16, 2023 09:59

fix issue #2913

ce006e3

remove disposed instances from responder.instances (optional)

f38e713

a-usr requested a review from tig as a code owner October 16, 2023 08:15

This was referenced Oct 16, 2023

Fixes #2913. Pos objects preventing Garbage Collection of views #2915

Closed

Pos objects preventing Garbage Collection of views #2913

Closed

a-usr changed the title ~~V1 miniedit~~ Fixes Nothing - remove Disposed views from Responder.instances when Debugging (This change is optional. I don't have any problem with it if you decided to not merge this.) Oct 16, 2023

a-usr changed the title ~~Fixes Nothing - remove Disposed views from Responder.instances when Debugging (This change is optional. I don't have any problem with it if you decided to not merge this.)~~ Fixes Nothing - remove Disposed views from Responder.instances when Debugging Oct 16, 2023

tig approved these changes Oct 17, 2023

View reviewed changes

tig requested changes Oct 17, 2023

View reviewed changes

tig added a commit to tig/Terminal.Gui that referenced this pull request Oct 17, 2023

Cleans up dim/pos objs in dispose. See gui-cs#2914.

f087247

This was referenced Oct 17, 2023

Cleans up dim/pos objects in View.Dispose #2917

Closed

Cleans up dim/pos objs in dispose. See #2914. #2918

Merged

Merge branch 'develop' into v1_miniedit

4a262ad

BDisp reviewed Oct 18, 2023

View reviewed changes

Terminal.Gui/Core/Responder.cs Outdated Show resolved Hide resolved

BDisp added 2 commits October 18, 2023 20:55

Ensures only clear Instances if they really was disposed.

ba9d6cd

Fix unit tests.

a288fdb

Merge pull request #1 from BDisp/v1_miniedit_fix_2913

524d719

V1 miniedit fix 2913

a-usr requested a review from tig October 23, 2023 09:07

Update RunStateTests.cs

eab3f43

Minor format change to trigger new Unittest execution

Fixes Pos/Dim static fields not being disposing.

535f8a7

Merge pull request #4 from BDisp/v1_miniedit-pos-dim-fix_2913

a304496

Fixes Pos/Dim static fields not being disposing.

a-usr changed the title ~~Fixes Nothing - remove Disposed views from Responder.instances when Debugging~~ Fixes #2913 - remove Disposed views from Responder.instances when Debugging Oct 23, 2023

a-usr requested a review from BDisp October 23, 2023 12:50

BDisp suggested changes Oct 23, 2023

View reviewed changes

UnitTests/Application/RunStateTests.cs Show resolved Hide resolved

tig approved these changes Oct 25, 2023

View reviewed changes

tig merged commit a8d2f8d into gui-cs:develop Oct 25, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2913 - remove Disposed views from Responder.instances when Debugging #2914

Fixes #2913 - remove Disposed views from Responder.instances when Debugging #2914

a-usr commented Oct 16, 2023 •

edited

Loading

tig left a comment

tig left a comment

a-usr commented Oct 18, 2023

a-usr commented Oct 18, 2023

a-usr commented Oct 18, 2023 •

edited

Loading

BDisp commented Oct 18, 2023

a-usr commented Oct 19, 2023

BDisp commented Oct 19, 2023

a-usr commented Oct 23, 2023

a-usr commented Oct 23, 2023

BDisp commented Oct 23, 2023

a-usr commented Oct 23, 2023 •

edited

Loading

a-usr commented Oct 23, 2023

BDisp commented Oct 23, 2023

BDisp commented Oct 23, 2023

a-usr commented Oct 23, 2023

BDisp left a comment

a-usr commented Oct 23, 2023

tig commented Oct 25, 2023

BDisp commented Oct 25, 2023

tig commented Oct 25, 2023

tig left a comment

Fixes #2913 - remove Disposed views from Responder.instances when Debugging #2914

Fixes #2913 - remove Disposed views from Responder.instances when Debugging #2914

Conversation

a-usr commented Oct 16, 2023 • edited Loading

Pull Request checklist:

tig left a comment

Choose a reason for hiding this comment

tig left a comment

Choose a reason for hiding this comment

a-usr commented Oct 18, 2023

a-usr commented Oct 18, 2023

a-usr commented Oct 18, 2023 • edited Loading

BDisp commented Oct 18, 2023

a-usr commented Oct 19, 2023

BDisp commented Oct 19, 2023

a-usr commented Oct 23, 2023

a-usr commented Oct 23, 2023

BDisp commented Oct 23, 2023

a-usr commented Oct 23, 2023 • edited Loading

a-usr commented Oct 23, 2023

BDisp commented Oct 23, 2023

BDisp commented Oct 23, 2023

a-usr commented Oct 23, 2023

BDisp left a comment

Choose a reason for hiding this comment

a-usr commented Oct 23, 2023

tig commented Oct 25, 2023

BDisp commented Oct 25, 2023

tig commented Oct 25, 2023

tig left a comment

Choose a reason for hiding this comment

a-usr commented Oct 16, 2023 •

edited

Loading

a-usr commented Oct 18, 2023 •

edited

Loading

a-usr commented Oct 23, 2023 •

edited

Loading