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

Move LogWindow/LogConfigWindow destructor logic -> OnClose #4231

Merged

Conversation

ligfx
Copy link
Contributor

@ligfx ligfx commented Sep 21, 2016

Fixes the issue on macOS where quitting Dolphin from the Dock causes a crash report. I'm not exactly sure why this works, but it feels 🌟right🌟 and it fixes the problem.


This change is Reviewable

@ligfx ligfx changed the title Move LogWindow and LogConfigWindow destructors into OnClose handlers Move LogWindow and LogConfigWindow destructor logic into OnClose handlers Sep 21, 2016
@ligfx ligfx force-pushed the fix_sigabrt_when_quitting_from_dock branch from 9cc306a to 2891cc0 Compare September 21, 2016 01:25
@lioncash
Copy link
Member

Source/Core/DolphinWX/LogWindow.cpp, line 157 at r1 (raw file):

  event.Skip();

  for (int i = 0; i < LogTypes::NUMBER_OF_LOGS; ++i)

This should be in its own function (RemoveAllListeners or something appropriate), which is called in OnClose()


Comments from Reviewable

@ligfx
Copy link
Contributor Author

ligfx commented Sep 21, 2016

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Source/Core/DolphinWX/LogWindow.cpp, line 157 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

This should be in its own function (RemoveAllListeners or something appropriate), which is called in OnClose()

Done.

Comments from Reviewable

@endrift
Copy link
Contributor

endrift commented Sep 21, 2016

Whoa a fix for this crash, finally! I've seen this crash a handful of times but was never sure what caused it.

@lioncash
Copy link
Member

LGTM once the merge commit is rebased out.

@ligfx ligfx force-pushed the fix_sigabrt_when_quitting_from_dock branch from 195bfad to 4e1fd54 Compare September 24, 2016 19:46
@leoetlino
Copy link
Member

leoetlino commented Sep 24, 2016

The commit title and message are a bit long (they should be about 52/72 characters long respectively). IMO, the title should be made shorter and the message should be wrapped to make it easier to read in a terminal and show up better even on GitHub.

@ligfx ligfx force-pushed the fix_sigabrt_when_quitting_from_dock branch from 4e1fd54 to cbdcfaa Compare September 25, 2016 21:00
Fixes the issue on macOS where quitting Dolphin from the Dock causes a
crash report (https://bugs.dolphin-emu.org/issues/9794). I'm not
exactly sure why this works, but it feels right and it turns out to fix
the problem.
@ligfx ligfx force-pushed the fix_sigabrt_when_quitting_from_dock branch from cbdcfaa to d1475df Compare September 25, 2016 21:07
@ligfx
Copy link
Contributor Author

ligfx commented Sep 25, 2016

@leoetlino done

@ligfx ligfx changed the title Move LogWindow and LogConfigWindow destructor logic into OnClose handlers Move LogWindow/LogConfigWindow destructor logic -> OnClose Sep 25, 2016
@lioncash lioncash merged commit 10cccd9 into dolphin-emu:master Oct 2, 2016
@EmptyChaos EmptyChaos mentioned this pull request Oct 4, 2016
14 tasks
@shuffle2
Copy link
Contributor

shuffle2 commented Oct 4, 2016

I had to revert some of this commit here #4292 could someone who was affected by it please look/test and see if I will regress you or not.

@ligfx ligfx deleted the fix_sigabrt_when_quitting_from_dock branch October 21, 2016 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants