Page MenuHomeWildfire Games

Fix Atlas crash introduced by rP23847
ClosedPublic

Authored by Stan on Jul 21 2020, 8:07 PM.

Details

Summary

rP23847 introduced a crash which happened each time someone used a non wxCheckbox control

Test Plan

Make sure everything works as expected, saving loading, editing fields

Remove conquest from binaries/data/mods/public/simulation/data/settings/victory_conditions/conquest_structures.json and binaries/data/mods/public/simulation/data/settings/victory_conditions/conquest_units.json and check that ticking conquest disable both of them. but that disabling it does nothing as it's handled by the rest of the code.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Stan created this revision.Jul 21 2020, 8:07 PM
Stan added inline comments.Jul 21 2020, 8:08 PM
source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
330 ↗(On Diff #12837)

There is something that doesn't work here for conquest, not sure what I do bad.

vladislavbelov added inline comments.Jul 21 2020, 8:16 PM
source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
115 ↗(On Diff #12837)

Might pass id instead, since it has the long type.

330 ↗(On Diff #12837)

What exactly doesn't work and how the break removing helps?

Stan added inline comments.Jul 21 2020, 8:17 PM
source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
115 ↗(On Diff #12837)

True

330 ↗(On Diff #12837)

Check conquest unit and structures

Uncheck one

Notice conquest is not disabled anymore

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2741/display/redirect

Silier added inline comments.Jul 21 2020, 8:30 PM
source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
330 ↗(On Diff #12837)

Hmm. Found another issue in current svn state.
Both checked -> disabled.
Uncheck one -> ticked and enabled.

Uncheck conquest.
Check structure -> uncheck structure -> conquest is checked

vladislavbelov added inline comments.Jul 21 2020, 8:37 PM
source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
244 ↗(On Diff #12837)

Don't use count for search, use find.

304 ↗(On Diff #12837)

Brace on new line. Also too many code, might be:

for (char& ch : escapedTitle)
	if (ch == ' ')
		ch = '_';

Also it's a code duplication .

Stan updated this revision to Diff 12839.Jul 21 2020, 10:00 PM

Fix notes

Stan marked 5 inline comments as done and an inline comment as not done.Jul 21 2020, 10:00 PM
Stan added inline comments.
source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
330 ↗(On Diff #12837)

Fixed Angen issue by setting it to false always.

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2743/display/redirect

Stan updated this revision to Diff 12841.Jul 21 2020, 10:08 PM

Fix name

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2744/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2746/display/redirect

Stan updated this revision to Diff 12844.Jul 21 2020, 11:31 PM

Fix the last bug

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2747/display/redirect

Stan updated this revision to Diff 12845.Jul 22 2020, 12:02 AM

Cleanup

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2748/display/redirect

Silier requested changes to this revision.Jul 22 2020, 9:14 PM
Silier added inline comments.
source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
316 ↗(On Diff #12845)

this is not working.
to test, remove "DisabledWhenChecked": ["conquest"], from one of the conquest specific files.

This revision now requires changes to proceed.Jul 22 2020, 9:14 PM
Stan updated this revision to Diff 12864.Jul 22 2020, 10:45 PM

Upload to the right diff T_T

Stan edited the test plan for this revision. (Show Details)Jul 22 2020, 10:53 PM

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2759/display/redirect

vladislavbelov accepted this revision.Jul 24 2020, 4:56 PM

I've tested the patch on Windows 10, it works for me for any map settings editing. The patch looks ok, a bit wordy but ok for atlas.

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Map/Map.cpp
111 ↗(On Diff #12864)

Unnecessary empty line.

Silier accepted this revision.Jul 24 2020, 8:44 PM

both disabledwhenchecked and changeonchecked works correctly
cannot reproduce any crashes

This revision is now accepted and ready to land.Jul 24 2020, 8:44 PM
This revision was automatically updated to reflect the committed changes.
Stan marked 2 inline comments as done.
Owners added a subscriber: Restricted Owners Package.Jul 24 2020, 8:53 PM