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

Settings context menu #116

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

networkfusion
Copy link
Collaborator

@networkfusion networkfusion commented Jul 3, 2024

Description

Adds ability to set the config.ini settings from the menu.

Motivation and Context

Ability to set the settings was missing from the menu.

How Has This Been Tested?

Tested on an SC64.

Screenshots

image

Types of changes

  • Improvement (non-breaking change that adds a new feature)
  • Bug fix (fixes an issue)
  • Breaking change (breaking change)
  • Config and build (change in the configuration and build system, has no impact on code or features)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>

@networkfusion networkfusion added the enhancement New feature or request label Jul 3, 2024
src/menu/views/settings_editor.c Outdated Show resolved Hide resolved
src/menu/views/settings_editor.c Outdated Show resolved Hide resolved
src/menu/views/settings_editor.c Outdated Show resolved Hide resolved
src/menu/views/settings_editor.c Outdated Show resolved Hide resolved
src/menu/views/settings_editor.c Show resolved Hide resolved
src/menu/views/settings_editor.c Outdated Show resolved Hide resolved
src/menu/views/settings_editor.c Outdated Show resolved Hide resolved
@IlDucci
Copy link

IlDucci commented Jul 4, 2024

This submenu looks great, do you think it could use a bit of simplification?

  • There are two different phrases for the usage of the A Button. Maybe you could remove the first line and change the A text hint at the bottom to, for example, "A: Save and Exit Menu".
  • Maybe the first asterisk is superfluous. If there are two different places where you can change the default directory, you could either try to avoid having the two sources of change and put everything into a single place or simply remove that extra information.

@networkfusion
Copy link
Collaborator Author

This submenu looks great, do you think it could use a bit of simplification?

* There are two different phrases for the usage of the A Button. Maybe you could remove the first line and change the A text hint at the bottom to, for example, "A: Save and Exit Menu".

* Maybe the first asterisk is superfluous. If there are two different places where you can change the default directory, you could either try to avoid having the two sources of change and put everything into a single place or simply remove that extra information.

IMO, when actually using the menu, the context of A is more intuatitive than all other alternatives I have attempted.
But given that all changes are auto saved when you change them, there is no point in displaying a "Save and exit".

I kinda agree that the asterisk's should not be there, but they can be removed at a later date, once we can fix the issue with SFX needing a restart and a better way to show read only options.

This PR was (for the moment) only trying to reach the develop branch, since it requires the SFX anyway, but could be merged with main, since it will sill work.

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

Successfully merging this pull request may close these issues.

None yet

3 participants