Skip to content

Conversation

@CitralFlo
Copy link
Member

No description provided.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 17, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request introduces several key updates to the Multification library, primarily focusing on enhancing notification capabilities within Minecraft plugins. A new README.md file has been created, detailing the library's features such as customizable messages, action bar notifications, and boss bar notifications. It provides setup instructions, including how to integrate the library into projects and create custom notification classes.

Additionally, a new Maven repository for Adventure snapshots has been added to the build configuration. The example plugin has been updated to register new commands, specifically a TimerCommand that utilizes a TimerManager for countdown functionality. New classes for handling boss bar notifications, including BossBarContent, BossBarResolver, and BossBarService, have been introduced, along with enhancements to serialization and deserialization processes. The test suite has also been expanded to cover new boss bar functionalities and ensure proper serialization formats.

Possibly related PRs

  • GH-48 Boss bar cooking #48: The changes in this PR involve enhancements to the handling of boss bar notifications, which directly relates to the new features introduced in the main PR's README regarding customizable notifications, including boss bar notifications.

Suggested reviewers

  • Kryspek
  • Kamicjusz
  • Jakubk15
  • eripe14
  • VerniQ
  • imDMK
  • Kazaruma
  • vLuckyyy
  • Hyd3r1

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between dbfe28f and 3bbce28.

📒 Files selected for processing (1)
  • buildSrc/src/main/kotlin/Versions.kt (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
buildSrc/src/main/kotlin/Versions.kt (1)

5-5: Looks good! Just a quick heads-up.

Nice job updating to a stable version. It's usually a smart move. Maybe double-check if everything still works smoothly with this change?


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Outside diff range and nitpick comments (8)
buildSrc/src/main/kotlin/multification-repositories.gradle.kts (1)

13-13: Looks good! Maybe add a space?

The new repository is a nice addition. For consistency with other lines, consider adding a space after the comment slash.

-    maven("https://s01.oss.sonatype.org/content/repositories/snapshots/") // adventure snapshots
+    maven("https://s01.oss.sonatype.org/content/repositories/snapshots/") //  adventure snapshots
multification-core/src/com/eternalcode/multification/notice/resolver/bossbar/BossBarContent.java (1)

11-11: Nice job on the record!

The BossBarContent record looks good. It covers all the needed info for a boss bar.

Quick tip: You could add a short comment explaining what this record does.

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/command/timer/TimerCommand.java (1)

18-21: The execute method works, but could be more flexible.

Maybe consider allowing different timer durations?

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/command/timer/TimerManager.java (1)

24-36: Let's make the timer even better!

The tickTimer method works, but we can improve it:

  1. Maybe add a check for negative seconds?
  2. For very long timers, we might want to use a different approach.

Want help making these changes?

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/ExamplePlugin.java (1)

41-42: Command setup looks good, but could be even better!

Nice job adding the TimerCommand! It's great that you're using the plugin's resources for the TimerManager.

A small suggestion: Consider moving the TimerManager creation to a separate method. It'll make the code even easier to read!

README.md (2)

1-13: Add alt text to the banner image

Hey there! The banner looks great, but let's make it even better by adding some alt text. This helps everyone understand what's in the image, even if they can't see it.

Try updating line 3 like this:

-![](/assets/readme-banner.png)
+![Multification Banner](/assets/readme-banner.png)
🧰 Tools
🪛 Markdownlint

3-3: null
Images should have alternate text (alt text)

(MD045, no-alt-text)


99-101: Small tweaks for the final section

Great info here! Let's make it even better with a few small changes:

  1. In line 99, try: "To add messages to the configuration, create a variable in the config with class Notice or BukkitNotice."
  2. For line 101, let's remove the period at the end of the heading.

These little fixes will make your README shine even more!

Here's how you could update it:

-Setting up configuration is easy both in cdn and okaeri configs. To add messages to the configuration, create variable in config with class `Notice` or `BukktiNotice`. You can also use builder. After plugin deploy you can find messages in configuration file.
+Setting up configuration is easy both in cdn and okaeri configs. To add messages to the configuration, create a variable in the config with class `Notice` or `BukkitNotice`. You can also use a builder. After plugin deployment, you can find messages in the configuration file.

-### To see more examples open the [example plugin module](https://github.com/EternalCodeTeam/multification/tree/master/examples/bukkit).
+### To see more examples open the [example plugin module](https://github.com/EternalCodeTeam/multification/tree/master/examples/bukkit)
🧰 Tools
🪛 LanguageTool

[uncategorized] ~99-~99: You might be missing the article “a” here.
Context: ...d messages to the configuration, create variable in config with class Notice or `Bukkt...

(AI_EN_LECTOR_MISSING_DETERMINER_A)


[uncategorized] ~99-~99: You might be missing the article “the” here.
Context: ...uration, create variable in config with class Notice or BukktiNotice. You can als...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)


[uncategorized] ~101-~101: Possible missing comma found.
Context: ...in configuration file. ### To see more examples open the [example plugin module](https:...

(AI_HYDRA_LEO_MISSING_COMMA)

🪛 Markdownlint

101-101: Punctuation: '.'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

multification-okaeri/test/com/eternalcode/multification/okaeri/NoticeSerializerTest.java (1)

583-583: Quick note: Be cautious with string replacements

In the removeBlankNewLines method, removing all quotation marks might affect the expected YAML strings.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between f2e7ec4 and dbfe28f.

⛔ Files ignored due to path filters (1)
  • assets/readme-banner.png is excluded by !**/*.png
📒 Files selected for processing (19)
  • README.md (1 hunks)
  • buildSrc/src/main/kotlin/multification-repositories.gradle.kts (1 hunks)
  • examples/bukkit/src/main/java/com/eternalcode/example/bukkit/ExamplePlugin.java (2 hunks)
  • examples/bukkit/src/main/java/com/eternalcode/example/bukkit/command/timer/TimerCommand.java (1 hunks)
  • examples/bukkit/src/main/java/com/eternalcode/example/bukkit/command/timer/TimerManager.java (1 hunks)
  • examples/bukkit/src/main/java/com/eternalcode/example/bukkit/config/MessagesConfig.java (3 hunks)
  • multification-cdn/src/com/eternalcode/multification/cdn/MultificationNoticeCdnComposer.java (9 hunks)
  • multification-cdn/test/com/eternalcode/multification/cdn/NoticeComposerTest.java (14 hunks)
  • multification-core/src/com/eternalcode/multification/notice/Notice.java (4 hunks)
  • multification-core/src/com/eternalcode/multification/notice/NoticeBroadcastImpl.java (1 hunks)
  • multification-core/src/com/eternalcode/multification/notice/NoticeKey.java (2 hunks)
  • multification-core/src/com/eternalcode/multification/notice/resolver/NoticeResolverDefaults.java (2 hunks)
  • multification-core/src/com/eternalcode/multification/notice/resolver/NoticeSerdesResult.java (2 hunks)
  • multification-core/src/com/eternalcode/multification/notice/resolver/bossbar/BossBarContent.java (1 hunks)
  • multification-core/src/com/eternalcode/multification/notice/resolver/bossbar/BossBarResolver.java (1 hunks)
  • multification-core/src/com/eternalcode/multification/notice/resolver/bossbar/BossBarService.java (1 hunks)
  • multification-core/test/com/eternalcode/multification/MultificationTest.java (2 hunks)
  • multification-okaeri/src/com/eternalcode/multification/okaeri/MultificationNoticeSerializer.java (3 hunks)
  • multification-okaeri/test/com/eternalcode/multification/okaeri/NoticeSerializerTest.java (16 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md

[uncategorized] ~99-~99: You might be missing the article “a” here.
Context: ...d messages to the configuration, create variable in config with class Notice or `Bukkt...

(AI_EN_LECTOR_MISSING_DETERMINER_A)


[uncategorized] ~99-~99: You might be missing the article “the” here.
Context: ...uration, create variable in config with class Notice or BukktiNotice. You can als...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)


