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

Improve ConfirmDialog and automatically handle managed UI objects #810

Conversation

falko17
Copy link
Collaborator

@falko17 falko17 commented Nov 18, 2024

ConfirmDialog improvements

This moves the ConfirmDialog out of the Drawable directories, since it is used in more situations than Drawable-specific menus.

Additionally, some minor refactorings and improvements were made, notably:

  • The dialog is now more configurable. Specifically, the title, description, button texts, button icons, and button colors can all be set to custom values.
    There is also a pre-made configuration for delete confirmations.
  • The visuals of the dialog have been slightly changed. For example, the confirm button is now green by default, the dialog is slightly larger, there are icons on the buttons, and so on.
  • The dialog should now be able to handle differing text lengths better.
  • A static convenience method has been added to make it easier to ask the user confirmation questions.
    It suffices to call this static method on the class with the desired configuration (i.e., message etc.) and then await the result (see below), without the need for the caller to have access to a fitting GameObject for the dialog (or needing to handle destroying it).
  • The dialog has been turned into a PlatformDependentComponent.
  • A fade-in and fade-out animation has been implemented for the dialog.
  • Instead of utilizing callbacks, the method to show the dialog now leverages its asynchronous nature by simply waiting until the user made their choice, then returning a boolean indicating that choice.
    This should also more easily enable patterns such as early return, as is commonly done with, for example, the analogous window.confirm() method in Web development.
    As a concrete example, the following is now possible:
if (!await ConfirmDialog.ConfirmAsync(new(message)))
{
    return;
}

To give an example of the visual changes and customizability, here's a before and after:
image $\Longrightarrow$ image

Additionally, the user is now asked for confirmation before a keybinding is rebound, fixing #683.

Automatic handling of managed UI objects

This commit abstracts away a common behavior of PlatformDependentComponents (PDCs). It's now possible to place an attribute on members of PDCs like this:

[ManagedUI(toggleEnabled: true)]
private GameObject someUI;

This will cause the someUI game object to be enabled/disabled whenever the PDC it's contained in is enabled/disabled. By default, it will also cause someUI to be destroyed whenever the PDC is destroyed. Additionally, if integral: true is specified, this will work the other way around as well: The PDC will be destroyed as soon as someUI is destroyed.

It's possible to place this attribute on both GameObjects and MonoBehaviours, as well as on enumerables of such Unity objects. It will then be applied to each of the contained objects.

Existing PDCs have been refactored to use this new functionality.

This moves the ConfirmDialog out of the Drawable directories, since it
is used in more situations than Drawable-specific menus.

Additionally, some minor refactorings and improvements were made,
notably:
* The dialog is now more configurable. Specifically, the title,
description,
  button texts, button icons, and button colors can all be set to custom
  values.
  There is also a pre-made configuration for delete confirmations.
* The visuals of the dialog have been slightly changed. For example, the
 confirm button is now green by default, the dialog is slightly larger,
  there are icons on the buttons, and so on.
* The dialog should now be able to handle differing text lengths better.
* A static convenience method has been added to make it easier to
  ask the user confirmation questions.
  It suffices to call this static method on the class with the desired
  configuration (i.e., message etc.) and then await the result (see
  below), without the need for the caller to have access to a fitting
  GameObject for the dialog (or needing to handle destroying it).
* The dialog has been turned into a `PlatformDependentComponent`.
* A fade-in and fade-out animation has been implemented for the dialog.
* Instead of utilizing callbacks, the method to show the dialog now
  leverages its asynchronous nature by simply waiting until the user
  made their choice, then returning a boolean indicating that choice.
  This should also more easily enable patterns such as early return, as
  is commonly done with the analogous `window.confirm()` method in Web
  development.
This commit abstracts away a common behavior of
PlatformDependentComponents (PDCs). It's now possible to place an
attribute on members of PDCs like this:

```csharp
[ManagedUI(toggleEnabled: true)]
private GameObject someUI;
```

This will cause the `someUI` game object to be enabled/disabled whenever
the PDC it's contained in is enabled/disabled.
By default, it will also cause `someUI` to be destroyed whenever the
PDC is destroyed. Additionally, if `integral: true` is specified, this
will work the other way around as well: The PDC will be destroyed as
soon as `someUI` is destroyed.

It's possible to place this attribute on both GameObjects and
MonoBehaviours, as well as on enumerables of such Unity objects.
It will then be applied to each of the contained objects.

Existing PDCs have been refactored to use this new functionality.
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few bad patterns I found which you should check.

Assets/SEE/UI/PlatformDependentComponent.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Cyclone1337 Cyclone1337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your pull request. I didn't encounter any errors during execution.
Your feature brings many great improvements to the ConfirmDialog, and I really like the new configurations. I especially appreciate the integration via ConfirmConfiguration.

However, we should keep in mind that the Pause key is a poorly chosen key for the settings menu. Not every laptop has this key. In fact, typically would an user expect the ESC key to be used for this, as is the case in all other games.

Assets/SEE/UI/Menu/ConfirmDialog.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/Drawable/FloatValueSliderController.cs Outdated Show resolved Hide resolved
@falko17
Copy link
Collaborator Author

falko17 commented Nov 20, 2024

Thank you for the review, @Cyclone1337!

However, we should keep in mind that the Pause key is a poorly chosen key for the settings menu. Not every laptop has this key. In fact, typically would an user expect the ESC key to be used for this, as is the case in all other games.

And yes, I absolutely agree—ESC seems like a much more natural choice. Currently, ESC is used for cancelling any actions, which also makes sense to me. Perhaps we should redesign the key binding system to allow for one key to be used for multiple situations? This could also help curb the large number of keybindings we currently have.

@falko17 falko17 force-pushed the 683-new-confirmation-dialog-needed-it-should-be-used-if-a-user-has-changed-a-keyboard-action-binding branch from 0391428 to 9e0b8e1 Compare November 20, 2024 12:36
github-actions[bot]

This comment was marked as duplicate.

@falko17 falko17 force-pushed the 683-new-confirmation-dialog-needed-it-should-be-used-if-a-user-has-changed-a-keyboard-action-binding branch from 9e0b8e1 to 1b27dfc Compare November 20, 2024 12:56
github-actions[bot]

This comment was marked as duplicate.

@koschke
Copy link
Collaborator

koschke commented Nov 20, 2024

@Cyclone1337 @falko17 It looks like this PR was approved. Can I merge it?

@falko17
Copy link
Collaborator Author

falko17 commented Nov 20, 2024

It looks like this PR was approved. Can I merge it?

Yes, I think you can go ahead—I've addressed all of @Cyclone1337's review comments now. Though you can take a second look at my changes, if you want to.

@koschke koschke merged commit c48defe into master Nov 20, 2024
8 of 10 checks passed
@koschke koschke deleted the 683-new-confirmation-dialog-needed-it-should-be-used-if-a-user-has-changed-a-keyboard-action-binding branch November 20, 2024 18:24
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.

New confirmation dialog needed. It should be used if a user has changed a keyboard-action binding.
3 participants