Velocity icon indicating copy to clipboard operation
Velocity copied to clipboard

Tab list api rough around the edges.

Open Rossterd opened this issue 1 year ago • 1 comments

Expected Behavior

The current tab list API allows the following to occur:

// Two players
Player player1 = p1.get();
Player player2 = p2.get();

// Game profile to send tablist entry for
GameProfile tabListPlayer = GameProfile.forOfflinePlayer("Johny");

// A TabListEntry which will internally reference Player1's TabList
TabListEntry tabListEntry = TabListEntry.builder()
    .profile(tabListPlayer)
    .displayName(Component.text("DisplayName").color(NamedTextColor.GOLD))
    .latency(1000)
    .gameMode(1)
    .tabList(player1.getTabList())
    .build();


// Add this entry both the player's TabLists. 
player1.getTabList().addEntry(tabListEntry);
player2.getTabList().addEntry(tabListEntry);

// If we try to change a value of the previously added TabListEntry for Player2,
// it changes the entry for Player1 because the entry still references that TabList.
Optional<TabListEntry> entry = player2.getTabList().getEntry(tabListPlayer.getId());
entry.ifPresent(listEntry -> listEntry.setDisplayName(Component.text("Changed DisplayName").color(NamedTextColor.DARK_PURPLE)));

Someone editing a tab list entry in player2's tab list would expect player2's tab list to change.

Actual Behavior

player1's tab list changes.

Steps to Reproduce

See code above.

Plugin List

Velocity Version

Velocity 3.4.0-SNAPSHOT (git-08a42b37-b449)

Additional Information

A simple solution is throwing an exception on the second AddEntry because player.getTabList() != tabListEntry.getTabList().

Another option is modifying the internal TabList when an TabListEntryis added to a TabList. But then why bother specifying a TabList when creating a TabListEntry? I think this area of the API is a little clunky in this respect and could use some work.

Rossterd avatar Nov 04 '24 15:11 Rossterd

The broken behavior here is that it doesn't support updating both tablists;

I've wanted to consider replacing this API with something that plugins can replace the impl of for their own needs (and all upserts, etc, would run through that). I'm not really sure what the answer here is, in the meantime, I'd assume that we should just try to make it track a set of tab lists, rather than just 1

electronicboy avatar Nov 04 '24 15:11 electronicboy