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

Update Remove/RemoveRange/RemoveAll calls in line with framework changes #19971

Merged
merged 11 commits into from
Sep 3, 2022

Conversation

peppy
Copy link
Member

@peppy peppy commented Aug 26, 2022

@peppy peppy force-pushed the remove-dispose-updates branch from afaf6ca to edab4ee Compare August 26, 2022 06:48
@peppy peppy force-pushed the remove-dispose-updates branch from 1d41485 to 105aa01 Compare August 29, 2022 06:57
smoogipoo
smoogipoo previously approved these changes Sep 1, 2022
Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

Looks alright, super hard to review though.

frenzibyte
frenzibyte previously approved these changes Sep 1, 2022
@frenzibyte
Copy link
Member

Test failures seem relevant, CheckAudioQualityTest.getMockWorkingBeatmap would probably have to be adjusted since Track now requires an argument on constructor.

@frenzibyte
Copy link
Member

frenzibyte commented Sep 3, 2022

Turns out UserProfileOverlay recreates the sections container when switching between users (to handle hiding certain sections on some profiles), and removes the header before disposing the old sections container to reuse it in the new container:

if (sectionsContainer != null)
sectionsContainer.ExpandableHeader = null;

Add(sectionsContainer = new ProfileSectionsContainer
{
ExpandableHeader = Header,
FixedHeader = tabs,
HeaderBackground = new Box
{
// this is only visible as the ProfileTabControl background
Colour = ColourProvider.Background5,
RelativeSizeAxes = Axes.Both
},
});

I've went ahead and removed disposing from both ExpandableHeader and Footer for simplicity.


In addition, I've also updated one inconsistent case in ScrollingTeamContainer, since both adding team and clearing teams should dispose all ScrollingTeams safely anyways.

@frenzibyte frenzibyte enabled auto-merge September 3, 2022 13:14
@frenzibyte frenzibyte merged commit cbc3627 into ppy:master Sep 3, 2022
@peppy peppy deleted the remove-dispose-updates branch September 5, 2022 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants