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

Change SignTranslateEvent to use Adventure #688

Open
DerToaster98 opened this issue Aug 10, 2024 · 7 comments
Open

Change SignTranslateEvent to use Adventure #688

DerToaster98 opened this issue Aug 10, 2024 · 7 comments

Comments

@DerToaster98
Copy link
Contributor

Is your feature request related to a problem? Please describe.
All old setLine(), getLine() etc methods that operate on strings are deprecated and the new methods use Components. We should do the same in our event.

In addition since 1.20 signs have multiple sides you can write on, so in that version we need to call that event per side, otherwise we will lose data.

Describe the solution you'd like
Migrate the fields and methods to use Components and as long as 1.18 is being used, define a wrapper class that actually fills and handles the relevant parts in Translate and Rotate tasks.

@TylerS1066
Copy link
Contributor

I think the days of 1.18.2 support are numbered, so if it's cleaner to make a 1.20+ compatible system with no support for 1.18.2, I'd prefer that.

@DerToaster98
Copy link
Contributor Author

DerToaster98 commented Aug 10, 2024

It is cleaner to remove 1.18 support. For a comparable system, i have the SignWrapper class in my sign PR, it essentially abstracts away the relevant calls to an event or sign side

@DerToaster98
Copy link
Contributor Author

So, just to get this straight: I do have permission to remove 1.18.x stuff from the repo via a PR that fixes this issue?

@TylerS1066
Copy link
Contributor

For now I'd like to keep 1.18.2 support, so if possible I'd like for this change to be separated from the sign PR. With that said, please feel free to open up a PR which removes 1.18.2 and implements this, it just may not be merged immediately.

Originally the plan was to keep 1.18.2 support until Minecraft 1.22 drops, but this issue is a good reason to accelerate that timeline and drop it earlier. However, AP is still running 1.18.2 and I will need to update it before we drop support.

@DerToaster98
Copy link
Contributor Author

Alright, do you mind if i use my "adapter" that i already have within the PR then?

@TylerS1066
Copy link
Contributor

Alright, do you mind if i use my "adapter" that i already have within the PR then?

Sure, but put in some comments mentioning that when 1.18.2 is dropped, they should be removed as well (specifically include "1.18.2" so that a search all finds them).

@DerToaster98
Copy link
Contributor Author

Alright, do you mind if i use my "adapter" that i already have within the PR then?

Sure, but put in some comments mentioning that when 1.18.2 is dropped, they should be removed as well (specifically include "1.18.2" so that a search all finds them).

Sounds good, i'll leave a deprection note and a comment in the relevant places.

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

No branches or pull requests

2 participants