SpectateLogin implementation #2745

Bagietas · 2023-08-31T15:09:50Z

Re-opened #2552 cause I don't have access to modify any of that.

ljacqu · 2023-09-03T23:11:09Z

Unit tests are missing
If the server crashes and players are not authenticated, players will be stuck in spectator gamemode (see comment thread in the original PR referencing "limbo player")

ljacqu · 2023-09-03T23:17:39Z

src/main/java/fr/xephi/authme/settings/properties/RestrictionSettings.java

+ newProperty("settings.restrictions.spectateStandLogin.headYaw", 0.0f);
+
+ @Comment("Head Pitch position for 'spectateStandLogin'.")
+ public static final Property<Double> HEAD_PITCH =


I think this should work with the version of ConfigMe that AuthMe is using (please ignore this suggestion if it doesn't)

Suggested change

public static final Property<Double> HEAD_PITCH =

public static final Property<Float> HEAD_PITCH =

Also the naming is a bit weird, I don't know that RestrictionSettings#HEAD_PITCH refers to the spectate stand feature, and as a user I'm not sure I'll understand what this setting represents. Is it possible to elaborate in the comment? I would make a suggestion but I'm actually not really sure what the yaw & pitch settings are for

I don't know If i understood you correctly, but HEAD_PITCH and HEAD_YAW are under spectateStandLogin so It's obvious what are these settings for.

Thanks for changing the comment. I also meant the field name, RestrictionSettings.HEAD_PITCH in the code sounds very generic. Could we maybe name it SPECTATE_HEAD_PITCH, or maybe you have a better idea?

ljacqu · 2023-09-03T23:20:11Z

src/main/java/fr/xephi/authme/AuthMe.java

@@ -316,6 +319,8 @@ public void onDisable() {
 // Wait for tasks and close data source
 new TaskCloser(this, database).run();

+ spectateLoginService.removeArmorstands();


Potential null pointer if the plugin crashes before this service is set on L251

The proper way to do this is to not have the field, but to use another method from the injector (not sure what methods exist by heart, but if there's something that returns a singleton if it's been registered, that would be it, otherwise maybe injector.createIfHasDependencies though it's not ideal to still potentially create it at this point)

The field on the class is no longer needed (an IDE shows an unused warning)

injector.createIfHasDependencies(SpectateLoginService.class) might return null, so the issue of a potential null pointer still remains. And I think it would be better to use injector#getIfAvailable

ljacqu · 2023-09-03T23:25:31Z

src/main/java/fr/xephi/authme/listener/PlayerListener.java

+ || spectateLoginService.hasStand(event.getPlayer())) {
+ // If a player is dead, no stand will be created for him
+ // therefore we can be sure that there will not be two stands
+ bukkitService.runTaskLater(() -> spectateLoginService.createStand(event.getPlayer()), 1L);


I can't confirm the comment about stands not being created if a player is dead but maybe I'm missing the relevant point, but interestingly the logic here specifies a stand should be created if the setting is enabled, or if the player has a stand? I don't think that makes much sense.

Removed unnecessary comment.

Now an armor stand is created for a player every time he respawns, regardless of whether an armor stand already exists or not

Bagietas · 2023-09-04T12:57:00Z

Unit tests are missing

What do you mean by this? (Never used tests)

If the server crashes and players are not authenticated, players will be stuck in spectator gamemode (see comment thread in the original PR referencing "limbo player")

Implemented it.

ljacqu · 2023-09-04T13:53:03Z

Unit tests are missing

What do you mean by this? (Never used tests)

Every class has a "Test" class associated to it, e.g. BukkitService has BukkitServiceTest. These are unit tests, which ensure that refactorings and similar don't break things in unexpected ways. Searching for JUnit should also provide additional information.

If the server crashes and players are not authenticated, players will be stuck in spectator gamemode (see comment thread in the original PR referencing "limbo player")

Implemented it.

I think it would be important to test different scenarios, like if existing limbo player data exists before these changes, there might be limbo player data already stored on disk. Since no "gamemode" exists, it's null, which I think will cause further issues down the line when we try to set the gamemode to null on a player. I don't fully remember the whole lifecycle of the LimboPlayer (and I'm going abroad soon, so a little pressed for time this week) but I think this would be nice to really test in detail to make sure this won't break players' data.
It might make sense to only touch the gamemode if the player is currently in spectator mode, the spectating setting is enabled, and we have another gamemode on the limbo player. Or maybe to have a config like we have for many of the other limbo stuff to allow it to be enabled and disabled. Someone else from the AuthMe team might have some good inputs here.

ljacqu

Your code doesn't compile btw

Bagietas · 2023-09-09T07:31:52Z

Unit tests are missing

What do you mean by this? (Never used tests)

Every class has a "Test" class associated to it, e.g. BukkitService has BukkitServiceTest. These are unit tests, which ensure that refactorings and similar don't break things in unexpected ways. Searching for JUnit should also provide additional information.

It's my first time I do something like tests. That's my try to implement it but I don't know if it's correct.

If the server crashes and players are not authenticated, players will be stuck in spectator gamemode (see comment thread in the original PR referencing "limbo player")

Implemented it.

I think it would be important to test different scenarios, like if existing limbo player data exists before these changes, there might be limbo player data already stored on disk. Since no "gamemode" exists, it's null, which I think will cause further issues down the line when we try to set the gamemode to null on a player. I don't fully remember the whole lifecycle of the LimboPlayer (and I'm going abroad soon, so a little pressed for time this week) but I think this would be nice to really test in detail to make sure this won't break players' data. It might make sense to only touch the gamemode if the player is currently in spectator mode, the spectating setting is enabled, and we have another gamemode on the limbo player. Or maybe to have a config like we have for many of the other limbo stuff to allow it to be enabled and disabled. Someone else from the AuthMe team might have some good inputs here.

If I understood you correctly I've added what you've meant.

Commit base by mrchefran.

5fd543c

ljacqu reviewed Sep 3, 2023

View reviewed changes

Code improvements.

a15a75d

Fix checkstyle issues.

164e270

Bagietas requested a review from ljacqu September 4, 2023 13:01

ljacqu requested changes Sep 4, 2023

View reviewed changes

Requested changes.

ed3e52c

Bagietka and others added 4 commits September 9, 2023 09:33

Checkstyle issues

3cee02b

Update SpectateLoginServiceTest.java

79b80c0

Update pom.xml

6196f07

Update SpectateLoginService.java

b76ac81

Bagietas requested a review from ljacqu September 9, 2023 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SpectateLogin implementation #2745

SpectateLogin implementation #2745

Bagietas commented Aug 31, 2023

ljacqu commented Sep 3, 2023

ljacqu Sep 3, 2023 •

edited

Loading

Bagietas Sep 4, 2023

ljacqu Sep 4, 2023

Bagietas Sep 9, 2023

ljacqu Sep 3, 2023 •

edited

Loading

Bagietas Sep 4, 2023

ljacqu Sep 4, 2023

Bagietas Sep 9, 2023

ljacqu Sep 3, 2023 •

edited

Loading

Bagietas Sep 4, 2023

ljacqu Sep 4, 2023

Bagietas Sep 9, 2023

Bagietas commented Sep 4, 2023

ljacqu commented Sep 4, 2023

ljacqu left a comment

Bagietas commented Sep 9, 2023

	public static final Property<Double> HEAD_PITCH =
	public static final Property<Float> HEAD_PITCH =

SpectateLogin implementation #2745

Are you sure you want to change the base?

SpectateLogin implementation #2745

Conversation

Bagietas commented Aug 31, 2023

ljacqu commented Sep 3, 2023

ljacqu Sep 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljacqu Sep 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljacqu Sep 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bagietas commented Sep 4, 2023

ljacqu commented Sep 4, 2023

ljacqu left a comment

Choose a reason for hiding this comment

Bagietas commented Sep 9, 2023

ljacqu Sep 3, 2023 •

edited

Loading

ljacqu Sep 3, 2023 •

edited

Loading

ljacqu Sep 3, 2023 •

edited

Loading