Paper

Paper copied to clipboard

Adds in Compontent methods for the new title methods in InventoryView - wasn't 100% sure on the custom inventoryholder rebased patch changes, but everything else should be alright Partially fixes #9189, however, I just carbon copied Spigot's implementation, unsure if Paper wanted a better implementation

Just wanna point out that this will currently break existing custom InventoryView implementations, which will surely affect some plugins compatibility.

yeah, this needs to be done in a backwards compatible way. sadly plugins implement parts of the bukkit api themselves...

In that case the component methods should just serialize back and forth and store as strings? Almost feels unnecessary but keeps compat

What you’d do is add default methods which defer to the legacy methods, add add the MustOverride annotation to hint people into replacing them

@imDaniX @MiniDigger

Just wanna point out that this will currently break existing custom InventoryView implementations, which will surely affect some plugins compatibility.

Upstream just added 2 new methods to InventoryView, getOriginalTitle and setTitle, so I'm pretty sure extending this is very much unsupported API.

Upstream just added 2 new methods to InventoryView, getOriginalTitle and setTitle, so I'm pretty sure extending this is very much unsupported API.

Yeah, I guess so, but it's actually one of rare occurrences where it mentions possibility of implementing it in the javadoc:

Note: If you implement this interface but fail to satisfy the expected contracts of certain methods, there's no guarantee that the game will work as it should.

So I'm 50/50 about this. Upstream adding getOriginalTitle and setTitle as abstract might've been a simple oversight. I'd say if Paper goes this way anyway, the note should just be removed, like with any other Bukkit interface.

https://hub.spigotmc.org/stash/projects/SPIGOT/repos/bukkit/pull-requests/874/overview Upstream doesn't appear to care abt backwards compat here for custom impl support - if that's the case I see no issue with the proposed impl

Paper comments look good to go hopefully? idk why i'm so bad at these

The title changing generally behaves rather inconsistently when called in a InventoryClickEvent. Here are a few run downs:

Bukkit.getScheduler().runTaskLater(this, () -> {
    event.getView().title(Component.text(Instant.now().toString()));
}, 1);

causes the client to loose the ability to collect items of the same type via double click.

event.getView().title(Component.text(Instant.now().toString()));

completely messes with the client server side sync, causing either desyncs in the cursor or simple inability to pick up/put down the item.

event.setCancelled(true);
event.getView().title(Component.text(Instant.now().toString()));

works well, the only combination of states that properly works.

Generally, I think this method needs a bit of a larger discussion, I already touched on this when this PR was proposed to paper, imo this logic barely deserves to live in the API, at best in UnsafeValues, tho that is just my opinion.

The title changing generally behaves rather inconsistently when called in a InventoryClickEvent. Here are a few run downs:

Bukkit.getScheduler().runTaskLater(this, () -> {
    event.getView().title(Component.text(Instant.now().toString()));
}, 1);

causes the client to loose the ability to collect items of the same type via double click.

event.getView().title(Component.text(Instant.now().toString()));

completely messes with the client server side sync, causing either desyncs in the cursor or simple inability to pick up/put down the item.

event.setCancelled(true);
event.getView().title(Component.text(Instant.now().toString()));

works well, the only combination of states that properly works.

Generally, I think this method needs a bit of a larger discussion, I already touched on this when this PR was proposed to paper, imo this logic barely deserves to live in the API, at best in UnsafeValues, tho that is just my opinion.

I have use my own nms way for this and my own experience is you have to send the update async for the title. I have not experiences any desync or item dupe with my way. So players can add or remove items without get stuck in a "loop".

So what is it I do then, I not modify the actual inventory or the original title at all. What I do is trick the client to think the inventory have new title, but it is far from perfect and if you accentually detect wrong type or size it will be glitchy client side. What is happening is this items from players inventory ends up in wrong inventory and player can lose the items.

I started a tread for a long time ago here and even show how I did it. I could show a video how my code works.

Generally, I think this method needs a bit of a larger discussion, I already touched on this when this PR was proposed to paper, imo this logic barely deserves to live in the API, at best in UnsafeValues, tho that is just my opinion.

Has there been discussion about that ever since?

Having access to Bukkit's setTitle(String) via the InventoryView class and Paper's title(Component) via UnsafeValues doesn't sound like a reasonable thing to do. Both methods should be in one place.

Despite all issues this can cause when called on InventoryClickEvent, which I do believe is also the case for setTitle(String) method, I think it still makes sense to add this for those who want to use it. A warning could be added to make sure people are aware of any potential side-effects that may come with using it.

