Implement Migration Tool

[MahBoiDeveloper]

Closes #750

To Do List

• Write patches for each release with major config changes
• Add embedded missing images and copy them if they are absent in Resource folder
• Fix Migration-INI.md wrong Edit GlobalThemeSettings.ini section
• Test migration tool on Mental Omega config
• Test migration tool on Twisted Insurrection config

[github-actions]

Nightly build for this pull request:

• artifacts.zip
  This comment is automatic and is meant to allow guests to get latest automatic builds without registering. It is updated on every successful build.

[SadPencil]

Remaining tasks:

• Assist Rampastring.Tools to allow editing the ini files without writing the parent ini to the child Rampastring/Rampastring.Tools#15
• Don't remove the comments in an ini file
• Investigate why the message box border (not the border in DrawBorders!) in Mental Omega disappears with @SadPencil, perhaps in Auguest
• Cliam this tool is experimental and might have potential bugs even if it gets included in a stable release in xna-cncnet-client. Show a prompt that modders must backup their files and test in through after applying this migration tool and we have absolutely no warrently.
• Address the wrong usage of string.Replace() method that triggers a CA1806 warning

[Metadorius]

• Don't remove the comments in an ini file

that one would be hard

[MahBoiDeveloper]

• Don't remove the comments in an ini file

Impossible to implement with current state of Rampastring.Tools

[SadPencil]

• Don't remove the comments in an ini file

Impossible to implement with current state of Rampastring.Tools

All right. Let's deal with the rest ones first.

[MahBoiDeveloper]

Note that this null reference issue was fixed via changing patch = new Patch_v2_11_0(arg).Apply(); to patch = new Patch_v2_11_0(arg); patch.Apply();. And therefore, the return type of Apply() method has been changed to void in de40f80

Check new implementation that removes needs to manually add every new patch in apply list.

[SadPencil]

Just letting you know that I just merged the develop branch into this PR. You should encounter a compile error about the wrong string.Replace() usage. It could be easily fixed so I'll leave this fix to you.

[SadPencil]

Note that this null reference issue was fixed via changing patch = new Patch_v2_11_0(arg).Apply(); to patch = new Patch_v2_11_0(arg); patch.Apply();. And therefore, the return type of Apply() method has been changed to void in de40f80

Check new implementation that removes needs to manually add every new patch in apply list.

                        .OrderBy(type => type.FullName) // Used to order patches by their names

What if we released 2.12.10? Would the string be placed before 2.12.9? Also consider something like 2.12.9-rc.1

Therefore, I suggest you use a list of string to store these patch class names, instead of finding and sorting them. Activator.CreateInstance lines can be preserved in my opinion.

[MahBoiDeveloper]

What if we released 2.12.10?

It would be applied after 2.12.9 and before 2.12.11 and before latest patch. Latest patch closes the list.

[SadPencil]

@SadPencil What if we released 2.12.10?

@MahBoiDeveloper It would be applied after 2.12.9 and before 2.12.11 and before latest patch. Latest patch closes the list.

It wouldn't be applied after 2.12.9 and before 2.12.11.

List<string> names = ["2.12.9-rc1", "2.12.9", "2.12.10"];
names=names.OrderBy(name=>name).ToList();
Console.Write(string.Join(", ", names));

Output:

2.12.10, 2.12.9, 2.12.9-rc1

That's why I suggest using a list of strings

[SadPencil]

Note that you need to save the patch variable outside of the try-catch scope. patch is always null in the catch block now.

var newPatch = CreateInstance();
patch = newPatch;
patch.Apply();

[MahBoiDeveloper]

That's why I suggest using a list of strings

Set this list manually is a mistake. But I have found another way to organize this list. We can just use enum extension to get all values of Version enum.

[SadPencil]

That's why I suggest using a list of strings

Set this list manually is a mistake. But I have found another way to organize this list. We can just use enum extension to get all values of Version enum.

seems good

[SadPencil]

Including clientcore? What if polyfill requires a lot of dll files (like the updater in 2.12.0)

[MahBoiDeveloper]

Including clientcore? What if polyfill requires a lot of dll files (like the updater in 2.12.0)

It is a good question how to include already existed enums into migration tool.

[SadPencil]

Including clientcore? What if polyfill requires a lot of dll files (like the updater in 2.12.0)

It is a good question how to include already existed enums into migration tool.

Just repeat the definition for now? I don't have a better way

[MahBoiDeveloper]

This doesn't feel right

[SadPencil]

This doesn't feel right

yeah. Let's just repeat the definition.

[MahBoiDeveloper]

yeah. Let's just repeat the definition.

@Metadorius what do you think? I agree with @SadPencil to reduce numbers of libs by repeating the definitions.

[Metadorius]

Why can't you reuse the same libs the client uses?

BTW, this is one of the reasons I proposed for it to be a part of the client invoked as a command line parameter.

[MahBoiDeveloper]

BTW, this is one of the reasons I proposed for it to be a part of the client invoked as a command line parameter.

Client assembly differences with libraries between different versions (like Rampastring.Tools.dll near to clientdx.exe that crash client on start) should not affect the migration tool. That's why I decided to make it as cli-tool in executable file.

[Metadorius]

Client assembly differences with libraries between different versions (like Rampastring.Tools.dll near to clientdx.exe that crash client on start) should not affect the migration tool. That's why I decided to make it as cli-tool in executable file.

Do we really need to inconvenience ourselves that much because of issues of wrong binaries? IMO we would save a lot of hassle by not handling the problems with binaries. The migration tool doesn't apply 100% of the needed changes anyway, so it's not like we will lose a lot by not handling this.