[uncategorized] ~101-~101: Possible missing comma found.
Context: ...in configuration file. ### To see more examples open the [example plugin module](https:...

(AI_HYDRA_LEO_MISSING_COMMA)

🪛 Markdownlint
README.md

101-101: Punctuation: '.'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


3-3: null
Images should have alternate text (alt text)

(MD045, no-alt-text)

🔇 Additional comments (58)
multification-core/src/com/eternalcode/multification/notice/resolver/bossbar/BossBarContent.java (2)

1-9: Looks good!

The package and imports are set up correctly.


13-16: Good work on the method!

The contents() method does exactly what it needs to do. It's simple and clear.

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/command/timer/TimerCommand.java (4)

1-7: Looks good!

The package and imports are set up nicely.


8-9: Good job on the annotations!

The command name and permission are clearly defined.


10-12: Nice and clean class setup!

The class and field are well-defined.


14-16: Good constructor!

It's set up correctly to initialize the timer manager.

multification-core/src/com/eternalcode/multification/notice/resolver/NoticeSerdesResult.java (2)

4-4: Looks good!

The new import is needed for the Section record. Nice job adding it.


36-41: Can you explain the Section record?

The new Section record looks okay, but I'm not sure why it's needed. Could you add a comment explaining what it's for? Also, why does anyElements() return an empty list?

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/command/timer/TimerManager.java (3)

1-8: Good job on the setup!

The class name and imports look good. They match what the class does.


10-18: Nice work on the constructor!

The fields and constructor look good. They're set up correctly.


20-22: Good start to the timer!

