From bcfb8cc00c6eccab6e27c78bc3da514186d5d9d5 Mon Sep 17 00:00:00 2001 From: Nianna <31940002+Nianna@users.noreply.github.com> Date: Sun, 13 Oct 2024 15:28:52 +0200 Subject: [PATCH] Feat/improve tag validation (#69) * Do not look for problems in unsupported tags * Clean up code in TagValueValidators * Rename TagValueValidators::defaultValidator to requiredValueValidator * Do not mark empty values in AddSongInfoDialog as invalid --- .../karedi/dialog/AddSongInfoDialog.java | 40 ++++---- .../karedi/dialog/EditFilenamesDialog.java | 2 +- .../nianna/karedi/dialog/EditTagDialog.java | 2 +- .../DuplicatedTagsConsistencyValidator.java | 17 ++-- .../nianna/karedi/song/SongChecker.java | 20 ++-- .../karedi/song/tag/TagKeyValidator.java | 21 ----- .../karedi/song/tag/TagValueValidators.java | 92 +++++++++---------- 7 files changed, 85 insertions(+), 109 deletions(-) delete mode 100644 src/main/java/com/github/nianna/karedi/song/tag/TagKeyValidator.java diff --git a/src/main/java/com/github/nianna/karedi/dialog/AddSongInfoDialog.java b/src/main/java/com/github/nianna/karedi/dialog/AddSongInfoDialog.java index 15552e5..797af23 100644 --- a/src/main/java/com/github/nianna/karedi/dialog/AddSongInfoDialog.java +++ b/src/main/java/com/github/nianna/karedi/dialog/AddSongInfoDialog.java @@ -14,17 +14,15 @@ import javafx.fxml.FXML; import javafx.scene.control.ButtonType; import javafx.scene.control.Control; -import javafx.scene.control.Dialog; import javafx.scene.control.TextField; -import org.controlsfx.validation.ValidationMessage; -import org.controlsfx.validation.ValidationSupport; +import org.controlsfx.validation.ValidationResult; +import org.controlsfx.validation.Validator; import java.util.ArrayList; import java.util.List; import java.util.Optional; -import java.util.stream.Collectors; -public class AddSongInfoDialog extends Dialog> { +public class AddSongInfoDialog extends ValidatedDialog> { @FXML private ManageableGridPane gridPane; @@ -43,8 +41,6 @@ public class AddSongInfoDialog extends Dialog> { private final FormatSpecification formatSpecification; - private ValidationSupport validationSupport = new ValidationSupport(); - public AddSongInfoDialog(FormatSpecification formatSpecification) { this.formatSpecification = formatSpecification; setTitle(I18N.get("dialog.creator.add_info.title")); @@ -105,29 +101,27 @@ private void registerValidators() { } private void registerValidator(Control control, TagKey key) { - validationSupport.registerValidator(control, TagValueValidators.forKey(key)); + Validator tagValidator = TagValueValidators.forKey(key); + Validator allowEmptyInputWrapper = (c, value) -> + value.isEmpty() ? new ValidationResult() : tagValidator.apply(c, value); + validationSupport.registerValidator(control, allowEmptyInputWrapper); } private List generateListOfValidTags() { List list = new ArrayList<>(); - List invalidFields = validationSupport.getValidationResult().getErrors().stream() - .map(ValidationMessage::getTarget) - .distinct() - .collect(Collectors.toList()); - addTag(TagKey.GAP, gapField, invalidFields).ifPresent(list::add); - addTag(TagKey.YEAR, yearField, invalidFields).ifPresent(list::add); - addTag(TagKey.LANGUAGE, languageField, invalidFields).ifPresent(list::add); - addTag(TagKey.CREATOR, creatorField, invalidFields).ifPresent(list::add); - addTag(TagKey.GENRE, genreField, invalidFields).ifPresent(list::add); - addTag(TagKey.TAGS, tagsField, invalidFields).ifPresent(list::add); + createTagIfDefined(TagKey.GAP, gapField).ifPresent(list::add); + createTagIfDefined(TagKey.YEAR, yearField).ifPresent(list::add); + createTagIfDefined(TagKey.LANGUAGE, languageField).ifPresent(list::add); + createTagIfDefined(TagKey.CREATOR, creatorField).ifPresent(list::add); + createTagIfDefined(TagKey.GENRE, genreField).ifPresent(list::add); + createTagIfDefined(TagKey.TAGS, tagsField).ifPresent(list::add); return list; } - private Optional addTag(TagKey key, TextField field, List invalidControls) { - if (!invalidControls.contains(field)) { - return Optional.of(new Tag(key, field.getText())); - } - return Optional.empty(); + private Optional createTagIfDefined(TagKey key, TextField field) { + return Optional.of(field.getText()) + .filter(value -> !value.isEmpty()) + .map(value -> new Tag(key, value)); } } diff --git a/src/main/java/com/github/nianna/karedi/dialog/EditFilenamesDialog.java b/src/main/java/com/github/nianna/karedi/dialog/EditFilenamesDialog.java index 2636a92..163809b 100644 --- a/src/main/java/com/github/nianna/karedi/dialog/EditFilenamesDialog.java +++ b/src/main/java/com/github/nianna/karedi/dialog/EditFilenamesDialog.java @@ -183,7 +183,7 @@ public void initialize() { Platform.runLater(() -> { validationSupport.registerValidator(titleField, TagValueValidators.forKey(TagKey.TITLE)); validationSupport.registerValidator(artistField, TagValueValidators.forKey(TagKey.ARTIST)); - validationSupport.registerValidator(coverExtensionField, TagValueValidators.defaultValidator()); + validationSupport.registerValidator(coverExtensionField, TagValueValidators.requiredValueValidator()); validationSupport.initInitialDecoration(); includeBackgroundCheckBox.setSelected(!hideBackground); includeVideoCheckBox.setSelected(!hideVideo); diff --git a/src/main/java/com/github/nianna/karedi/dialog/EditTagDialog.java b/src/main/java/com/github/nianna/karedi/dialog/EditTagDialog.java index e5dc962..f2ac150 100644 --- a/src/main/java/com/github/nianna/karedi/dialog/EditTagDialog.java +++ b/src/main/java/com/github/nianna/karedi/dialog/EditTagDialog.java @@ -31,7 +31,7 @@ public class EditTagDialog extends Dialog { private ValidationDecoration valueDecoration = new GraphicValidationDecoration(); private ValidationDecoration keyDecoration = new GraphicValidationDecoration(); - private Validator valueValidator = TagValueValidators.defaultValidator(); + private Validator valueValidator = TagValueValidators.requiredValueValidator(); private Validator keyValidator = Validator .createEmptyValidator(I18N.get("dialog.tag.key_required")); diff --git a/src/main/java/com/github/nianna/karedi/song/DuplicatedTagsConsistencyValidator.java b/src/main/java/com/github/nianna/karedi/song/DuplicatedTagsConsistencyValidator.java index 8b18f83..8c0afc6 100644 --- a/src/main/java/com/github/nianna/karedi/song/DuplicatedTagsConsistencyValidator.java +++ b/src/main/java/com/github/nianna/karedi/song/DuplicatedTagsConsistencyValidator.java @@ -2,13 +2,12 @@ import com.github.nianna.karedi.problem.InconsistentTagsProblem; import com.github.nianna.karedi.problem.TagProblem; +import com.github.nianna.karedi.song.tag.FormatSpecification; import com.github.nianna.karedi.song.tag.TagKey; import java.util.Map; import java.util.Optional; -import static java.util.Objects.nonNull; - public class DuplicatedTagsConsistencyValidator { private static final Map TAG_DUPLICATES = Map.of( @@ -25,14 +24,18 @@ private DuplicatedTagsConsistencyValidator() { } public static Optional validate(Song song, TagKey key) { - if (TAG_DUPLICATES.containsKey(key)) { + if (hasSupportedDuplicateKey(song, key)) { String tagValue = song.getTagValue(key).orElseThrow(); - String similarTagValue = song.getTagValue(TAG_DUPLICATES.get(key)).orElse(null); - if (nonNull(similarTagValue) && !tagValue.equals(similarTagValue)) { - return Optional.of(new InconsistentTagsProblem(key, TAG_DUPLICATES.get(key))); - } + return song.getTagValue(TAG_DUPLICATES.get(key)) + .filter(similarTagValue -> !tagValue.equals(similarTagValue)) + .map(ignored -> new InconsistentTagsProblem(key, TAG_DUPLICATES.get(key))); } return Optional.empty(); } + private static boolean hasSupportedDuplicateKey(Song song, TagKey key) { + return TAG_DUPLICATES.containsKey(key) + && FormatSpecification.supports(song.getFormatSpecificationVersion(), TAG_DUPLICATES.get(key)); + } + } diff --git a/src/main/java/com/github/nianna/karedi/song/SongChecker.java b/src/main/java/com/github/nianna/karedi/song/SongChecker.java index 0bf5a8d..2e96723 100644 --- a/src/main/java/com/github/nianna/karedi/song/SongChecker.java +++ b/src/main/java/com/github/nianna/karedi/song/SongChecker.java @@ -13,10 +13,11 @@ import com.github.nianna.karedi.problem.ProblemsCombiner; import com.github.nianna.karedi.problem.TagForNonexistentTrackProblem; import com.github.nianna.karedi.problem.TagValidationErrorProblem; +import com.github.nianna.karedi.problem.UnsupportedTagProblem; +import com.github.nianna.karedi.song.tag.FormatSpecification; import com.github.nianna.karedi.song.tag.MultiplayerTags; import com.github.nianna.karedi.song.tag.Tag; import com.github.nianna.karedi.song.tag.TagKey; -import com.github.nianna.karedi.song.tag.TagKeyValidator; import com.github.nianna.karedi.song.tag.TagValueValidators; import com.github.nianna.karedi.util.Converter; import com.github.nianna.karedi.util.ListenersUtils; @@ -92,10 +93,18 @@ public ObservableList getProblems() { private void refreshTag(Tag tag) { removeTagProblems(tag); + if (FormatSpecification.supports(song.getFormatSpecificationVersion(), tag.getKey())) { + doValidateTag(tag); + } else { + combiner.add(new UnsupportedTagProblem(tag.getKey())); + } + } + + private void doValidateTag(Tag tag) { TagKey.optionalValueOf(tag.getKey()).ifPresent(tagKey -> { - validateValue(tagKey, tag.getValue()); - performAdditionalValueValidation(tagKey); - DuplicatedTagsConsistencyValidator.validate(song, tagKey).ifPresent(combiner::add); + validateValue(tagKey, tag.getValue()); + performAdditionalValueValidation(tagKey); + DuplicatedTagsConsistencyValidator.validate(song, tagKey).ifPresent(combiner::add); }); validateTagKey(tag.getKey()); } @@ -105,9 +114,6 @@ private void onTagRemoved(Tag tag) { } private void validateTagKey(String tagKey) { - song.formatSpecificationVersion() - .flatMap(formatVersion -> TagKeyValidator.validate(tagKey, formatVersion)) - .ifPresent(combiner::add); if (MultiplayerTags.isANameTagKey(tagKey)) { MultiplayerTags.getTrackNumber(tagKey) .filter(trackIndex -> trackIndex >= song.getTrackCount()) diff --git a/src/main/java/com/github/nianna/karedi/song/tag/TagKeyValidator.java b/src/main/java/com/github/nianna/karedi/song/tag/TagKeyValidator.java deleted file mode 100644 index 2d04065..0000000 --- a/src/main/java/com/github/nianna/karedi/song/tag/TagKeyValidator.java +++ /dev/null @@ -1,21 +0,0 @@ -package com.github.nianna.karedi.song.tag; - -import com.github.nianna.karedi.problem.TagProblem; -import com.github.nianna.karedi.problem.UnsupportedTagProblem; - -import java.util.Optional; - -public abstract class TagKeyValidator { - - private TagKeyValidator() { - - } - - public static Optional validate(String tagKey, FormatSpecification formatSpecification) { - if (!FormatSpecification.supports(formatSpecification, tagKey)) { - return Optional.of(new UnsupportedTagProblem(tagKey)); - } - return Optional.empty(); - } - -} diff --git a/src/main/java/com/github/nianna/karedi/song/tag/TagValueValidators.java b/src/main/java/com/github/nianna/karedi/song/tag/TagValueValidators.java index 254fcbf..cf2f318 100644 --- a/src/main/java/com/github/nianna/karedi/song/tag/TagValueValidators.java +++ b/src/main/java/com/github/nianna/karedi/song/tag/TagValueValidators.java @@ -1,16 +1,15 @@ package com.github.nianna.karedi.song.tag; -import java.time.Year; -import java.util.Optional; - -import org.controlsfx.validation.ValidationResult; -import org.controlsfx.validation.Validator; - -import javafx.scene.control.Control; import com.github.nianna.karedi.I18N; import com.github.nianna.karedi.util.Converter; import com.github.nianna.karedi.util.ForbiddenCharacterRegex; import com.github.nianna.karedi.util.StringValidators; +import javafx.scene.control.Control; +import org.controlsfx.validation.ValidationResult; +import org.controlsfx.validation.Validator; + +import java.time.Year; +import java.util.Optional; public class TagValueValidators { @@ -18,7 +17,7 @@ private TagValueValidators() { } public static Validator forKey(String key) { - return TagKey.optionalValueOf(key).map(TagValueValidators::forKey).orElse(defaultValidator()); + return TagKey.optionalValueOf(key).map(TagValueValidators::forKey).orElse(requiredValueValidator()); } public static Validator forKey(TagKey key) { @@ -28,30 +27,10 @@ public static Validator forKey(TagKey key) { }; } - private static Validator specificForKey(TagKey key) { - switch (key) { - case BPM: - return TagValueValidators::bpmValidator; - case YEAR: - return TagValueValidators::yearValidator; - default: - return defaultValidator(); - } - } - - public static Validator defaultValidator() { + public static Validator requiredValueValidator() { return Validator.createEmptyValidator(I18N.get("validator.tag.value_required")); } - private static boolean canBeNegative(TagKey key) { - switch (key) { - case VIDEOGAP: - return true; - default: - return false; - } - } - public static Optional forbiddenCharacterRegex(TagKey key) { if (TagKey.expectsAFileName(key)) { return Optional.of(ForbiddenCharacterRegex.FOR_FILENAME); @@ -77,6 +56,26 @@ public static Optional forbiddenCharacterRegex(String key) { return TagKey.optionalValueOf(key).flatMap(TagValueValidators::forbiddenCharacterRegex); } + public static ValidationResult validate(TagKey key, String value) { + return forKey(key).apply(null, value); + } + + public static boolean hasValidationErrors(TagKey key, String value) { + return !validate(key, value).getErrors().isEmpty(); + } + + private static Validator specificForKey(TagKey key) { + return switch (key) { + case BPM -> TagValueValidators::bpmValidator; + case YEAR -> TagValueValidators::yearValidator; + default -> requiredValueValidator(); + }; + } + + private static boolean canBeNegative(TagKey key) { + return key == TagKey.VIDEOGAP; + } + private static Validator legalInputValidator(TagKey key) { if (TagKey.expectsAnInteger(key)) { if (canBeNegative(key)) { @@ -105,30 +104,25 @@ private static Validator legalInputValidator(TagKey key) { } private static ValidationResult bpmValidator(Control c, String newValue) { - ValidationResult result = ValidationResult.fromWarningIf(c, I18N.get("validator.tag.bpm_range"), Converter - .toDouble(newValue).map(value -> (value < 200 || value > 400)).orElse(false)); - return result; + return ValidationResult.fromWarningIf( + c, + I18N.get("validator.tag.bpm_range"), + Converter + .toDouble(newValue) + .map(value -> (value < 200 || value > 400)) + .orElse(false) + ); } private static ValidationResult yearValidator(Control c, String newValue) { int currentYear = Year.now().getValue(); - ValidationResult result = ValidationResult.fromWarningIf(c, I18N.get("validator.tag.year.too_big"), - Converter.toInteger(newValue).map(value -> { - return value > currentYear; - }).orElse(false)); - return result; - } - - public static ValidationResult validate(TagKey key, String value) { - return forKey(key).apply(null, value); - } - - public static boolean hasValidationErrors(TagKey key, String value) { - return validate(key, value).getErrors().size() > 0; - } - - public static boolean hasValidationWarnings(TagKey key, String value) { - return validate(key, value).getWarnings().size() > 0; + return ValidationResult.fromWarningIf( + c, + I18N.get("validator.tag.year.too_big"), + Converter.toInteger(newValue) + .map(value -> value > currentYear) + .orElse(false) + ); } }