RFC: Teleport to players' last locations in worlds #1794

qwylz · 2017-02-01T20:43:31Z

First, thank you for the great plugin. Recently I tested several plugins for managing multiple worlds and Multiverse-Core handily came out on top. Thank you!

Secondly, this is not actually a request to merge code at this time--this code is definitely not ready to be merged. I've submitted this PR more as an early request for comments and guidance as I work to get the code ready for a real pull request. Will you please read through this submission and the accompanying code and provide any suggestions and guidance necessary for me to get this ready for a true pull request?

This code is intended to add a new, non-default feature to Multiverse-Core. When enabled, the feature makes it so that when a player leaves a world, his/her last position in the world is saved. Then, when teleporting back to the world using the teleport syntax of just the bare world name (i.e. no "w:" or "e:" prefix), the player is teleported to his/her previously saved location in the world rather than to the world's spawn point. If the player has no saved location for the world, or if the save cannot be read/parsed for some reason, the player is teleported to the world's spawn point as a fallback measure. The behavior when using the "w:" or "e:" syntaxes is unchanged (i.e. always teleports to the world's spawn location or an exact location, respectively).

In it's current form, the code works, but is in an hackish, "can I even get this to work" kind of state. I need to take care of the following items before it's really ready for a true pull request:

Currently, it's not possible to disable the feature. I still need to add a configuration property for the plugin that will control whether the feature is enabled or disabled. I plan to have it default to being disabled, so that, by default, there is no change in behavior.
The code is currently embedded in several different classes. Very ugly. I need to refactor the code into it's own class(es) (and an exception) which will be utilized by the various listener and command classes.
I haven't done anything about permissions, yet. There should probably be separate permissions for teleporting to a world at the player's saved location versus teleporting to the world's spawn point or exact locations.
The code currently saves a players location in a world when leaving a world either via teleporting or the player quitting. It does not save players' locations when the server is stopped. I'm not sure how to listen for that. Is there a bukkit event? A player's location is also not saved when he/she is kicked from a world. I figure that if someone is misbehaving enough to be kicked, his/her location doesn't need to be save (however, if there was a previously saved location for the player, that saved information is not deleted).
While my day job is software development, my experience is in other languages. I've never written a line of Java until writing the code for this feature yesterday. Hopefully, I haven't abused Java too badly, and there's likely more Java-like ways to do some of what this code does. Suggestions are welcome, please.

One final note: I have developed this code under Oracle's official Java 8 JDK. Hopefully, I haven't used any constructs or patterns that cause problems on earlier versions or other flavors of Java.

Again, thank you for the great plugin. I hope that I can get the code for this feature in order so that it can be merged. Please let me know of any guidance and suggestions that you have.

dumptruckman · 2017-02-01T21:39:44Z

Rather than a global config option, perhaps consider just using a single simple default: false permission for whether they should teleport to the world last location. This way the option can be per player and is still off by default.
I'll have to look over the code and make comments there.
See 1
PlayerQuitEvent should cover all cases of the player leaving the server. PlayerChangeWorldEvent should cover all cases of players changing world. I would suggest making a simple yaml data file for this information. Just be sure to associate the location data with player UUID.

dumptruckman · 2017-02-01T21:42:08Z

src/main/java/com/onarandombox/MultiverseCore/destination/DestinationFactory.java

+                }
+                if (yc != null) {
+                    try {
+                        if (! yc.isSet("schema"))


YamlConfiguration should have like getInteger() for this.

Ah ha! Yes, I see that now. When I first looked at the docs for YamlConfiguration, I didn't realize that the list of methods was not in alphabetic order. I stopped when I saw the .get and .isSet methods and no other .get or .is methods. I've refactored to use the .isInt, .isDouble, .getInt, and .getDouble methods. Thanks for the tip!

dumptruckman · 2017-02-01T21:42:51Z

src/main/java/com/onarandombox/MultiverseCore/destination/DestinationFactory.java

+                    yc = null;
+                    // fall through to use world spawn location
+                }
+                if (yc != null) {


It's been a while since I've looked at MV's code but I think we have a simpler way to serialize/deserialize location data.

dumptruckman · 2017-02-01T21:43:33Z

src/main/java/com/onarandombox/MultiverseCore/destination/DestinationFactory.java

     *
     * @return A non-null MVDestination
     */
-    public MVDestination getDestination(String destination) {
+    public MVDestination getDestination(String destination, Player player) {


Probably better you leave the old method in just add this as a new method. This way we can preserve backwards compatibility.

If you look a little later in the diff, you'll see that's what I already did. I didn't want to break the existing API. The original method is just a shim that supports the original API. It simply calls the new API with null for player and the behavior is the same as the old behavior and still keeps the code DRY.

qwylz · 2017-02-07T00:33:34Z

Having the ability to enable/disable this on a per-player basis is a good idea. It allows for more use cases than my simple case which I had not thought of. I think that using the permissions system might be a good way to handle how to enable/disable for each player. I think that's what you were suggesting, as well (yes?).

I still think there will need to be a global default enable/disable item, otherwise I don't think my use case is achievable (unless I misunderstand what you're meaning). My use case is to specifically allow this behavior for all users without having to enable it for each new player that comes along. Having the global setting and then per-user permissions allows for both whitelisting and blacklisting use cases. Out of the box, it would be in whitelisting mode so as to retain backward compatibility.

dumptruckman · 2017-02-07T05:59:21Z

If you use only permissions (white list as you say) rather than also having a configuration value you can just give your default user the permission in your permission manager. Even better, if you want it for everyone no questions you can set it to default: true in permissions.yml in your server root.

qwylz · 2017-02-07T21:36:21Z

I think my confusion comes from not having studied MV's permissions model, thus far. Now that I've started to look at it, I think it's starting to make sense, but I need to do so more exploration on how the permissions all work together.

To that end, I have a question:

If a player has multiverse.teleport.self.e, it enables them to use /mv tp e: in/into any and all worlds for which they have multiverse.access.WORLDNAME, right? In other words, if a player has the .e permission they can teleport to exact destinations in all worlds to which they have the corresponding .access permission and there is no way to say that a player can exact teleport in/to worlds A and C, but not B (but still allow the player access to B via other teleport methods), right?

dumptruckman · 2017-02-08T12:01:52Z

Yes, you have that right.

qwylz · 2017-02-11T00:18:10Z

Now that I understand how the permissions all work, I understand what you meant about using permissions rather than needing a global configuration item. It's a great idea! I've got that all sorted out and implemented now. I think I've now taken care of all the outstanding items.

There are three new permissions used:

multiverse.teleport.self.l
multiverse.teleport.other.l
multiverse.save.lastlocations

The first two are used to control who can use the new last location destination functionality. The /mv tp command, when given a destination that does not specify a location type identifier, will intelligently figure out whether to use a last location destination or just the original world spawn destination based on permissions.

If a player has either multiverse.teleport.self.l or multiverse.save.lastlocations his/her last location in a world will be saved (otherwise it will not be saved). This makes it possible to make it so a user's last locations are saved without allowing them to teleport to their last locations (by just giving the user multiverse.save.lastlocations).

I think I have this all ready to be a real PR. Can this code be merged?

P.S.
Sorry, I see that I spelled refactor wrong in my last commit. :-(

dumptruckman

Very nice overall. There's still a few things that need addressing before we can merge.

dumptruckman · 2017-02-11T03:24:10Z

src/main/java/com/onarandombox/MultiverseCore/MultiverseCore.java

@@ -258,6 +260,8 @@ public void onLoad() {
        // Setup our SafeTTeleporter
        this.safeTTeleporter = new SimpleSafeTTeleporter(this);
        this.unsafeCallWrapper = new UnsafeCallWrapper(this);
+        // Set up MVPlayerLocation
+        MVPlayerLocation.init(this);


Is there any way we can avoid static initialization like this?

dumptruckman · 2017-02-11T03:30:38Z

src/main/java/com/onarandombox/MultiverseCore/destination/LastLocationDestination.java

+        }
+
+        player = (Player) e;
+        this.location = MVPlayerLocation.getPlayerLastLocation(player, world);


Can this location be set in setDestination using a non static method? Such as plugin.getPlayerLocationManager().getPlayerLastLocation... or something. Just trying to avoid static stuff like this.

dumptruckman · 2017-02-11T03:36:17Z

src/main/java/com/onarandombox/MultiverseCore/listeners/MVPlayerListener.java

-        this.plugin.removePlayerSession(event.getPlayer());
+        Player player = event.getPlayer();
+        if (this.plugin.getMVPerms().hasPermission(player, "multiverse.teleport.self."
+                + LastLocationDestination.getID(), true)


Theoretically the third arg here should be false, but it isn't used anymore so I guess it doesn't matter.

dumptruckman · 2017-02-11T03:37:00Z

src/main/java/com/onarandombox/MultiverseCore/listeners/MVPlayerListener.java

+     * This method is called when a player teleports anywhere.
+     * @param event The Event that was fired.
+     */
+    @EventHandler(priority = EventPriority.MONITOR)


You should add an ignoreCancelled = true here.

dumptruckman · 2017-02-11T03:48:12Z

src/main/java/com/onarandombox/MultiverseCore/listeners/MVPlayerListener.java

@@ -315,7 +339,9 @@ private void sendPlayerToDefaultWorld(final Player player) {
            new Runnable() {
                @Override
                public void run() {
-                    player.teleport(plugin.getMVWorldManager().getFirstSpawnWorld().getSpawnLocation());
+                    LastLocationDestination destination = new LastLocationDestination();


Why are there no permission checks here?

dumptruckman · 2017-02-11T03:54:51Z