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

add client setting for cooldown type, refactor show-coordinates #2193

Merged
merged 12 commits into from
May 15, 2021
Merged

add client setting for cooldown type, refactor show-coordinates #2193

merged 12 commits into from
May 15, 2021

Conversation

Konicai
Copy link
Member

@Konicai Konicai commented May 4, 2021

This allows the user to choose what kind of cooldown they want to see (title, action bar, or false) through a dropdown. Similarly to the show coordinate option, they aren't shown the option if it is disabled globally through the geyser config (at least that is the only setter of DEFAULT_SHOW_COOLDOWN currently).

This was was done in a new PreferencesCache in the session. I threw the show coordinates code in there as well since it seemed more appropriate. Also turned the allowShowCoordinates boolean into a @Getter field so that the form constructor is a bit nicer.

Relies on GeyserMC/languages#69

Konicai added 3 commits May 3, 2021 20:52
…ting

# Conflicts:
#	connector/src/main/java/org/geysermc/connector/network/session/GeyserSession.java
- note, i accidentally removed the line that set it null on close when I merged master into this branch
@Konicai
Copy link
Member Author

Konicai commented May 4, 2021

Should have pushed at once to avoid the extra builds... whoops.

Also I think I just borked my languages submodule?

I'll replace the dropdown text with the proper locale getter once we think this is good to go

@Konicai
Copy link
Member Author

Konicai commented May 4, 2021

Also I forgot to mention, CooldownUtils.getByName returns TITLE instead of DISABLED when the given string has no match, to line up with the javadoc.

import org.geysermc.connector.utils.CooldownUtils;

@Getter
public class PreferencesCache {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this name should be changed, but I'm not sure to what. I'm going to have a PR after OptionalPack is merged that allows a plugin to change if a client can Bedrock godbridge or not, so the boolean for that might as well be stuck here. Open to suggestions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it would stop all connected geyser players from god-bridging, why would the boolean be stored in every session instance?

Do you mean the PR would allow plugins to change an allowGodbridging value that geyser uses to prevent god-bridging, or it would allow an extension plugin to do the work of preventing god-bridging?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it would stop all connected geyser players from god-bridging, why would the boolean be stored in every session instance?

We can't do that because of GeyserConnect, or if server owners only wanted to block per-world for example. The plugin would be separate and use plugin messages to communicate to Geyser.

@Konicai
Copy link
Member Author

Konicai commented May 10, 2021

Tested successfully on bungeecord (For the button text, the key names are shown instead of their values since the translation strings haven't been added to the languages submodule yet)

Copy link
Member

@Camotoy Camotoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that, if the Geyser instance disables the cooldown type, that should prevent Geyser from doing so as well? Knowing now that Java Edition has a toggle for it is making me think that's unnecessary - though right now we also allow the blocking of coordinates.

@@ -1226,7 +1228,7 @@ public void sendDownstreamPacket(Packet packet) {
public void setReducedDebugInfo(boolean value) {
reducedDebugInfo = value;
// Set the showCoordinates data. This is done because updateShowCoordinates() uses this gamerule as a variable.
getWorldCache().updateShowCoordinates();
getPreferencesCache().updateShowCoordinates();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be the Lombok call, and can just be preferencesCache.updateShowCoordinates()

}

if (CooldownUtils.getDefaultShowCooldown() != CooldownUtils.CooldownType.DISABLED) {
CooldownUtils.CooldownType cooldownType = CooldownUtils.CooldownType.values()[settingsResponse.getDropdownResponses().get(offset).getElementID()];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use CooldownType.VALUES here. values() is a function that allocates a new array (which we don't need when we have one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I wasn't sure if there was a significant difference

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the same apply to the gamemode and difficulty stuff below this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not believe those have a VALUES static field.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the gamerule enum is ours, but the VALUES static field is private. the difficulty enum is mcprotocollibs and doesn't have one.

not sure if this is going out of the scope of this pr

@@ -149,7 +158,7 @@ public static CooldownType getByName(String name) {
return type;
}
}
return DISABLED;
return TITLE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't do this because legacy configs that have their cooldown settings set to false will break.

@Camotoy Camotoy merged commit 3e3b8fa into GeyserMC:master May 15, 2021
@Konicai Konicai deleted the cooldown-user-setting branch May 24, 2021 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants