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

Change: Disable NewGRF window apply button if no change was made #8934

Merged
merged 1 commit into from Apr 5, 2021

Conversation

perezdidac
Copy link
Contributor

@perezdidac perezdidac commented Apr 3, 2021

Motivation / Problem

The NewGRF window has a 'Apply changes' button that's always enabled, even if the user has not made any change to the NewGRF. In addition, closing the NewGRF window without having made any change executes the reload of NewGRF which can take some time.

Description

This change makes the 'Apply changes' button to be disabled (grayed out) by default until changes are made, including loading a preset, dragging/dropping from/to the inactive NewGRFs list, clicking on the 'Add' or 'Remove' button, toggling the palette, and opening the parameters window. I believe with this all changes are covered. I have also verified with the scenario editor, where NewGRFs cannot be modified by default unless the hidden developer option is changed in the configuration file.

Limitations

None I can think of.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@LordAro
Copy link
Member

@LordAro LordAro commented Apr 3, 2021

The NewGRF window is (along with a lot of the other new game menus) is going to be rewritten for 1.12, so I'm tempted to close this as it's not a change that will ever see a release... (happy for other opinions)

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Apr 3, 2021

The NewGRF window is (along with a lot of the other new game menus) is going to be rewritten for 1.12, so I'm tempted to close this as it's not a change that will ever see a release... (happy for other opinions)

Good to know, is there a wiki or documentation about the plans? I don't want to work on things that will be re-written :)
Submitting this will benefit patches like JGRPP though, and making sure the next design adheres to the right UX principles.

src/newgrf_gui.cpp Outdated Show resolved Hide resolved
src/newgrf_gui.cpp Outdated Show resolved Hide resolved
src/newgrf_gui.cpp Show resolved Hide resolved
src/newgrf_gui.cpp Outdated Show resolved Hide resolved
@nielsmh nielsmh added the backport requested label Apr 5, 2021
@perezdidac perezdidac force-pushed the aaa branch 2 times, most recently from 01a01ae to f6946e5 Compare Apr 5, 2021
@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Apr 5, 2021

Thanks @michicc for the comments, I just made simplifications and looks pretty cleaner now.

@michicc michicc added the preview label Apr 5, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8934 Apr 5, 2021 Inactive
@michicc
Copy link
Member

@michicc michicc commented Apr 5, 2021

Found a missed variant: Moving a NewGRF in the active list up or down does not enable the apply button.

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Apr 5, 2021

Found a missed variant: Moving a NewGRF in the active list up or down does not enable the apply button.

Wohaa! Good catch! I can't believe I missed that, well that's awesome, let me fix that and force push the new code.

michicc
michicc approved these changes Apr 5, 2021
@michicc michicc changed the title Change: NewGRF window apply button to disable if changes not made Change: Disable NewGRF window apply button if no change was made Apr 5, 2021
@michicc michicc merged commit 43c465e into OpenTTD:master Apr 5, 2021
12 checks passed
@TrueBrain TrueBrain removed the backport requested label Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants