search

APDevTeam / Movecraft

The original movement plugin for Paper. Reloaded. Again.
GNU General Public License v3.0
122 stars 79 forks source link

Change SignTranslateEvent to use Adventure #688

Open DerToaster98 opened 2 months ago

DerToaster98 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

TylerS1066 commented 2 months ago

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 commented 2 months ago

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.