So what is it I do then, I not modify the actual inventory or the original title at all. What I do is trick the client to think the inventory have new title, but it is far from perfect and if you accentually detect wrong type or size it will be glitchy client side. What is happening is this items from players inventory ends up in wrong inventory and player can lose the items.

I've been doing that prior InventoryTitle#setTitle(String) was added and since I don't call it without cancelling the InventoryClickEvent first, I wasn't aware of any desyncs and other glitches that may happen. Either way, I think implementation of this PR should not be different from one introduced by CraftBukkit.

Despite all issues this can cause when called on InventoryClickEvent

We don't want to add more API that is inherently broken. I'm leaning towards deprecating the setTitle(String) method with the notice that it is just broken and shouldn't be relied upon.

I wasn't aware of any desyncs and other glitches that may happen.

Yeah, they certainly exist as lynxplay pointed out. Double-clicking to collect items of the same type just doesn't work.

If we can show that cancelling + changing the title has no negative effect and is reliable, I think separate API should be introduced to do just that. Force cancel the event + change the title so the 2 things cannot be separated. That can't be done with a method on InventoryView.

Generally, I think this method needs a bit of a larger discussion, I already touched on this when this PR was proposed to paper, imo this logic barely deserves to live in the API, at best in UnsafeValues, tho that is just my opinion.

Has there been discussion about that ever since?

Having access to Bukkit's setTitle(String) via the InventoryView class and Paper's title(Component) via UnsafeValues doesn't sound like a reasonable thing to do. Both methods should be in one place.

Despite all issues this can cause when called on InventoryClickEvent, which I do believe is also the case for setTitle(String) method, I think it still makes sense to add this for those who want to use it. A warning could be added to make sure people are aware of any potential side-effects that may come with using it.

So what is it I do then, I not modify the actual inventory or the original title at all. What I do is trick the client to think the inventory have new title, but it is far from perfect and if you accentually detect wrong type or size it will be glitchy client side. What is happening is this items from players inventory ends up in wrong inventory and player can lose the items.

I've been doing that prior InventoryTitle#setTitle(String) was added and since I don't call it without cancelling the InventoryClickEvent first, I wasn't aware of any desyncs and other glitches that may happen. Either way, I think implementation of this PR should not be different from one introduced by CraftBukkit.

I talking about if you implement that function self and what I seen does this api use same logic and methods as I do. You need to make sure you get right inventory and something I guess the set tittle in the api do.

When I implemented this I have to call this asynchronous or player get stuck in clicking on a item. What I experienced is you have to send a update package to player (after modify title) and that could in theory create issues.

Just for you don't do it synchrony and at least I think you could dupe items in theory or item loss. Because the client and server side inventory is not syncing.

Cancel the event could reduce the problem, but not remove it.

Only way I think is modify the original NMS code, so title is not hardwired to the inventory self. Only time I could say it is safe to change title is when you open inventory not after.

We don't want to add more API that is inherently broken. I'm leaning towards deprecating the setTitle(String) method with the notice that it is just broken and shouldn't be relied upon.

I mean, I understand that reliability of this method is questionable, is there anything that can be done on the server-side to fix it?

Also wouldn't call these methods "broken" - they do serve a purpose and can occasionally break when used in the wrong place, or at the wrong time. This applies to number of other methods within the Bukkit API and it's usually stated in javadocs.

If we can show that cancelling + changing the title has no negative effect and is reliable, I think separate API should be introduced to do just that. Force cancel the event + change the title so the 2 things cannot be separated. That can't be done with a method on InventoryView.

Any idea of what this could be? Because when I'm thinking of InventoryClickEvent#changeTitle(Component) or similar, that doesn't really appeal to me. There are number of places such functionality may be desired to be used, including those that are not associated with any events - eg. animated titles using repeating task.

is there anything that can be done on the server-side to fix it?

I don't know, and I don't really care to try and hack a solution in to fix some of the issues. The game's protocol just flat out doesn't support changing titles without re-creating and re-sending the entire inventory open packet which makes the client create a whole new screen and change to it.

Based on my quick understanding of how double-click works, it is tracked on the client, and if the current screen sees 2 clicks within a certain time span, it then sends a packet telling the server to collect all items to the cursor. But that's not possible if you change the title because it is a new instance of the client's screen so it resets the double-click tracking.

Also wouldn't call these methods "broken" - they do serve a purpose

Having any effect doesn't mean they also aren't broken.

can occasionally break when used in the wrong place, or at the wrong time. This applies to number of other methods within the Bukkit API and it's usually stated in javadocs.

This is bad in the first place, we shouldn't add more API that this statement applies to. Just because API exists that this statement applies to, doesn't mean we should keep adding API in this manner.