search

potatolain / TravelPortals

A portal system for Bukkit for Minecraft
BSD 2-Clause "Simplified" License
3 stars 2 forks source link

A few small 1.13.1 (2.4-SNAPSHOT) fixes #21

Closed potatolain closed 5 years ago

potatolain commented 5 years ago

Takes care of a couple bugs that came up in #20.

Hey @Phoenix616 if you get some time over the next few days, would you mind giving this a sanity check? At this point you probably know the codebase better than I do!

I'm also not sure how you do releases now that you have this integrated into your CI. I can bump the version in pom.xml and/or update the changelog if you'd like.

I'm attaching a jar with the updated code to the original ticket to get the reporter up and running.

Also, thanks for taking the time to set this up with maven; it's way easier to work with than what I had!

Phoenix616 commented 5 years ago

Thanks for looking into that, not sure why I wasn't watching this repo :/

My plan for releases would be the following: Put test builds for major updates (like the alpha one for 1.13) on the bukkit page so that people not looking at each individual jenkins build notice the update and once we think it's stable enough release that as a proper release on Bukkit. The actual number in the version shouldn't change from test to release build but the release wouldn't include -SNAPSHOT in the version. (Which in the maven-ecosystem indicates that something it a rolling dev build and not a stable release)

Regarding the default name: I think the best solution would be to change the internal portal map so that it uses the location string as the key in the PortalStorage (so location string -> WarpLocation mapping) and only map portal name to the location string for portals that already have a name. That should work around the issue with portals that have no name a bit better and maybe even allow a feature where portals private to different players can share the same name?

Besides that: I kinda like the idea of every portal getting assigned a name based on its coordinates (which should include the world, otherwise it could conflict again :wink:) by default as players could directly use the portal without having to name it manually and therefore getting rid of the issue of thinking of a name (and hoping that it wasn't used yet), maybe we could make that a config option as some people might not want their world names leaked for whatever reason.

potatolain commented 5 years ago

I think that all makes sense! No time tonight, but I'll see what I can do over this weekend.

Good catch on the world name btw; that would've caused some weird bugs down the road!

I had intentionally hidden portals with the default name to stay in line with current behavior, but perhaps there's an argument to be made for showing them.

potatolain commented 5 years ago

Just added a new commit that does it the more optimal way you suggested.

For now I just reverted the change to add default names to all portals. While I think it's a cool idea, I kinda just want to get this working again for now.

I'd like to merge this, let jenkins do its thing, then get the new build up on bukkitdev (same 2.4-SNAPSHOT version for now I guess). Thoughts?

Phoenix616 commented 5 years ago

Looks good. i've gone ahead and merged your branch into master so that jenkins will hopefully pickup that change.