The startTimer method looks simple and does its job well.

multification-core/src/com/eternalcode/multification/notice/NoticeKey.java (2)

5-5: New import looks good!

The import for BossBarContent fits right in with the other imports.


21-21: Nice addition of BOSS_BAR constant!

The new BOSS_BAR constant matches the style of the other constants. Good job keeping things consistent!

multification-core/src/com/eternalcode/multification/notice/resolver/NoticeResolverDefaults.java (2)

4-5: New imports look good!

These imports will help with the new boss bar feature. Nice addition!


31-31: Great job adding the boss bar resolver!

The new boss bar resolver fits in nicely with the other resolvers. It's a good addition to the registry.

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/config/MessagesConfig.java (3)

6-6: New imports look good!

The added imports for BossBar and Duration are needed for the new timer feature. Nice job!

Also applies to: 10-10


25-25: Cool gradient effect!

The new subtitle looks great with the gradient colors. It's a nice touch that makes the message pop!


35-38: Awesome boss bar timer!

The new bossbarTimer looks great! It adds a cool feature for showing timers to players. Good job on using a placeholder for the time too!

examples/bukkit/src/main/java/com/eternalcode/example/bukkit/ExamplePlugin.java (2)

5-6: New imports look good!

The TimerCommand and TimerManager imports are in the right place. They'll help with the new timer feature.


5-6: Great job adding the timer feature!

You've smoothly integrated the new timer functionality into the plugin. The imports and command setup look good. This update will give users more options, which is awesome!

Also applies to: 41-42

README.md (3)

15-30: Great job on the introduction!

The intro looks spot-on! It's clear and covers all the key points about what Multification can do. Nice work!


32-39: Setup instructions look good

The setup part is clear and easy to follow. You've made it simple for people to add Multification to their projects. Well done!


41-97: Code examples are helpful

These examples are great! They show exactly how to set up and use Multification in a project. This will be super helpful for users.

multification-core/test/com/eternalcode/multification/MultificationTest.java (1)

104-104: Nice touch with the test name!

Adding a display name makes the test purpose clearer. Good job!

multification-okaeri/src/com/eternalcode/multification/okaeri/MultificationNoticeSerializer.java (1)

61-63: Handling 'Section' results looks good!

The addition for serializing NoticeSerdesResult.Section is correct.

multification-core/src/com/eternalcode/multification/notice/Notice.java (2)

107-130: Great addition of bossBar methods!

The new static methods for boss bar notifications are a valuable enhancement.


218-233: Builder methods for bossBar look good!

The builder now offers flexible options for creating boss bar notices.

multification-core/src/com/eternalcode/multification/notice/NoticeBroadcastImpl.java (1)

325-328: Nice addition of the getThis() method!

Including this method helps with method chaining and keeps the code clean.

multification-cdn/test/com/eternalcode/multification/cdn/NoticeComposerTest.java (9)

331-333: Great addition of boss bar tests

Adding the ConfigBossbarWOBuilder class improves test coverage for boss bar serialization without using the builder.


335-359: Test methods are well-structured

The test serializeBossBarSectionWithAllPropertiesWOBuilder effectively verifies serialization with all properties.


361-387: Good job testing without progress

Your tests for boss bar serialization without progress ensure correct handling when progress isn't specified.


389-417: Effective testing without overlay

The ConfigBossbarWithoutOverlayWOBuilder class and its test confirm proper behavior when overlay is omitted.


419-447: Comprehensive tests for minimal configurations

Including tests without both progress and overlay enhances confidence in default behavior handling.


449-478: Nice use of builder pattern

The ConfigBossBarWithAllProperties class demonstrates effective use of the builder for boss bar configuration.


481-509: Thorough testing without progress

Your test ensures that boss bar serialization works correctly when progress isn't provided.


511-542: Solid verification without overlay

Testing boss bar serialization without an overlay confirms the robustness of default settings.


544-574: Ensuring functionality with minimal settings

The test for boss bar without progress and overlay confirms proper handling of minimal configurations.

multification-okaeri/test/com/eternalcode/multification/okaeri/NoticeSerializerTest.java (16)

322-324: Great addition of the ConfigBossbarWOBuilder class!

The boss bar notice configuration is clear and easy to understand.


326-339: Test method looks solid

The serializeBossBarWOBuilderWithAllProperties test correctly verifies serialization with all properties.


353-354: Nice work on ConfigBossbarWithoutProgressWOBuilder

This class effectively tests serialization without the progress property.


356-369: Test covers the scenario well

The serializeBossBarWOBuilderWithoutProgressProperty test checks serialization without the progress property as expected.


381-382: ConfigBossbarWithoutOverlayWOBuilder looks good

You're properly testing the absence of the overlay property.


384-396: Test method is well-implemented

