Page MenuHomeWildfire Games

Fixed assert fails in Actor Editor
ClosedPublic

Authored by shh on Mar 8 2019, 3:19 PM.

Details

Summary

The first assert was from wxWidgets. It fails as both expand and align flags are passed to the sizer. Removed align flag to match the dialog as shown here.
Second assert was simply not true. So I've changed it to if statement.

Test Plan

After reproducing steps from ticket, no assert is failed.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6954
Build 11378: Vulcan BuildJenkins

Event Timeline

shh created this revision.Mar 8 2019, 3:19 PM
Vulcan added a subscriber: Vulcan.Mar 8 2019, 3:21 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/AtlasUI/CustomControls/Windows/AtlasDialog.cpp
|   1| /*·Copyright·(C)·2009·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2009"

source/tools/atlas/AtlasUI/CustomControls/EditableListCtrl/EditableListCtrl.cpp
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2014"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1085/display/redirect

shh added a comment.Mar 8 2019, 3:23 PM

Oops, I've forgot to fix the copyright year. Should I fix and add new diff?

vladislavbelov added inline comments.Mar 8 2019, 3:36 PM
source/tools/atlas/AtlasUI/CustomControls/EditableListCtrl/EditableListCtrl.cpp
79

By CC:

if (col < 0 || col >= static_cast<int>(m_ColumnTypes.size()))
    return;
source/tools/atlas/AtlasUI/CustomControls/Windows/AtlasDialog.cpp
54

Which version of wxWidget do you use? Because it may depend on the version and may don't work for some old but still supported version AFAIK.

You could use "Update Diff" in right panel or update via arch.

shh updated this revision to Diff 7531.Mar 8 2019, 3:52 PM
shh marked an inline comment as done.
shh added inline comments.
source/tools/atlas/AtlasUI/CustomControls/Windows/AtlasDialog.cpp
54

I'm using 3.1.2. Which versions are supported? I may try on them too.

shh added inline comments.Mar 8 2019, 4:06 PM
source/tools/atlas/AtlasUI/CustomControls/Windows/AtlasDialog.cpp
54

Quick note. The assert was added for this combination of flags by this commit on Apr 3, 2015. According to wikipedia it should be added to wxWIdgets 3.0.3.

shh updated this revision to Diff 7532.Mar 8 2019, 4:07 PM
Silier added a subscriber: Silier.EditedMar 17 2019, 8:22 PM

I can confirm this is fixing assertion error in both cases described in ticket.

wraitii accepted this revision.Apr 20 2019, 5:13 PM
This revision is now accepted and ready to land.Apr 20 2019, 5:13 PM
vladislavbelov added inline comments.
source/tools/atlas/AtlasUI/CustomControls/Windows/AtlasDialog.cpp
54

The problem is I suppose that we still support platforms where 2.8 is by default or we suggest 2.8 by default. Such as FreeBSD or Mandriva: https://trac.wildfiregames.com/wiki/BuildInstructions.

@Itms what is our plan about the wxWidgets?

Stan added a subscriber: Stan.Apr 23 2019, 8:13 AM
Stan added inline comments.
source/tools/atlas/AtlasUI/CustomControls/Windows/AtlasDialog.cpp
54

Well we already dropped it with that commit. We might as well make it official.

Itms added a comment.Apr 23 2019, 8:30 AM

I think that if we keep supporting 2.8 we need to build patches with it, else we will commit a lot of 3.0+ changes without noticing. I believe the overwhelming majority of distributions used by our devs and contributors ships 3+.

I would be in favor of officially moving to 3+, which is possible if it is available (not necessarily the default,since it is for devs and package maintainers) on the platforms you mention.

In D1784#75828, @Itms wrote:

I think that if we keep supporting 2.8 we need to build patches with it, else we will commit a lot of 3.0+ changes without noticing. I believe the overwhelming majority of distributions used by our devs and contributors ships 3+.
I would be in favor of officially moving to 3+, which is possible if it is available (not necessarily the default,since it is for devs and package maintainers) on the platforms you mention.

I definitely agree to move forward and drop old versions. But probably we still have users with OS that uses 2.8 (and sometimes can't use newer versions without pain) by default or we recommend to use 2.8 by our build instructions.

So at least we need to make a remark about it in our build instructions.

Stan added a comment.Apr 23 2019, 12:48 PM
In D1784#75828, @Itms wrote:

I think that if we keep supporting 2.8 we need to build patches with it, else we will commit a lot of 3.0+ changes without noticing. I believe the overwhelming majority of distributions used by our devs and contributors ships 3+.
I would be in favor of officially moving to 3+, which is possible if it is available (not necessarily the default,since it is for devs and package maintainers) on the platforms you mention.

I definitely agree to move forward and drop old versions. But probably we still have users with OS that uses 2.8 (and sometimes can't use newer versions without pain) by default or we recommend to use 2.8 by our build instructions.

So at least we need to make a remark about it in our build instructions.

For Windows we officially dropped it when we committed #5098. For the rest of the platforms most of the ticket are about being compatible, but there is no clear decision.

In D1784#75860, @Stan wrote:

For Windows we officially dropped it when we committed #5098. For the rest of the platforms most of the ticket are about being compatible, but there is no clear decision.

What's about 2.9?

Stan added a comment.Apr 23 2019, 3:41 PM

Wrong ticket #2891

In D1784#75860, @Stan wrote:

For Windows we officially dropped it when we committed #5098. For the rest of the platforms most of the ticket are about being compatible, but there is no clear decision.

What's about 2.9?

2.9 was an unstable/dev branch which became 3.0 stable (even numbers = stable, odd numbers = unstable). So there should be no 2.9 in the wild. We only ever supported it temporarily until 3.0 was released.

vladislavbelov accepted this revision.Oct 2 2019, 4:31 PM

The patch seems ok to me. I'm going to commit that if nobody has objections.

P.S. We dropped 2.8, but AFAIK Slackware 14.2 uses 2.8, at least by https://slackware.pkgs.org/14.2/slackpack-x86_64/wxwidgets-2.8.12-x86_64-4gds.txz.html.

Owners added a subscriber: Restricted Owners Package.Oct 3 2019, 3:21 PM

@shh thank you for the patch! Sorry for long reviewing.