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

Standardize dialog input validation as a new class #78744

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jun 27, 2023

Since some time we have an established way to validate input in various dialogs. We use a panel with error messages, like this one:

However every dialog implemented it separately, with led to duplicate code and even some inconsistencies (like missing panel style or using wrong listing character).

This PR adds EditorValidationPanel. It serves 2 purposes:

  • displays provided messages, styling them depending on message type (ok/warning/error/info)
  • reduces boilerplate code by doing some error management automatically

EditorValidationPanel's constructor expects a vector of strings. They will serve as default "valid" messages (i.e. no errors); size of the vector determines number of labels. After panel is created, use EditorValidationPanel.update() to queue an update method. This method will first setup labels to their default state (so the status is "valid" by default and you don't need to set it each time), then a designated update callback is called. In this callback the user is supposed to use set_message() method to set the texts of the labels. The set_message() takes label index, message text and message type (ok/warning/error/info). EditorValidationPanel will automatically prefix the messages with •, assign them colors based on type and disable or enable the dialog's OK button if it was registered.

I replaced all custom validations with EditorValidationPanel, in some cases making them more consistent (although I tried to preserve the previous appearance, notably the notes without bullet points in script create dialog). This also exposed some messages that previously weren't previously used due to faulty logic. I noticed that AnimationBlendSpaceEditor also uses some similar panel with errors, but it behaves a bit differently and is not a dialog, so I left it for now (although the validation panel is capable of handling it).

EDIT:
After latest changes the messages are no longer part of the constructor. Instead you need to define them with explicit ID.

EDIT2:
Fixes #80218

@KoBeWi KoBeWi added this to the 4.x milestone Jun 27, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner June 27, 2023 12:01
@KoBeWi KoBeWi force-pushed the easy_peasy_routine_refactor_stuff branch from 84c83b8 to 9b7293f Compare June 27, 2023 12:03
@KoBeWi KoBeWi requested review from YuriSizov and removed request for a team June 27, 2023 12:10
@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Aug 4, 2023
editor/gui/editor_validation_panel.cpp Outdated Show resolved Hide resolved
editor/gui/editor_validation_panel.cpp Outdated Show resolved Hide resolved
editor/scene_create_dialog.cpp Outdated Show resolved Hide resolved
@KoBeWi KoBeWi force-pushed the easy_peasy_routine_refactor_stuff branch from 9b7293f to b79f897 Compare August 5, 2023 22:11
@KoBeWi KoBeWi requested a review from YuriSizov August 5, 2023 22:12
@KoBeWi KoBeWi force-pushed the easy_peasy_routine_refactor_stuff branch from b79f897 to 6a469ac Compare August 7, 2023 09:36
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Implementation looks sensible now. I understand your arguments and they make sense to me. I've tested all 4 dialogs a bit, and it seems to work as expected, though admittedly I didn't test every error message.

One thing that caught my eye, which isn't related to this PR, but can be addressed by it, is that the template message is colored like a successful check, when it should probably be an info message:

image

(Also apparently this is a valid filename, who knew!)


There are some small suggestions (and a typo), but LGTM otherwise.

editor/gui/editor_validation_panel.h Outdated Show resolved Hide resolved
Comment on lines +51 to +60
void EditorValidationPanel::_notification(int p_what) {
switch (p_what) {
case NOTIFICATION_THEME_CHANGED: {
theme_cache.valid_color = get_theme_color(SNAME("success_color"), SNAME("Editor"));
theme_cache.warning_color = get_theme_color(SNAME("warning_color"), SNAME("Editor"));
theme_cache.error_color = get_theme_color(SNAME("error_color"), SNAME("Editor"));
} break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be _update_theme_item_cache, though in such a small class it doesn't change much.

editor/gui/editor_validation_panel.h Show resolved Hide resolved
@KoBeWi KoBeWi force-pushed the easy_peasy_routine_refactor_stuff branch 2 times, most recently from db9b8e8 to 822bb48 Compare August 7, 2023 20:30
@KoBeWi KoBeWi force-pushed the easy_peasy_routine_refactor_stuff branch from 822bb48 to 7f41403 Compare August 8, 2023 14:14
@akien-mga akien-mga merged commit 60d6e14 into godotengine:master Aug 8, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Script Create Dialog is enormous
3 participants