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 warp saving format #5322

Open
wants to merge 3 commits into
base: 2.x
Choose a base branch
from

Conversation

Wyrdix
Copy link

@Wyrdix Wyrdix commented Apr 23, 2023

In reply to #5267 with JRoy's idea

Saving warps using a uuid instead of the santized warp name that was causing problems with warps with similar names.
Added backwards conversion
Added generation of a warp index so that an administrator knows which file stores which warp

Note that no changes have been made to the api layer, so these changes should not affect existing bridges with plugins.

Feel free to point out mistakes I've made, I'm still new to the pull request system and I'm trying to learn

@pop4959
Copy link
Member

pop4959 commented Apr 23, 2023

Hi, thanks for opening a PR!

I'm not sure that this implementation has been discussed very much outside of the comment in that other issue.

Personally, I don't really see what benefit keying warps by UUID accomplishes, when the warp names need to be unique themselves anyway (there cannot be 2 warp uuids that point to the same warp name).

As far as I'm concerned, the benefit of using UUIDs would have solely been to avoid special characters in warps' file names. However if you now use a UUID for the file name, you cannot simply load a file from the warp name itself (you would have to search all files or index them all to know which one is which). So you kind of shoot yourself in the foot with the extra redirection which is otherwise entirely unnecessary.

Open to discussion, but I'd be more inclined to instead loosen the restrictions on sanitization rather than move things to a UUID based system. I think the concern there is still backwards compatibility with existing sanitized warps, but maybe this can be put behind a configuration option. We did something similar to support less sanitized nicknames (unsafe nickname permission there instead of config).

@Wyrdix
Copy link
Author

Wyrdix commented Apr 23, 2023

Thanks for the feedback.
Well to be honest, this change was added to the milestone so I thought it was fine doing it

The big problem is that you can't really lessen the sanitizing because I don't think all os used to host minecraft server are compatible with non ascii character

@pop4959
Copy link
Member

pop4959 commented Apr 23, 2023

No worries, sorry for any confusion. I think JRoy added it to the milestone as something he personally wanted to look into further. Comments on GitHub should not necessarily be taken at face value, especially if they have not been thoroughly discussed or explored yet.

In general, it's usually a good idea to comment on the issue or discuss further with us on the EssentialsX Development Discord server, as we don't want to waste your time. However, PRs are appreciated of course and serve as a basis for further discussion.

OS support is definitely another issue. However, UUIDs still don't magically solve that as far as I'm concerned. See my above comment about requiring search/indexing which is not the case currently. If you think you have another idea which solves these problems effectively feel free to propose them.

@Wyrdix
Copy link
Author

Wyrdix commented Apr 23, 2023

Well I didn't look into that that much but warps were loaded all at once all I changed was using uuid in order to solve the problem

When the warps are loaded it is effectively the same as before.
So even though like you said we can't know which file correspond to which warp, because we load all of them at once and save them in a map it isn't a problem.

@Wyrdix
Copy link
Author

Wyrdix commented Apr 23, 2023

Another way to say it, it is that the index is generated without adding weight to the current system as it is generated at the same time as warps are loaded

@pop4959
Copy link
Member

pop4959 commented Apr 23, 2023

Gotcha. I can see how that seems reasonable then. To be fair I'm not really familiar with this either. But that seems to be pretty inefficient, if they are all being loaded on startup anyway then why several files? Seems like a lot more file I/O overhead then.

@pop4959
Copy link
Member

pop4959 commented Apr 23, 2023

Given the above, UUIDs work I guess, but it still feels pretty arbitrary. Not as bad given it works roughly the same as it does currently though.

@pop4959 pop4959 added type: enhancement Features and feature requests. module: main Issues or PRs for the main Essentials module labels May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: main Issues or PRs for the main Essentials module type: enhancement Features and feature requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants