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

Remove unnecessary loading mod exceptions #13367

Merged
merged 1 commit into from Jun 29, 2017

Conversation

Projects
None yet
5 participants
@rob-v
Contributor

rob-v commented May 27, 2017

Removed exception "Failed to register currrent mod metadata
System.ArgumentException: An item with the same key has already been added." from debug.log on each OpenRA launch and some loading exceptions in debugger.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 28, 2017

Contributor

Updated - added 'missing icon.png' for non hidden mods to debug

Contributor

rob-v commented May 28, 2017

Updated - added 'missing icon.png' for non hidden mods to debug

Show outdated Hide outdated OpenRA.Game/Game.cs
icons[id] = sheetBuilder.Add(bitmap);
return new Manifest(id, package);
if (package.Contains("mod.yaml"))

This comment has been minimized.

@pchote

pchote May 29, 2017

Member
if (!package.Contains("mod.yaml"))
	return null;

would be more concise.

You can put the package disposal inside a finally block to make sure it gets cleaned up on these early returns.

@pchote

pchote May 29, 2017

Member
if (!package.Contains("mod.yaml"))
	return null;

would be more concise.

You can put the package disposal inside a finally block to make sure it gets cleaned up on these early returns.

This comment has been minimized.

@rob-v

rob-v May 29, 2017

Contributor

This change + move disposal to finally isn't sufficient, because we want disposal on error, missing mod.yaml, but not on valid package so it would need more refactoring and the end result might be less concise than now.

@rob-v

rob-v May 29, 2017

Contributor

This change + move disposal to finally isn't sufficient, because we want disposal on error, missing mod.yaml, but not on valid package so it would need more refactoring and the end result might be less concise than now.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 29, 2017

Contributor

Updated

Contributor

rob-v commented May 29, 2017

Updated

@pchote pchote dismissed their stale review Jun 22, 2017

Code changed.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 22, 2017

Member

This complements a PR that I have planned to improve mod registration for the playtest, so adding to the milestone. Will try and re-review on the weekend.

Member

pchote commented Jun 22, 2017

This complements a PR that I have planned to improve mod registration for the playtest, so adding to the milestone. Will try and re-review on the weekend.

@pchote pchote added this to the Playtest featuring updated HitShapes milestone Jun 22, 2017

@pchote

pchote approved these changes Jun 24, 2017

Looks good. Just one minor request, and then bonus points if you can also remove the obsolete comment about support dir / oramods at L116.

Show outdated Hide outdated OpenRA.Game/InstalledMods.cs
@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 24, 2017

Contributor

Updated - added also check in Game.InitializeMod as it crashed lately when launched with wrong mod id.

Contributor

rob-v commented Jun 24, 2017

Updated - added also check in Game.InitializeMod as it crashed lately when launched with wrong mod id.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 25, 2017

Contributor

Updated.

Contributor

rob-v commented Jun 25, 2017

Updated.

@pchote

pchote approved these changes Jun 27, 2017

@pchote pchote requested review from reaperrr and abcdefg30 Jun 29, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 29, 2017

Contributor

Can't really comment on the code, but the "Failed to register currrent mod metadata..." is gone from debug.log and the "Unknown or invalid mod" exception works, too.
I'll wait a little with merging due to the nit, but otherwise I think this is good to go.

Contributor

reaperrr commented Jun 29, 2017

Can't really comment on the code, but the "Failed to register currrent mod metadata..." is gone from debug.log and the "Unknown or invalid mod" exception works, too.
I'll wait a little with merging due to the nit, but otherwise I think this is good to go.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 29, 2017

Contributor

Updated debug text.

Contributor

rob-v commented Jun 29, 2017

Updated debug text.

@reaperrr reaperrr merged commit f75115a into OpenRA:bleed Jun 29, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment