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

Add Settings Menu #31

Closed
wants to merge 9 commits into from

Conversation

Azure-Agst
Copy link
Contributor

This PR adds a separate settings menu as requested in #30 alongside a few other tweaks here and there.

  • Renamed Config to ConfigData, and wrapped it in a Config class that does loading/saving from JSON.
  • Moved a bunch of Text Boxes and Checkboxes to a new Settings Menu
  • Added a new menu item to open app from tray
  • Alert user on first shrink to tray on how to close app fully
  • Fixed "Show Time Lapsed" functionality

@gabehxd
Copy link
Member

gabehxd commented Dec 31, 2019

Thanks for another PR, for the settings i would like to keep things such as the states, image keys, and image text to be on the main form instead of settings for faster switching and perhaps even move IP and client ID to settings

@Azure-Agst
Copy link
Contributor Author

Azure-Agst commented Dec 31, 2019

Two thoughts: We might be able to keep the large key/text in settings since the app sets the that ID automatically and potentially convert those text boxes to house the Home Menu key/text values. Or, we could bring those boxes back to main view and have them be populated automatically with the Name/ID from the Switch.

As for the Client ID, I can see that moving to settings but then first-time use flow would be kinda wonky which was holding me back from sending it over in the first place. Maybe just have another Message box pop up on first run that opens settings?

@gabehxd
Copy link
Member

gabehxd commented Dec 31, 2019

The text boxes act more as an override that is meant to be more easily accessible and it seems right to keep it in the main view at all times for quick customization. As for the Client ID and IP a prompt on first opening the app to ask for an IP and Client ID would work well.

@Azure-Agst
Copy link
Contributor Author

Gotcha. I'll poke around later tonight after New Year's festivities die down on my end.

@gabehxd
Copy link
Member

gabehxd commented Dec 31, 2019

Alright sounds good. I was also thinking about making the switch over to the built in settings from JSON but I think i will do that once this PR is all set so i can also improve CLI settings aswell.

@Azure-Agst
Copy link
Contributor Author

So here's what I came up with:

  • New setup flow opens Settings on first launch for Client ID input
  • Moved Large Image Key/Text back to Main Menu, disabled by default
    • Auto-populates key with Title ID and text with Game Title
  • Added toggle for Large Key/Text Override in settings for users that desire to manually set Key/Text
    • Still uses defaults unless explicitly overwritten in text box, like the original behavior
  • Added Client ID Validation in settings
  • Added a Quit button on Main Menu.

@gabehxd
Copy link
Member

gabehxd commented Jan 2, 2020

Most of the code looks good but I'm not sure if I like how override has to enabled (I did enabled by default) I would rather keep that out.

@Azure-Agst
Copy link
Contributor Author

I added the override option because the majority of the users most likely wouldn't end up using the override, but those who want it and know what they're doing would still have the option. I figured making the text read-only by default could prevent an accidental character deletion and potentially a future issue submission because of it. Just makes it more user-proof, I guess?

@gabehxd
Copy link
Member

gabehxd commented Jan 2, 2020

Hm, I think more people would use it but either way i think it's a little unnecessary.

@Azure-Agst
Copy link
Contributor Author

Azure-Agst commented Jan 2, 2020 via email

@Azure-Agst
Copy link
Contributor Author

Closing this due to inactivity.

@Azure-Agst Azure-Agst closed this Jan 30, 2020
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.

2 participants