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

PathConfigPane: Prevent an invalid index assert #5604

Merged
merged 1 commit into from Jun 14, 2017

Conversation

sepalani
Copy link
Contributor

This PR prevents the following assert to be triggered:

---------------------------
wxWidgets Debug Alert
---------------------------
..\..\src\common\ctrlsub.cpp(111): assert ""pos < GetCount()"" failed in wxItemContainer::Delete(): invalid index
Do you want to stop the program?
You can also choose [Cancel] to suppress further warnings.

Ready to be reviewed & merged.

@leoetlino
Copy link
Member

Shouldn't the button just be disabled in that case?

@sepalani
Copy link
Contributor Author

@shuffle2
Copy link
Contributor

Maybe the need for this changed at some point and it's screwing things up now?

@sepalani
Copy link
Contributor Author

@shuffle2
Even if it isn't needed anymore, it's probably not what provoked this. It can only be triggered after the assert. So in best case scenario it does more work for nothing, worse one there is something wrong in OnISOPathSelectionChanged.

The only visual clue I have is the fact that the remove button is "blinking" when I do something that is triggering this function such as adding/removing paths. It seems it got disabled and then re-enabled somehow.

@shuffle2
Copy link
Contributor

Oh, I didn't look which function that code was in, thought it was init code :')

@shuffle2
Copy link
Contributor

https://github.com/dolphin-emu/dolphin/blob/master/Source/Core/DolphinWX/Config/PathConfigPane.cpp#L147
This is what is causing the "Remove" button to be enabled (I guess the enable is applied in a hierarchical way, so it overrides the setting put on the Remove wxButton itself).
I think it was a regression from here: bfa9cc2#diff-e369a4db42acf1ce10d24b5289031b4aR148

@shuffle2
Copy link
Contributor

this seems to fix it:

...
  Bind(wxEVT_UPDATE_UI, &PathConfigPane::OnEnableIfCoreNotRunning, this);
}

void PathConfigPane::OnEnableIfCoreNotRunning(wxUpdateUIEvent& event)
{
  if (event.GetId() != m_remove_iso_path_button->GetId())
    WxEventUtils::OnEnableIfCoreNotRunning(event);
}

@shuffle2
Copy link
Contributor

lgtm


void PathConfigPane::OnEnableIfCoreNotRunning(wxUpdateUIEvent& event)
{
// Prevent the Remove button to be enabled via wxUpdateUIEvent

This comment was marked as off-topic.

This comment was marked as off-topic.

@leoetlino leoetlino merged commit 40a9e58 into dolphin-emu:master Jun 14, 2017
@sepalani sepalani deleted the wx-rm-path branch June 14, 2017 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants