openMSX / wxcatapult

23 stars 4 forks source link

fix XRC warnings in wxWidgets 3.2.0 #44

Closed pjagiello closed 2 years ago

pjagiello commented 2 years ago

When compiling Catapult using newest wxWidgets (3.2.0), it results in lots of warnings being displayed on startup (and when opening other dialogs afterwards), I believe this is due to this change:

Using invalid flags with wxBoxSizer or wxGridSizer items now triggers asserts when done from the code or error messages when done in XRC. These asserts are best avoided by fixing the flags, but wxSizerFlags::DisableConsistencyChecks() can be used to globally suppress them until this can be done. Even less intrusively, environment variable WXSUPPRESS_SIZER_FLAGS_CHECK can be set (to any value) to achieve the same effect.

(from https://raw.githubusercontent.com/wxWidgets/wxWidgets/v3.2.0/docs/changes.txt )

The aforementioned workaround with the environment variable didn't work for me on Linux.

The warnings are like so (I removed duplicates):

18:14:17: XRC error: 40: vertical alignment flag "wxALIGN_CENTER_VERTICAL" has no effect inside a vertical box sizer, remove it and consider inserting a spacer instead
18:14:17: XRC error: 305: horizontal alignment flag "wxALIGN_CENTER_HORIZONTAL" has no effect inside a horizontal box sizer, remove it and consider inserting a spacer instead
18:14:17: XRC error: 11: "wxEXPAND" is incompatible with horizontal alignment flag ""wxALIGN_CENTER_HORIZONTAL"" in a vertical box sizer
18:14:17: XRC error: 20: "wxEXPAND" is incompatible with vertical alignment flag ""wxALIGN_CENTER_VERTICAL"" in a horizontal box sizer
18:14:17: XRC error: 279: both "wxALIGN_RIGHT" and "wxALIGN_CENTER_HORIZONTAL" specify horizontal alignment and can't be used together

I removed the flags that the warning claim to have no effect, for wxEXPAND errors I opted to leave wxEXPAND instead of the other flags although I'm not sure what is intended (after fixing I noticed a change in how the main window behaves when resized, but I think the behaviour after the patch is reasonable).

I'm not overly familiar with wxWidgets so I might have missed something, but after this patch no errors seem to appear. Not sure about any other incompatibilities with 3.2.0 (the link lists some others) but it seems that other than the warnings, Catapult works just fine.

MBilderbeek commented 2 years ago

@pjagiello thanks for this contribution!

@Vampier are you able to test this on Windows when I merge this? Binaries are here: https://github.com/openMSX/wxcatapult/actions/runs/2853286099 Just replacing the exe won't be enough, the xrc files must be replaced as well (if possible, just running the full thing from the zip there would be the best test).

In any case, test attention for this PR must fully go to window layout and resize behaviour.

As we still use wx 3.0.x for Windows (and I also use that locally on Linux), it would be good to check that we're still OK on these libs.

@pjagiello Have you also tested these changes with wx 3.0.x?

Vampier commented 2 years ago

The auto compile should work - I’ll be down for the count for a few as I am recovering from surgery Sent from my iPhoneOn Aug 21, 2022, at 12:56, Manuel Bilderbeek @.***> wrote: @pjagiello thanks for this contribution! @Vampier are you able to test this on Windows when I merge this? Binaries are here: https://github.com/openMSX/wxcatapult/actions/runs/2853286099 Just replacing the exe won't be enough, the xrc files must be replaced as well (if possible, just running the full thing from the zip there would be the best test). In any case, test attention for this PR must fully go to window layout and resize behaviour. As we still use wx 3.0.x for Windows (and I also use that locally on Linux), it would be good to check that we're still OK on these libs. @pjagiello Have you also tested these changes with wx 3.0.x?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

MBilderbeek commented 2 years ago

@Vampier Ah, I wasn't talking about compiling, but about testing. But first get yourself recovered. We'll find another way to test it on other platforms as well.

pjagiello commented 2 years ago

@MBilderbeek Haven't tested it with 3.0.x, no (patch was mainly motivated by my "bleeding edge" distribution already switching to 3.2). I can attempt it later this week though.

pjagiello commented 2 years ago

Did some more thorough testing, and found two flags that I removed that were unrelated to the warnings (and it made the rendering engine dropdown look off-center) - should be fixed now.

Did some testing with wxWidgets 3.0.5.1 and it looks to me that 3.0.5 with and without my patch, and 3.2 with the patch look the same, see the main window: main_wx30_nopatch

... but 3.2 without the patch looks funky (this is the change in behaviour that I noticed): main_wx32_nopatch With the patch it looks like it looked like in 3.0.5.

There is also a difference on the Misc Controls tab, with the Action Controls box. In 3.0.5 and patched 3.2 it looks like this: misc_wx30_nopatch

while in unpatched 3.2 the box is bunched up on the right: misc_wx32_nopatch

Other than that I also noticed that horizontal sliders look slightly different in 3.2 (it has these gray horizontal "ticks" along the slider, you can see it on the Performance Controls box in the previous screenshots), but I think this is just a wxWidgets change and unrelated to my patch.

Overall, it seems to me that on Linux the patch works fine on 3.0.5 and fixes some issues introduced in 3.2.

Other places where I had to make a choice which flag to retain (but I found no difference in testing on Linux):

MBilderbeek commented 2 years ago

Do I understand correctly that the Action Controls change is now also fixed in this PR? I.e. it looks the same as before and with 3.0.2?

pjagiello commented 2 years ago

Yes, as best I can tell, everything (including Action Controls box) looks the same in 3.0.5 before the PR, 3.0.5 after the PR, and 3.2.0 after the PR (other than the ticks thing I mentioned), only 3.2.0 before the PR looks different (all buttons on the right).

MBilderbeek commented 2 years ago

Thanks a lot for this update!