executionByFork / MCC_Mod_Manager

A Mod Manager for Halo MCC on PC
Other
13 stars 11 forks source link

Tab Pattern Implementation + API refactor #1

Closed Darksound9 closed 4 years ago

Darksound9 commented 4 years ago

Hey there Forko

It's Dagan, or I'm fine with Darksound, or Dark if you prefer.

I'm a developer primarily focused in C# in .NET Framework & .NET Core, with significant experience in Microsoft SQL Server databases, Azure pipelines, Docker containers, and of course WinForms. There's plenty of other hats I've worn in my 2 & 1/2 years as a dev, not counting collegiate experience, but those are my strong suits. Here is my resume so you can get a better feel for my background. Feel free to add me on LinkedIn!

Now, onto the product! I uh...I rearranged a lot of your app lol. Fear not, it was not done wantonly. Rather than diving right into the PR, I recommend pulling the branch yourself, and comparing the changes done on a commit by commit basis. I'll break down the action items here. While you appear to have a technical background as solid as my own, I'm going to be thorough here until I can get a better feel for your knowledge base. Do excuse my verbosity!

  1. Params were provided to in-handler API calls by providing whatever's needed from the form. Unfortunately, I was unable to find an easy way to modify the params sent to event handlers without creating brand new controls--textboxes, buttons, etc--and sending info from those. To me that was an amount of work that defeated the purpose of the enhancement. Instead I opted to use a global reference to MasterForm for the API's to use, instead of receiving any params. This reference is in initialized in Program.cs. The state of the entire application can be read from Program.MasterForm from any of the static libraries. As I'm sure you know, as the project grows, use of such a global reference sporadically and inconsistently throughout the API can result in spaghetti code. So long as oversight is maintained on this reference, we can enjoy its convenience...carefully ;) Mostly by keeping form reads/updates as consolidated to the four major API's as possible, or in whatever way you want to guide the architecture.
  2. Making click events refer to calls in other API classes (those outside of said Forms code-behind) can only be done by modifying this same forms designer file. This is not ideal, as any change to the element from within the designer (even shifting it two pixels) annihilates custom event hooks. So instead I've moved the handler registration process into MasterForms code-behind file, into the LoadEventHandlers function under the General Functions region. This is called when the form is initially loaded, and this way makes it invisible to the designer. You just have to be careful when clicking on elements in the designer, so it doesn't generate handlers you don't need for them in the code behind. Doing so shouldn't break existing hooks, just cause it to run the original + an empty shell on click. I'm 90% sure you can add multiple events to a hook.

Phew. Aaaannd...that's it. As I said over text, your mod manager is the cleanest of those I've used. I'd like to work with you to keep enhancing it so it becomes the de facto choice for managing mods for MCC, and I intend to bring to you a feature to help cement that fact. Y'see, I'm one of the fortunate souls who has the Microsoft Store version of the game. Judging by the fact that the default directory shows for steam, I'm guessing you do not. The store version is encrypted as a UWP app in the WindowsApps folder on the C drive. In order to access it, you need to use a dumper executable that hijacks a winApp encrypted process of your choice (Could be MCC, Gears 5, hell even the Xbox app) like a dang virus, and uses it to dump the contents into the format you see and are so familiar with on Steam. From there, the game can be played in its dumped form both Once this is done, a series of powershell scripts is required to register the game for quick launch. It's not easy or intuitive for the average person, and I'd like to automate it inside of your manager! I think doing so will make it an immediate choice for a very significant chunk of the user base, gaining the tools popularity. We'll talk more about what that all will look like later. I'm not yet sure whether it should be built in directly, a separate form, or a different solution all together, idk. But I'd like them to be tied at at least a surface layer to encourage usage of the manager.

Thanks so much for reading this far, if you have. I hope to continue collaborating with you on this project soon!

Cheers, Dag/Darksound

executionByFork commented 4 years ago

Thank you for the time you spent on these changes. Unfortunately, however, I can't accept this PR for a number of reasons.

  1. These are changes against the master branch. I am actively developing the beta v0.7 version on a separate branch and my update is going to change a fair amount of the internals. This massive PR would not merge without every single change becoming a merge conflict. Resolving these would be a massive undertaking which I do not have time for.
  2. Along the same lines as the first point, this PR is way to big to merge. You have 50 changed files in this, with an unknown amount of code line changes. Because you've renamed files and made changes in the same commit, it hides all of the actual code changes you made, making it impossible for me to review the actual changes to the code. Changes of this size should be broken out into smaller, easily digestible changes. You may understand all of the code, but I don't because I didn't write it. To me this looks like an entirely new project.
  3. You are including a bunch of DLLs for a tool called Fody in this. I have no idea why including Fody is necessary here. This is supposed to be a small, lightweight tool which makes backups of files before overwriting them. I am already going to have to have two DLLs sitting in the same folder as the mod manager exe, and I think that alone is too many. Adding all of these files just clutters up the entire folder. Additionally, because DLLs are binary files, I have no way of verifying that they do not contain malicious code inside of them. I will not commit any code to this repository unless I know exactly what it is doing and that it will not compromise people who use it. You are going to have to find a way to make these changes without including DLLs in the PR.

If you want to help improve this tool, I would be glad to look over PRs that you send me, but the commits must be small, incremental changes, and each commit should have a body explaining what the changes are and why they are being changed. Please refrain from moving files around and changing file names in the same commit that you are making edits to the actual code. File name changes should be their own, separate commit so GitHub shows that a file is renamed, instead of showing a file as deleted and a new file created. Also, please don't rename variables and functions unless it is absolutely necessary, and if you do this, again make the name changes their own separate commit. Lastly, I will not accept binary files in PRs under any circumstances. It's just too much of a risk for malware.

Darksound9 commented 4 years ago

I'm opting to not respond in a public manner for now. Lol

Darksound9 commented 4 years ago

C h r i s t.

The author isn't interested in collaboration on new features in any capacity. On top of the slow input time, they seem intent to prove their own abilities on their own timeline with their own very specific vision of this product. I personally advise any devs with new ideas they'd like to introduce to keep said ideas to themselves. Only work with this author if you are happy implementing only his specific subtasks that he delegates to you.

executionByFork commented 4 years ago

I have not dismissed any of your ideas, I simply asked you to break them down into sizable commits with one feature a piece. I discussed this PR with you over email in length, so suggesting that I am not willing to collaborate in any capacity is quite false. I have given you valid reasons multiple times as to why I will not accept this pull request as is, and have given you options to break down your changes and resubmit as one or multiple PRs, yet you are choosing not to do this. It seems as if you are the one not willing to work with me.

Also, PRs aren't intended for this sort of discussion, so I am going to lock this. If you would like to discuss your changes with me further you can email me.