Skip to content
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

Rework config file generation and fix cases where configs were ignored #183

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

itsschwer
Copy link

Changes

  • Split the binding for each component's configs into separate functions
  • Only generate configs for enabled components
    • Avoids cluttering users' config folder with unnecessary config files
    • Encourages that each component is self-contained
  • Remove config StatsDisplayEnable and replace with ComponentsStatsDisplay (previously effectively useless)
    • Makes StatsDisplay component more consistent with other components — see below for further suggestions
  • Check config ComponentsItemSorting is enabled before attempting to sort Command/Scrapper menus

Potential future changes

I wasn't sure if I should work on these areas, if you had existing plans or suggestions, or if I should open these as issues.

  • StatsDisplay.Hook() is empty and inconsistent with other components (hooking currently occurs in constructor?)
    • Should this be addressed?
  • Review coupling between CommandImprovements and ItemSorting
    • May be a good idea to move the sorting logic from CommandImprovements.cs to ItemSorting.cs, using another PickupPickerPanel_SetPickupOptions hook
  • Fix typos in config variable names
    • Misc: NotificiationsNotifications
    • Misc, AdvancedIcons: EquipementEquipment

Avoid cluttering users' config folder
To further improve consistency with other components, hooking functions should be moved from the constructor to the currently empty `StatsDisplay.Hook()`
Sorting logic should probably be moved to ItemSorting.cs
Copy link
Contributor

github-actions bot commented Feb 3, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@itsschwer
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant