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

Revert #4808 fix. Due to #12591 and investigations. #12729

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

kirsan31
Copy link
Contributor

@kirsan31 kirsan31 commented Jan 8, 2025

Fix #12591.
This revert #4808 fix commit from #11358 PR.

The clean up of DisplayedItems after removal has already been implemented in DoLayoutIfHandleCreated but only if handle is created:

protected internal virtual void OnItemRemoved(ToolStripItemEventArgs e)
{
// clear cached item states.
OnItemVisibleChanged(e, performLayout: true);
((ToolStripItemEventHandler?)Events[s_eventItemRemoved])?.Invoke(this, e);
}

if (performLayout)
{
DoLayoutIfHandleCreated(e);
}

So we have leak only if handle is destroyed (#4808 case) and we didn't take this into account :(
All are much more complicated than originally intended. And lead to some side affect see this comment.

I think that is safer to revert the fix and reopen #4808 then.

Microsoft Reviewers: Open in CodeFlow

@kirsan31 kirsan31 requested a review from a team as a code owner January 8, 2025 07:50
@kirsan31 kirsan31 mentioned this pull request Jan 8, 2025
@kirsan31
Copy link
Contributor Author

kirsan31 commented Jan 8, 2025

Can #4808 be opened for comments? I want to add some for the possible future editors...

Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.05114%. Comparing base (aaac41d) to head (87a65cb).
Report is 22 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (aaac41d) and HEAD (87a65cb). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (aaac41d) HEAD (87a65cb)
Debug 3 2
test 1 0
Additional details and impacted files
@@                 Coverage Diff                  @@
##                main      #12729          +/-   ##
====================================================
- Coverage   76.03780%   50.05114%   -25.98667%     
====================================================
  Files           3181        1996        -1185     
  Lines         639670      284531      -355139     
  Branches       47215       41781        -5434     
====================================================
- Hits          486391      142411      -343980     
+ Misses        149759      139411       -10348     
+ Partials        3520        2709         -811     
Flag Coverage Δ
Debug 50.05114% <100.00000%> (-25.98667%) ⬇️
integration 18.16600% <100.00000%> (-0.00781%) ⬇️
production 50.05114% <100.00000%> (+0.21861%) ⬆️
test ?
unit 47.26128% <100.00000%> (+0.20606%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@Tanya-Solyanik Tanya-Solyanik merged commit cd5dd1c into dotnet:main Jan 8, 2025
8 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0 Preview1 milestone Jan 8, 2025
ricardobossan pushed a commit to ricardobossan/winforms that referenced this pull request Jan 9, 2025
…et#12729)

Fix dotnet#12591.
This reverts dotnet#4808 fix commit from dotnet#11358 PR.

The disposal of DisplayedItems after removal has already been implemented in DoLayoutIfHandleCreated but only if handle was created. So we have a leak only if handle is destroyed (dotnet#4808 case) and we didn't take this into account.
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.

Undo operation in DemoConsole causes ContextMenuStrip to become unusable
2 participants