Implemented plugin update folder

[OakLoaf]

The goal of this PR was to make progress on a plugin update folder for Velocity.

Tasks:

• Initial implementation
• Discuss nullability annotation for VelocityConfiguration#getUpdateFolderName
• Discuss default config value
• Discuss what the config value should be to entirely disable the update folder
• Test final implementation

closes #809
based on #842

[OakLoaf]

Before marking this as ready for review I wanted to discuss the 3 topics mentioned in the tasks list, so any discussion regarding these would be greatly appreciated.

[Toffikk]

i believe an empty string would be the best way to go about disabling it, same as it is done in other softwares

update-folder = ""

[OakLoaf]

i believe an empty string would be the best way to go about disabling it, same as it is done in other softwares

update-folder = ""

I completely agree, that's what my head initially went to. Wasn't sure if there was a different practice in toml as I've not worked with it before :)

I'll adjust that one

[OakLoaf]

I also noticed that in most places in the config object it seems like the nullable annotation is not used so I'll remove it unless suggested otherwise for VelocityConfiguration#getUpdateFolderName

[Toffikk]

I also noticed that in most places in the config object it seems like the nullable annotation is not used so I'll remove it unless suggested otherwise for VelocityConfiguration#getUpdateFolderName

technically the name is not nullable, as it is an empty string (and should be checked against being empty, not null) in the update-folder = "" syntax, so marking it as @Nullable would be wrong.

[OakLoaf]

technically the name is not nullable, as it is an empty string (and should be checked against being empty, not null) in the update-folder = "" syntax, so marking it as @Nullable would be wrong.

That's true, currently it was requiring being set to null to disable it, I'll make these changes now. I am also going to opt for .update as default for now as that is my personal preference and what I think makes most sense.

[Timongcraft]

I think Velocity's update folder should mirror the Paper update folder's name (update without a dot prefix).
Also why does this need config option? I don't see why the average admin would need to change this and for advanced setups a system property is better suited IMO.
I also don't get why we would create the update folder by default or why you don't utilize Java's Files class and instead use #toFile.

[OakLoaf]

I think Velocity's update folder should mirror the Paper update folder's name (update without a dot prefix).

The dot prefix acts as a way of keeping the folder at the top of the directory list and separates it from being mixed in with the plugin folders. I personally think it makes the update folder much more accessible and easy to use, which is why I set it as the default. I would go as far to say that Paper could also benefit from changing the default to include the prefix.

Also why does this need config option? I don't see why the average admin would need to change this and for advanced setups a system property is better suited IMO.

I personally think a config option is nice to have for people who prefer different formats or languages. It could use a system properpty but I don't really think this being a config option has any drawbacks

I also don't get why we would create the update folder by default or why you don't utilize Java's Files class and instead use #toFile.

I am happy to change this if needed

[Timongcraft]

The dot prefix acts as a way of keeping the folder at the top of the directory list and separates it from being mixed in with the plugin folders. I personally think it makes the update folder much more accessible and easy to use, which is why I set it as the default. I would go as far to say that Paper could also benefit from changing the default to include the prefix.

That makes sense, thank you for explaining.

I personally think a config option is nice to have for people who prefer different formats or languages. It could use a system properpty but I don't really think this being a config option has any drawbacks

I don't get how different languages/formats matter here. For the average admin it is that folder and the folder being standardized also simplifies debugging for other people.

I am happy to change this if needed

Ofc it is not needed, IMO it's better to use the NOI though, as that is the more modern and cleaner solution.

[OakLoaf]

I don't get how different languages/formats matter here. For the average admin it is that folder and the folder being standardized also simplifies debugging for other people.

That's fair and very true. I guess it's almost like if Paper added a config option for .paper-remapped which would be unnecessary. With system properties the option would still be there for the update folder so I do get your point. Would we stick to a single velocity.update-folder-name where an empty string disables the update folder or do you think a second velocity.disable-update-folder would be suitable?

Ofc it is not needed, IMO it's better to use the NOI though, as that is the more modern and cleaner solution.

Was just having a look through the changes in this PR and the only places I am using toFile are for isDirectory which was what was previously used in other parts of the code, I'll migrate VelocityPluginManager and VelocityServer over to use NIO instead

[Timongcraft]

Well IMO we don't need to disable the update folder if we just don't auto create it, no?

[OakLoaf]

Well IMO we don't need to disable the update folder if we just don't auto create it, no?

I based the need to disable off of this discussion in the other pr regarding this topic

[Timongcraft]

Well IMO we don't need to disable the update folder if we just don't auto create it, no?

I based the need to disable off of discussion in the other pr regarding this topic

Well then I think we might want a config section to disable it for individual plugins, but it also more seems like something for plugin authors to handle. If they are truly malicious they can do anything and if not then users should either raise an issue or search for an alternative. Also this should only become an issue after this feature is introduced, no?

[OakLoaf]

Well then I think we might want a config section to disable it for individual plugins, but it also more seems like something for plugin authors to handle. If they are truly malicious they can do anything and if not then users should either raise an issue or search for an alternative. Also this should only become an issue after this feature is introduced, no?

In that case, I think sticking to just an empty string to disable is probably suitable enough as it doesn't require any extra options.