Adding more options for configuring ping passthrough. - #1870
Adding more options for configuring ping passthrough.#1870TheMiningTeamYT wants to merge 11 commits into
Conversation
- Bumped config-version to 2.8 - Updated comments and grammar - Removed legacy ping passthrough in the config
| MODS, | ||
| DESCRIPTION, | ||
| ALL | ||
| public class PingPassthroughMode { |
There was a problem hiding this comment.
make this a record, and put the default value as a static default inside of here
There was a problem hiding this comment.
What do you mean "put the default value as a static default"? I'm not too familiar with Java in general or records in particular (I had to google them to even know what you were talking about).
There was a problem hiding this comment.
public static final PingPassthroughMode DEFAULT = new Ping....That way the default is contained alongside the definitions of all of those booleans
There was a problem hiding this comment.
I think I understand; you want an instance of ping passthrough mode in the file just to indicate what the defaults are, yes? Or are you wanting me to use it in some specific way?
There was a problem hiding this comment.
Actually it might be a good idea to do something like the new packet limiter code
| " Should Velocity pass the favicon (also known as the server icon) from the backend server when responding to server list ping requests?" | ||
| ); | ||
|
|
||
| // Create ping passthrough entry for the mods info |
There was a problem hiding this comment.
I don't think these // Create ping passthrough entry for the X comments are adding much value, they can be omitted
There was a problem hiding this comment.
I'll be honest, I agree with you. They have now been removed.
| || this.modinfo; | ||
| } | ||
|
|
||
| // Not used, just here to state what the defaults are. |
There was a problem hiding this comment.
Not used, just here to state what the defaults are.
This is odd, why not use this in VelocityConfiguration line 83?
There was a problem hiding this comment.
Or I'd actually just overload the record constructor with the defaults (all false) and use it in VelocityConfiguration.
This doesn't work well with using DEFAULT in the getOrElses, but you can ofc do both - public static final PingPassthroughMode DEFAULT = new PingPassthroughMode();, either is fine
There was a problem hiding this comment.
I'm getting conflicting requests here. I'd appreciate it if y'all could decide on what you want me to implement (or just deal with it among yourselves after accepting the pull request).
There was a problem hiding this comment.
Not really conflicting, the comment was that the default instance should exist here rather than being an anonymous default instance created elsewhere, this can then be used for the defaults elsewhere
There was a problem hiding this comment.
Alright, done. I ended up implementing both the overloaded constructor and the default PingPassthroughMode as @WouterGritter suggested. However, I kept the initialization of pingPassthrough in VelocityConfiguration.java line 83 as private PingPassthroughMode pingPassthrough = PingPassthroughMode.DEFAULT; since that's more in line with how the rest of the code is written.
| private boolean onlineModeKickExistingPlayers = false; | ||
| @Expose | ||
| private PingPassthroughMode pingPassthrough = PingPassthroughMode.DISABLED; | ||
| private PingPassthroughMode pingPassthrough = new PingPassthroughMode(false, false, false, false, false); |
There was a problem hiding this comment.
= PingPassthroughMode.DEFAULTwas the primary change I suggested rather than having this randomly created instance;
the blow block of getOrElse() can also use the default instance, that way it's all defined in a singular place
There was a problem hiding this comment.
Ah okay I understand now. I honestly forgot that was there. That being said, I think change this to function more like the PacketLimiterConfig. I like how it puts all the code for parsing the sections in one place. Unless you disagree.
There was a problem hiding this comment.
The limiter config was just a quick toss together for the sake of getting this out, if I wanted everything in one place I would just have PingPassthroughMode.of(CommentedConfig) for that
There was a problem hiding this comment.
The limiter config was just a quick toss together for the sake of getting this out
Let me ask then, is there a different way you would prefer me to do this?
(Sorry if I come of as a bit annoying, I'm not the most experienced Java developer and I really wanna make sure I do this right so this PR can finally be merged).
|
By the way, out of the scope this pull request, but while working on it I discovered that your gitignore doesn't include VS Code's workspace config directory (.vscode). Just thought y'all should know. I may open a pull request for this at some point. |
281a356 to
1218588
Compare
5 participants
This pull request replaces pull request #1519. The existing code has had its commit history cleaned up, brought up to compatibility with the current head of the new active branch dev/4.0.0, and had its configuration options moved into their own section as was previously requested.
What follows is the description from the original pull request.
When using Velocity, I have been frustrated by the lack of fine control over ping passthrough, specifically the lack of support for passing through favicons without passing through the whole ping request. I've also seen people on line asking how to pass through favicons, so I decided to add the ability to control which aspects of server list ping requests are passed through from the back end server, and which aspects aren't, through 5 new boolean configuration options in the config file.