search

CompSciLauren / stardew-valley-daily-screenshot-mod

:honeybee: A Stardew Valley mod that automatically takes a screenshot of your entire farm at the start of each day
https://www.nexusmods.com/stardewvalley/mods/4779
MIT License
10 stars 9 forks source link

Adding 'Generic Mod Config Menu' mod integration to resolve issue #42… #46

Closed f3wer closed 3 years ago

f3wer commented 3 years ago

… in og repo.

Summary

Therefore added 3 new boolean fields in the ModsConfig.cs class and altered the OnGameLaunched method in ModEntry.cs. I used the this example code to implement this changes.

Things to consider

  1. I don't know if "old config files" without the new three boolean attributes will lead to problems. Maybee the fixup method must be expanded for this reason?
  2. The screenshot rules information is not configurable with the menu. I simply wouldn't know how to implement this. A corresponding disclaimer has been included to the config menu as a label with a text hint.
  3. You could probably add the constant default values as settable within range?
  4. Is the Thread.Sleep operation at line 687 in ModEntry.cs necessary; or could it also be made configurable to set MILLISECONDS_TIMEOUT "dynamically" to potentially avoid lag everytime a screenshot is taken?
  5. The smapi nugget package you're using could be updated.

Testing

I compiled and ran the game without any problems. The newly added features seemed to be working. Due to the fact that this is my first mod edit and pull request, I just wanna' stress that you may take a closer look.

CompSciLauren commented 3 years ago

Hey, first off thank you so much for working on this and creating a PR! Fantastic stuff. I'm reviewing the changes and testing things out, I'll try to finish reviewing this soon.

Just wanted to respond to your "things to consider" section:

  1. I tested this out and don't believe it will cause any problems. The config seems to update automatically without any issues.
  2. The disclaimer is definitely good to include in this case, good idea. I think what we might do is just try to get these changes merged, and we can make a separate GitHub Issue to add the rest of the config options at a later time. If we do go with that, I'll log a GitHub Issue. Will know once I'm finished reviewing. (Edit: added GitHub Issue #48)
  3. Sorry I don't think I understand what you're referring to here. Can you please explain a bit more? What constant default values are you referring to, and what do you mean by "settable within range"?
  4. That sounds like a good idea, I'll have to look into that.
  5. Good to know, thanks. I think I'll wait until we get this PR merged so that we can get these changes out to users. But after that I'll check that out. Logged #47 to track this issue.
CompSciLauren commented 3 years ago

Finished reviewing, looks great and is now merged! Thanks again for your efforts. Decided that I'll go ahead and see if I can add the other config options before we release the next update. Added issue #48 to help track this.

f3wer commented 3 years ago
  1. Sorry I don't think I understand what you're referring to here. Can you please explain a bit more? What constant default values are you referring to, and what do you mean by "settable within range"?

To be honest, I don't know what I meant. I think we can ignore this point.