-
Notifications
You must be signed in to change notification settings - Fork 60
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
Project Refactoring #59
base: dev
Are you sure you want to change the base?
Conversation
…ttings saving/loading
@@ -19,7 +19,7 @@ public class ValueSwap | |||
public static void ReadEnemyList() | |||
{ | |||
EnemyList = new List<Enemy>(); | |||
string[] lines = Properties.Resources.ENEMIES.Split(new[] { "\r\n", "\r", "\n" }, StringSplitOptions.None); | |||
string[] lines = Resources.GetTextFile("ENEMIES.txt").Split(new[] { "\r\n", "\r", "\n" }, StringSplitOptions.None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean ENEMIES.txt (and other resource files) are now included as files instead of being part of the application? Not sure I like the idea of people fiddling with the resource files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In general, I think most people will avoid touching them out of fear of breaking the program. I also haven't heard of it being a huge problem in OoTR, even though most releases have always distributed the entire source code because the program is coded in Python (though I also don't pay attention to the support channels at all, so I could be wrong on that).
Still, i could make it an embedded file and do some reflection magic to retrieve it
mmr-lib/Models/Settings/LogicMode.cs
Outdated
Casual, | ||
[Description("Glitched Logic")] | ||
Glitched, | ||
[Description("Vanilla Logic")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is "Vanilla Logic" an appropriate name? the old name was "Vanilla Layout" which makes sense since none of the items are moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it because it seemed like a consistency thing, but you're right. I think changing it to "Vanilla Item Placement" would be more descriptive.
mmr-lib/Models/Settings/Settings.cs
Outdated
{ | ||
SetField(ref useCustomItemList, value); | ||
|
||
//Todo: enable/disable ui elements instead, then have a "validate" stage for generating settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an important todo to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a "validate" stage right now is not super important because we only have the one gui for now, but technically with the core logic being in it's own library now, it should be more responsible in managing it's own state within the mmr lib, and not the gui.
Edit: i see the segment of code this comment is on. So the issue here is that ui wise toggling the UseCustomItemList element will cause the ui to forget the user's settings for the other options, which is unexpected and undesirable. A better way would be to instead make it so that toggling UseCustomItemList will toggle the dependent Enable properties on the UI to avoid altering the backing settings.
winform/Forms/MainForm.Designer.cs
Outdated
@@ -38,14 +38,17 @@ private void InitializeComponent() | |||
this.cUserItems = new System.Windows.Forms.CheckBox(); | |||
this.tSettings = new System.Windows.Forms.TabControl(); | |||
this.tabROMSettings = new System.Windows.Forms.TabPage(); | |||
this.cPatch = new System.Windows.Forms.CheckBox(); | |||
this.label9 = new System.Windows.Forms.Label(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer that labels have proper names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tried, but i'm not particularly motivated to give names to ui elements that aren't being altered programmatically.
… previous purpose.
This expands on the code from #55