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

Drop Dolphin patch to wxWidgets (and fix issues) #4013

Merged
merged 1 commit into from Sep 9, 2016

Conversation

leoetlino
Copy link
Member

@leoetlino leoetlino commented Jul 15, 2016

This removes a Dolphin-specific patch to the wxWidgets3 code
for the following reasons:

  • Calling wxWindowGTK::DoSetSize on a top-level window can end up
    calling wxTopLevelWindowGTK::DoMoveWindow, which triggers an assert
    because it is not supposed to be called for a top-level wxWindow.
  • We should not be patching the wxWidgets code because that means the
    toolbars will still be broken if someone builds without using the
    WX that is in our Externals.

Instead, we now use a derived class for wxAuiToolBar and override
DoSetSize() to remove the problematic behaviour to get the same effect
(fixing toolbars) but without changing Externals code and without
potentially causing asserts.


This change is Reviewable

@delroth
Copy link
Member

@delroth delroth commented Jul 15, 2016

We shouldn't be patching externals WX at all. Linux distros are not building that code. I have no idea why we have a patch for this :(

@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Jul 15, 2016

I agree, but if we remove it completely, debug toolbars are broken :/ though I wonder if that's a bug in Dolphin's code or in wxWidgets.

@EmptyChaos
Copy link
Contributor

@EmptyChaos EmptyChaos commented Jul 16, 2016

@leoetlino The bug is with wxWidgets AUI. It computes the widget size then calls SetClientSize but it also overrides DoSetSize and clamps the resize values which causes the widget to not resize correctly. It does not expect SetClientSize to call DoSetSize (which it does not on Windows, but does on OS X and GTK). You can roll this change back and try to fix AUI instead, I changed it at the window level because I was afraid of other bugs caused by the same thing, I didn't experience any assertions myself (GTK2).

@delroth It was in the list of patches at the top of the PR.

@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Jul 16, 2016

Hmm, so it's a wx bug… I think it was a good idea to change it at the window level, so this can potentially fix more issues caused by the same bug, but it also caused a bug for top-level windows (wx asserts, which this PR fixes).

But the problem is that neither this fix nor the Dolphin-specific patches are going to be included if Dolphin is built without using the Externals wxWidgets. Patching wx AUI source wouldn't be better.

I'm also building with GTK 2, and I'm definitely getting asserts :/

@BhaaLseN
Copy link
Member

@BhaaLseN BhaaLseN commented Jul 16, 2016

Can we simply derive from the affected wx class in our code and override that particular method to do something else?

@EmptyChaos
Copy link
Contributor

@EmptyChaos EmptyChaos commented Jul 16, 2016

@BhaaLseN Realize and RealizeHelper are non-virtual so they can't be inherited. DoSetSize could be but I don't know if removing the clamping behavior will break something else.

@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Jul 16, 2016

Could we only override for wxAuiToolbar? Even then, the change would be smaller than the current patch, would fix the toolbar and the assertions.

@EmptyChaos
Copy link
Contributor

@EmptyChaos EmptyChaos commented Jul 16, 2016

@leoetlino I'm not sure I follow. Do you mean patching wxAuiToolBar inside wxWidgets, or trying to inherit from wxAuiToolBar and override DoSetSize to remove the clamping, or something else?

@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Jul 16, 2016

Sorry, I was a bit unclear.

I meant inheriting wxAuiToolbar and overriding DoSetSize, since we want to avoid patching the wx code.

@EmptyChaos
Copy link
Contributor

@EmptyChaos EmptyChaos commented Jul 16, 2016

@leoetlino It should be straightforward to do that

class DolphinAuiToolBar : public wxAuiToolBar
{
public:
  DolphinAuiToolBar(...) : wxAuiToolBar(...) {}
protected:
  void DoSetSize(...) override { wxWindow::DoSetSize(...); }
};

Then s/wxAuiToolBar/DolphinAuiToolBar/ in DolphinWX. It needs to be tested to make sure all the various debugger panels still work with this change.

@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Jul 16, 2016

So, I tried master on a clean Ubuntu 16.04 and an up-to-date sid install in containers, and the asserts didn't show up… so it must be something with my system, something that a full clean rebuild doesn't fix.
Anyway, I'm going to see if overriding that method in our own derived class works and doesn't bring any more issues.

@leoetlino leoetlino changed the title Fix wxWidgets assert-triggering code (wxGTK) Remove Dolphin-specific patch to wxWindowGTK Jul 16, 2016
@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Jul 16, 2016

Tried this on Linux -- the asserts and the toolbars are fixed. On Windows, this doesn't change anything. Could someone test this on OS X?

