-
Notifications
You must be signed in to change notification settings - Fork 23
Native Support for Adventure Library #60
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
base: dev
Are you sure you want to change the base?
Conversation
|
This is cool! |
|
Hey, thanks a lot for your contribution! I have a lot of stuff going on in my private life lately but I will try to find some time this weekend to have a look at it. |
Exlll
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the things that I commented: I generally try to stay below 80 chars per line. 90 chars per line is a hard limit except for some edge cases and in tests. Could you please reformat your code so that the line length is within these limits?
Also please prefix all top-level classes with Adventure, i.e. AdventureComponentSerializer, AdventureComponentFormat, etc.
Overall pretty nice work 👍
configlib-adventure/src/main/java/de/exlll/configlib/ComponentFormat.java
Outdated
Show resolved
Hide resolved
configlib-adventure/src/main/java/de/exlll/configlib/ComponentFormat.java
Outdated
Show resolved
Hide resolved
| * @param formats the formats to use, in order of preference | ||
| */ | ||
| public ComponentSerializer(ComponentFormat... formats) { | ||
| this(Arrays.asList(formats), Arrays.asList(formats)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should make a defensive copy here, too. Calling asList is not enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already called List.copyOf in another constructor. This constructor just converts the array to a List to pass it to the other constructor. or I miss understanding here?
configlib-adventure/src/main/java/de/exlll/configlib/ComponentSerializer.java
Outdated
Show resolved
Hide resolved
configlib-adventure/src/main/java/de/exlll/configlib/ComponentSerializer.java
Outdated
Show resolved
Hide resolved
configlib-adventure/src/test/java/de/exlll/configlib/KeySerializerTest.java
Outdated
Show resolved
Hide resolved
configlib-core/src/test/java/de/exlll/configlib/SerializersTest.java
Outdated
Show resolved
Hide resolved
configlib-velocity/src/main/java/de/exlll/configlib/ConfigLib.java
Outdated
Show resolved
Hide resolved
configlib-adventure/src/main/java/de/exlll/configlib/SoundSerializer.java
Outdated
Show resolved
Hide resolved
| public Sound deserialize(String element) { | ||
| String[] parts = element.split(DELIMINATOR); | ||
| float pitch = 1.0f; | ||
| float volume = 1.0f; | ||
| Sound.Source source = defaultSource; | ||
|
|
||
| int endIndex = parts.length - 1; | ||
|
|
||
| // Try to parse source | ||
| if (endIndex >= 0) { | ||
| try { | ||
| source = Sound.Source.valueOf(parts[endIndex]); | ||
| endIndex--; | ||
| } catch (IllegalArgumentException ignored) { | ||
| } | ||
| } | ||
|
|
||
| // Try to parse volume and pitch | ||
| if (endIndex >= 0) { | ||
| Float lastFloat = tryParseFloat(parts[endIndex]); | ||
| if (lastFloat != null) { | ||
| boolean hasSecondFloat = false; | ||
| if (endIndex > 0) { | ||
| Float secondLastFloat = tryParseFloat(parts[endIndex - 1]); | ||
| if (secondLastFloat != null) { | ||
| volume = lastFloat; | ||
| pitch = secondLastFloat; | ||
| endIndex -= 2; | ||
| hasSecondFloat = true; | ||
| } | ||
| } | ||
|
|
||
| if (!hasSecondFloat) { | ||
| pitch = lastFloat; | ||
| endIndex--; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Key key = buildKey(parts, endIndex); | ||
| return Sound.sound(key, source, volume, pitch); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parsing logic seems overly complex. Perhaps you can replace this with a Pattern? Something like this:
Pattern pattern = Pattern.compile(
"""
^(?<key>[a-zA-Z:]+)\
(?:\\s+(?<volume>\\d+(?:\\.\\d+)?)?)?\
(?:\\s+(?<pitch>\\d+(?:\\.\\d+)?)?)?\
(?:\\s+(?<source>MASTER|AMBIENT|MUSIC)?)?\
\\s*$\
""");
Matcher matcher = pattern.matcher("minecraft 0.4 MASTER");
System.out.println(matcher.matches());
System.out.println(matcher.group("key"));
System.out.println(matcher.group("volume"));
System.out.println(matcher.group("pitch"));
System.out.println(matcher.group("source"));There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by using regex (provide by you) with dynamic generation to respect DELIMINATOR field and Sound.Source enum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I fallback to defaultSource if input source is not valid or throw an Exception? The current way is fallback to defaultSource.
Fix: Exlll#60 (comment) Fix: Exlll#60 (comment) Fix: Exlll#60 (comment) Fix: Exlll#60 (comment) Fix: Exlll#60 (comment) Fix: Exlll#60 (comment) Fix: Exlll#60 (comment) Fix: Exlll#60 (comment) Fix: Exlll#60 (comment) Fix: Exlll#60 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds first-party Adventure API type support to ConfigLib via a new configlib-adventure module, and wires it into the Paper and Velocity plugin modules for convenient defaults.
Changes:
- Introduces
configlib-adventurewith serializers/default-registration forComponent,Key, andSound, plus new tests. - Integrates Adventure defaults into Paper/Velocity (
PAPER_DEFAULT_PROPERTIES,VELOCITY_DEFAULT_PROPERTIES) and updates their Gradle dependencies. - Improves Windows portability in core tests and updates README documentation for the new module.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle.kts | Adds the new configlib-adventure module to the Gradle build. |
| configlib-adventure/build.gradle.kts | Declares Adventure-related dependencies and test dependencies for the new module. |
| configlib-adventure/src/main/java/de/exlll/configlib/AdventureConfigLib.java | Adds a helper to register Adventure serializers onto a properties builder. |
| configlib-adventure/src/main/java/de/exlll/configlib/AdventureComponentSerializer.java | Implements Component serialization/deserialization across multiple formats. |
| configlib-adventure/src/main/java/de/exlll/configlib/AdventureComponentFormat.java | Defines format detection/matching for Component strings. |
| configlib-adventure/src/main/java/de/exlll/configlib/AdventureKeySerializer.java | Implements Key serialization/deserialization with optional default namespace behavior. |
| configlib-adventure/src/main/java/de/exlll/configlib/AdventureSoundSerializer.java | Implements compact Sound string serialization/deserialization. |
| configlib-adventure/src/test/java/de/exlll/configlib/AdventureTestConfiguration.java | Adds a configuration type covering many Adventure values for round-trip testing. |
| configlib-adventure/src/test/java/de/exlll/configlib/AdventureConfigurationTests.java | Adds round-trip tests for saving/loading Adventure-backed configurations. |
| configlib-adventure/src/test/java/de/exlll/configlib/AdventureComponentTests.java | Adds tests validating multi-format Component deserialization order. |
| configlib-adventure/src/test/java/de/exlll/configlib/AdventureKeySerializerTest.java | Adds tests for Key serialization/deserialization behavior. |
| configlib-adventure/src/test/java/de/exlll/configlib/AdventureSoundSerializerTest.java | Adds tests for Sound serialization/deserialization behavior. |
| configlib-paper/src/main/java/de/exlll/configlib/ConfigLib.java | Adds PAPER_DEFAULT_PROPERTIES and refactors builder construction. |
| configlib-paper/build.gradle.kts | Adds dependency on configlib-adventure and ensures its checks run for plugin compilation. |
| configlib-velocity/src/main/java/de/exlll/configlib/ConfigLib.java | Adds VELOCITY_DEFAULT_PROPERTIES for Adventure-enabled defaults. |
| configlib-velocity/build.gradle.kts | Adds dependency on configlib-adventure and ensures its checks run for plugin compilation. |
| configlib-core/src/testFixtures/java/de/exlll/configlib/TestUtils.java | Adds isWindows() and refactors Windows path handling helper. |
| configlib-core/src/test/java/de/exlll/configlib/SerializersTest.java | Updates expected file-path strings to be platform-specific. |
| buildSrc/src/main/kotlin/libs-config.gradle.kts | Removes cross-project compileJava → core:check dependency from libs config. |
| README.md | Documents Adventure module usage and Paper default properties updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
configlib-adventure/src/main/java/de/exlll/configlib/AdventureComponentFormat.java
Show resolved
Hide resolved
configlib-adventure/src/main/java/de/exlll/configlib/AdventureComponentSerializer.java
Show resolved
Hide resolved
configlib-adventure/src/main/java/de/exlll/configlib/AdventureConfigLib.java
Show resolved
Hide resolved
configlib-paper/src/main/java/de/exlll/configlib/ConfigLib.java
Outdated
Show resolved
Hide resolved
configlib-core/src/test/java/de/exlll/configlib/SerializersTest.java
Outdated
Show resolved
Hide resolved
configlib-adventure/src/test/java/de/exlll/configlib/AdventureKeySerializerTest.java
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
This pull request addresses issue #29, which I opened previously, to add native support for the Adventure API Component.
Changes in this PR
configlib-adventure, shaded into the Paper and Velocity modules (since both natively support Adventure).Component,Key, andSound.Notes
I noticed that running
gradle testat the root or within any module only executed tests in thecoremodule. I am not sure whether this behavior was intentional.I modified the behavior so that:
If the previous test configuration was intentional, please let me know and I will revert the changes accordingly.
I also fixed two failing tests in the
coremodule that were caused by Windows-specific behavior (as shown in the screenshot). All core module tests now pass on Windows.If you are not satisfied with the changes mentioned above, I am willing to revert or adjust them as needed.