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

DlgPrefEffects: fix preset removed from list when canceling delete #4470

Merged
merged 1 commit into from
Oct 22, 2021

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Oct 22, 2021

@github-actions github-actions bot added the ui label Oct 22, 2021
@ronso0
Copy link
Member

ronso0 commented Oct 22, 2021

Now there's no dialog anymore? Well...we could consider that bug fixed ;)

@Be-ing
Copy link
Contributor Author

Be-ing commented Oct 22, 2021

Huh?

@Holzhaus
Copy link
Member

Now there's no dialog anymore? Well...we could consider that bug fixed ;)

I didn't test this, but looking at the diff I think nothing changed w.r.t. showing a dialog. Can you elaborate?

@ronso0
Copy link
Member

ronso0 commented Oct 22, 2021

Neither do I.

What I did:

  • select a chain I just imported
  • hit Delete
  • nothing happens, except the chain is deselected

Rebasing onto #4467 doesn't fix it.

m_pChainPresetManager->deletePreset(selectedPresetName);
if (m_pChainPresetManager->deletePreset(selectedPresetName)) {
focusedChainStringList.removeAll(selectedPresetName);
}
}

This comment was marked as outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oof... good catch

Comment on lines 268 to 292
if (!unfocusedChainStringList.contains(selectedPresetName)) {
m_pChainPresetManager->deletePreset(selectedPresetName);
if (m_pChainPresetManager->deletePreset(selectedPresetName)) {
focusedChainStringList.removeAll(selectedPresetName);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

else branch missing?
what happens when the preset is in both lists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the missing else branch

@ronso0
Copy link
Member

ronso0 commented Oct 22, 2021

LGTM, waiting for the green lights

@Swiftb0y Swiftb0y merged commit 3a5fac3 into mixxxdj:effects_refactoring Oct 22, 2021
@Be-ing Be-ing deleted the preset_delete branch November 13, 2021 22:53
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.

4 participants