#include <wx/aui/auibar.h>
#include <wx/window.h>

// XXX: This fixes wxAuiToolBar setting itself to 21 pixels wide regardless of content

This comment has been minimized.

This comment has been minimized.

@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Jul 16, 2016

Actually, it looks like this fixes an issue with the previous patch which slightly changes some windows' sizes and the wxAuiToolbar in floating mode:

before:
before PR

after:
after PR

(the space at the top is where the window title would be)

@delroth
Copy link
Member

@delroth delroth commented Jul 17, 2016

@lioncash mind reviewing and merging? Thanks.

@EmptyChaos
Copy link
Contributor

@EmptyChaos EmptyChaos commented Jul 17, 2016

@leoetlino Could you remove the patch from the OS X wxWidgets port as well please.

@lioncash
Copy link
Member

@lioncash lioncash commented Jul 17, 2016

The bug should also be reported to wxWidgets devs (if it's not already), as well, so we can get rid of this whenever it does end up being fixed.

@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Jul 17, 2016

@EmptyChaos done. Have you already reported this bug?
And yes, this should be removed once it's fixed in wxWidgets.

@EmptyChaos
Copy link
Contributor

@EmptyChaos EmptyChaos commented Jul 17, 2016

wxWidgets Issue 17599

@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Jul 18, 2016

@dolphin-emu-bot rebuild
Thanks for reporting it; I do hope it gets fixed so we can drop this. But in the meantime, could someone review this? I have no idea why, but I'm getting asserts with the current patch.

@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Aug 14, 2016

This apparently also fixes games always launching in a tiny window on start, which makes sense given it removes the DoSetSize hack.

Also, it turns out that there were always asserts for anyone that made a debug build of Dolphin.

Seeing as those two issues are quite annoying, and the wx bug report got no response and it's been one month, could we consider merging this, and later removing the custom toolbar hack when this gets fixed by upstream?

@phire
Copy link
Member

@phire phire commented Aug 14, 2016

Code LGTM, but could someone make sure the debug toolbar works on both OSX and Linux?

@EmptyChaos
Copy link
Contributor

@EmptyChaos EmptyChaos commented Aug 14, 2016

I've tested this on Linux and it works for me, I believe @leoetlino develops on Linux so that was mostly assumed. I have no idea about OS X though.

@gamax92
Copy link

@gamax92 gamax92 commented Aug 14, 2016

Tested here on Linux and the toolbar seems fine, and can confirm here that it does fix the issue with games always launching in a tiny window on start.

@kemenaran
Copy link

@kemenaran kemenaran commented Aug 17, 2016

I just tested on OS X (10.11): the asserts are fixed, and it looks fine (although I don't know how it is supposed to look actually).

Docked

Windowed

@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Aug 22, 2016

Yes, that's how it's supposed to look like. Thanks for testing it on OS X.
(And yes, I tested this on Linux as well, and the reason for this PR is that the various bugs and asserts that only happen on Linux/OS X annoyed me)

@leoetlino leoetlino force-pushed the wx-assert-fix branch 2 times, most recently from 4cab07e to 9cf9283 Compare Sep 3, 2016
@leoetlino leoetlino changed the title Remove Dolphin-specific patch to wxWindowGTK Drop Dolphin patch to wxWidgets (and fix issues) Sep 3, 2016
@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Sep 3, 2016

Rebased and fixed the build. (Not sure why wxWidgets now needs wx/aui/auibar.h to be included; it didn't before).

This removes a Dolphin-specific patch to the wxWidgets3 code
for the following reasons:

* Calling wxWindowGTK::DoSetSize on a top-level window can end up
  calling wxTopLevelWindowGTK::DoMoveWindow, which triggers an assert
  because it is not supposed to be called for a top-level wxWindow.

* We should not be patching the wxWidgets code because that means the
  toolbars will still be broken if someone builds without using the
  WX that is in our Externals.

Instead, we now use a derived class for wxAuiToolBar and override
DoSetSize() to remove the problematic behaviour to get the same effect
(fixing toolbars) but without changing Externals code and without
causing asserts and other issues.
@leoetlino
Copy link
Member Author

@leoetlino leoetlino commented Sep 7, 2016

Is there anything else to do for this PR? The code has been reviewed and tested, and it'd be a shame to have the recent debugger improvements but a pretty much broken UI in debug mode.

@degasus degasus merged commit 6e488e6 into dolphin-emu:master Sep 9, 2016
10 of 11 checks passed
@leoetlino leoetlino deleted the wx-assert-fix branch Sep 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
9 participants