Skip to content

add rich message component support to configuration#10225

Merged
Machine-Maker merged 1 commit into
PaperMC:masterfrom
yannicklamprecht:issue/5449-implement-richmessage-component-config-support
Mar 3, 2024
Merged

add rich message component support to configuration#10225
Machine-Maker merged 1 commit into
PaperMC:masterfrom
yannicklamprecht:issue/5449-implement-richmessage-component-config-support

Conversation

@yannicklamprecht
Copy link
Copy Markdown
Contributor

@yannicklamprecht yannicklamprecht commented Feb 6, 2024

resolves #5449

TODOS:

  • Wait for adventure 4.16.0 release
  • remove mavenLocal() and migrate to version 4.16.0
  • add changes to adventure patch

@yannicklamprecht yannicklamprecht requested a review from a team as a code owner February 6, 2024 22:03
Copy link
Copy Markdown
Member

@Machine-Maker Machine-Maker left a comment

Choose a reason for hiding this comment

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

We should probably allow the ability to pass custom MiniMessage instances for this for custom tags and whatnot.

@imDaniX
Copy link
Copy Markdown

imDaniX commented Feb 7, 2024

We should probably allow the ability to pass custom MiniMessage instances for this for custom tags and whatnot.

Having an option to pass custom ComponentSerializer in general would be helpful too.

As a side note, this will currently throw an exception if a user uses § in their string. I'm just thinking that there should be some safe railings for this case (like returning a def/null component), or at least some javadoc info mentioning the behavior.

@kashike kashike self-requested a review February 7, 2024 07:41
@yannicklamprecht
Copy link
Copy Markdown
Contributor Author

We should probably allow the ability to pass custom MiniMessage instances for this for custom tags and whatnot.

Done.

Copy link
Copy Markdown
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

I think it would be great if we split this into

getRichMessage(path)
getRichMessage(path, defaultMini)

getComponent(path, ComponentSerialiser<T, String>, defaultStr)

that way, we also enable easy fetching for alternative formats down the line.

Comment thread patches/api/0459-add-rich-message-component-support-to-configuration.patch Outdated
Comment thread patches/api/0460-add-rich-message-component-support-to-configuration.patch Outdated
Copy link
Copy Markdown
Member

@emilyy-dev emilyy-dev left a comment

Choose a reason for hiding this comment

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

Generally looks good, my couple concerns are regarding documentation mostly

Comment thread patches/api/0460-add-rich-message-component-support-to-configuration.patch Outdated
Comment thread patches/api/0460-add-rich-message-component-support-to-configuration.patch Outdated
Comment thread patches/api/0460-add-rich-message-component-support-to-configuration.patch Outdated
Comment thread patches/api/0460-add-rich-message-component-support-to-configuration.patch Outdated
@lynxplay
Copy link
Copy Markdown
Contributor

lynxplay commented Feb 9, 2024

General "lets wait and see" for PaperMC/adventure#1034, as we could use ComponentDecoder over ComponentSerialiser if possible.

I'd give it a couple days, otherwise we'll commodore it down the line.

@emilyy-dev
Copy link
Copy Markdown
Member

Also maybe the patch could/should be promoted to the adventure patch, would like to know if anyone has strong opinions against that

@yannicklamprecht
Copy link
Copy Markdown
Contributor Author

General "lets wait and see" for KyoriPowered/adventure#1034, as we could use ComponentDecoder over ComponentSerialiser if possible.

I'd give it a couple days, otherwise we'll commodore it down the line.

Waiting another week or month will not kill it. So just wait for the decoder.

Copy link
Copy Markdown
Member

@Machine-Maker Machine-Maker left a comment

Choose a reason for hiding this comment

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

Should we add set methods as well? I don't think there are for other specific types, but since we aren't hooking Component into the snakeyaml type system, maybe some set methods would be good?

Comment thread patches/api/0461-add-rich-message-component-support-to-configuration.patch Outdated
Comment thread patches/api/0461-add-rich-message-component-support-to-configuration.patch Outdated
@yannicklamprecht
Copy link
Copy Markdown
Contributor Author

Should we add set methods as well? I don't think there are for other specific types, but since we aren't hooking Component into the snakeyaml type system, maybe some set methods would be good?

Addressed with 72031ae

Comment thread patches/api/0461-add-rich-message-component-support-to-configuration.patch Outdated
Comment thread patches/api/0461-add-rich-message-component-support-to-configuration.patch Outdated
Comment thread patches/api/0461-add-rich-message-component-support-to-configuration.patch Outdated
@yannicklamprecht
Copy link
Copy Markdown
Contributor Author

Should we fixup this onto the adventure commit?

@lynxplay
Copy link
Copy Markdown
Contributor

Yea 👍 add yourself as coauthor but yea

@yannicklamprecht
Copy link
Copy Markdown
Contributor Author

Yea 👍 add yourself as coauthor but yea

Done.

@Machine-Maker Machine-Maker merged commit 9c4bb0d into PaperMC:master Mar 3, 2024
@yannicklamprecht yannicklamprecht deleted the issue/5449-implement-richmessage-component-config-support branch March 3, 2024 18:59
LeonTG pushed a commit to LeonTG/Paper that referenced this pull request May 17, 2026
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.

Add Component-Methods to FileConfigurations

9 participants