The serializeBossBarWOBuilderWithoutOverlayProperty test verifies serialization without the overlay property effectively.


412-413: ConfigBossbarWithoutProgressAndOverlayWOBuilder is well-defined

This class tests serialization without both progress and overlay properties nicely.


415-426: Test accurately checks serialization

The serializeBossBarWOBuilderWithoutProgressAndOverlayProperties test validates the serialization without progress and overlay properties.


441-445: Good use of the builder pattern in ConfigBossBar

This enhances readability and makes the code easy to maintain.


447-470: Test effectively verifies boss bar serialization

The serializeBossBarSectionWithAllProperties test checks all properties are serialized correctly.


472-476: ConfigBossBarWithoutProgress is well-crafted

Testing without the progress property is handled well here.


478-500: Test properly verifies absence of progress property

The serializeBossBarSectionWithoutProgressProperty test functions as expected.


502-506: ConfigBossBarWithoutOverlay looks good

This configuration tests serialization without the overlay property effectively.


508-533: Test correctly checks serialization without overlay

The serializeBossBarSectionWithoutOverlayProperty test validates the intended behavior.


535-539: ConfigBossBarWithoutProgressAndOverlay is well-designed

You're covering the scenario without both progress and overlay properties nicely.


541-565: Test effectively handles serialization without progress and overlay

The serializeBossBarSectionWithoutProgressAndOverlayProperties test confirms the expected serialization.

multification-cdn/src/com/eternalcode/multification/cdn/MultificationNoticeCdnComposer.java (5)

14-14: Imports look good!

The necessary classes are properly imported.

Also applies to: 16-16, 27-27


46-46: Method signature updated appropriately.

Adding String key enhances the method's functionality.


110-114: Great addition for handling sections.

This improves the serialization logic.


139-146: New method toNamedSection is a helpful addition.

It enhances how sections are serialized.


215-218: Double-check result.project() usage.

Please ensure it returns the intended result in this context.


public class BossBarService {

private final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(8);
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using fewer threads.

An 8-thread pool might be more than necessary. A smaller pool could work just as well.

}

Duration remaining = Duration.between(Instant.now(), expiration);
float progress = 1 - (float) remaining.getSeconds() / duration.getSeconds();
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid division by zero.

If duration.getSeconds() is zero, this will cause an error. Please add a check to prevent this.

Comment on lines 68 to 70
BossBar.Color color = BossBar.Color.valueOf(sectionResult.elements().getOrDefault("color", DEFAULT_COLOR));
Duration duration = DurationParser.TIME_UNITS.parse(sectionResult.elements().getOrDefault("duration", DEFAULT_DURATION));

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle invalid color input

If the 'color' value is invalid, it might cause an exception. Let's add a check to use the default color when needed.

Comment on lines 75 to 80
if (sectionResult.elements().get("overlay") != null) {
overlay = Optional.of(BossBar.Overlay.valueOf(sectionResult.elements().get("overlay")));
}
else {
overlay = Optional.empty();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle invalid overlay input

An invalid 'overlay' value could lead to exceptions. It's a good idea to default to a safe value if parsing fails.

Comment on lines 93 to 95
double progress = Double.parseDouble(rawProgress);
return OptionalDouble.of(progress);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure 'progress' is within valid range

Let's validate that 'progress' is between 0.0 and 1.0 to prevent unexpected behavior.

Comment on lines 112 to 120
if (value instanceof Map<?, ?> mapValue) {
NoticeDeserializeResult<?> noticeResult = this.noticeRegistry.deserialize(key, new NoticeSerdesResult.Section((Map<String, String>) mapValue))
.orElseThrow(() -> new UnsupportedOperationException(
"Unsupported notice key: " + key + " with values: " + mapValue));

this.withPart(builder, noticeResult);
continue;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure safe casting in deserialization.

Casting mapValue to (Map<String, String>) may cause issues if the map doesn't contain String keys and values. Please check the map's contents before casting to prevent potential errors.

Comment on lines 240 to 254
private Result<Blank, Exception> appendEntry(Entry entry, Notice.Builder builder) {
String value = this.deserializePiece(entry.getValue());
String key = entry.getName();

NoticeSerdesResult.Single result = new NoticeSerdesResult.Single(value);
Optional<NoticeDeserializeResult<?>> optional = this.noticeRegistry.deserialize(key, result);

if (optional.isEmpty()) {
return Result.error(new IllegalStateException("Unsupported notice type: " + key));
}

this.withPart(builder, optional.get());
return Result.ok();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider simplifying similar code in appendEntry and appendSection.

Combining shared logic might make the code cleaner.

Also applies to: 255-286

@CitralFlo CitralFlo closed this Oct 17, 2024
@CitralFlo CitralFlo deleted the bump-down-version branch October 17, 2024 17:42
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.

4 participants