Skip to content

Fix crash on subsequent song loads #7051

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.

Sign up for GitHub

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

Merged
michaelgregorius merged 1 commit into from
Jan 7, 2024
Merged

Fix crash on subsequent song loads #7051

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/gui/MixerView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,11 @@ void MixerView::refreshDisplay()
// delete all views and re-add them
for (int i = 1; i<m_mixerChannelViews.size(); ++i)
{
chLayout->removeWidget(m_mixerChannelViews[i]);
m_racksLayout->removeWidget(m_mixerChannelViews[i]->m_effectRackView);
auto * mixerChannelView = m_mixerChannelViews[i];
chLayout->removeWidget(mixerChannelView);
m_racksLayout->removeWidget(mixerChannelView->m_effectRackView);

delete mixerChannelView;
Comment on lines +209 to +213
Copy link
Contributor

@sakertooth sakertooth Jan 7, 2024

Choose a reason for hiding this comment

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

I believe the recommendation is to use deleteLater instead?

Copy link
Contributor Author

@michaelgregorius michaelgregorius Jan 7, 2024

Choose a reason for hiding this comment

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

I think deleteLater should not always be used but rather in situations where it is needed (some scenarios are listed here). In fact, I think it might even lead to complications when it is used in a context where a direct deletion works as well.

Calling delete will immediately unlink the MixerChannelView from it's parent and delete the instance. The link between the parent and the child likely was what lead to the problems where the Fader was repainted without a model. So if we'd call deleteLater instead of delete an event for later deletion will be put into the event queue. If there's an event in the queue that will be handled before the deletion event and which leads to a repaint then we'd still run into problem. This is not the case here but it shows that deleteLater might also lead to problems.

Copy link
Contributor

@sakertooth sakertooth Jan 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The link between the parent and the child likely was what lead to the problems where the Fader was repainted without a model.

Ideally, the code should be using some RAII mechanism at the highest level for the project tree. Reloading the project would imply deletion of some old Project object with a new one, so child objects are not left alive and bugs like this can never happen, but for now this works.

Edit: But for the function refreshDisplay it would make sense to actually delete the object.

}
m_channelAreaWidget->adjustSize();

Expand Down