Closing modal on "Next" pressed in editors #4486
Conversation
src/gui_common/ModalManager.cs
Outdated
| /// Attempt to clear all open modals. | ||
| /// </summary> | ||
| /// <returns>Returns true if all modals were closed, false otherwise.</returns> | ||
| public bool ClearModals() |
There was a problem hiding this comment.
Would CloseAllOpenModals be a better name?
There was a problem hiding this comment.
I think the current one is already good. This indeed clears modalStack by closing all.
| @@ -125,6 +130,25 @@ public bool HideTopMostPopup() | |||
| return null; | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Attempts to hide the given modal. | |||
| /// This won't hide the modal if it's exclusive and doesn't allow closing on escape. | |||
There was a problem hiding this comment.
Woops! Good catch :)
| /// <summary> | ||
| /// Attempt to clear all open modals. | ||
| /// </summary> | ||
| public void ClearModals() |
There was a problem hiding this comment.
Does the HideModal method cause a modal to be removed from the stack? If so doesn't the foreach cause a crash by continuing to iterate? Maybe instead the loop should check if the stack is empty and if not attempt to close the top most one.
If this doesn't remove the modals in the stack, then I'm still of the opinion that the name of this method is wrong and should really be something like ForceHideModals.
There was a problem hiding this comment.
Nah it doesn't remove from the stack, happy to rename it.
There was a problem hiding this comment.
Alright, then the foreach doesn't crash. And yeah I'd really like it to be renamed if the method doesn't even "clear" the modals from the data structure.
|
We are currently in feature freeze until the next release. |
|
What's the status on this? As far as I can quickly see my latest review comments weren't addressed yet. |
Brief Description of What This PR Does
Editors will now attempt to close all their open modals before continuing.
It will not let the editor continue if there is an exclusive modal open that refuses to close.
Closes #4470
Related Issues
Progress Checklist
Note: before starting this checklist the PR should be marked as non-draft.
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR)
styleguide.
Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.