Infernus101 / KitUI

Advance AdvancedKits! Select Kits from a form, just do /kit and get kits info and select it if available! For Minecraft Bedrock Edition!
GNU General Public License v3.0
44 stars 33 forks source link

Incompatible with other plugins that use Forms #37

Closed Awzaw closed 6 years ago

Awzaw commented 6 years ago

https://github.com/Infernus101/KitUI/blob/17c8151726b0397bceecff4da7f9e39989ae4b06/src/Infernus101/KitUI/PlayerEvents.php#L71

This changes $formId in any ModalFormResponsePacket received.

Infernus101 commented 6 years ago

Can you create a pull request to fix... @Awzaw

Awzaw commented 6 years ago

The problem here, and for all other 'UI' plugins is going to be avoiding formId collisions with the core and each other. The current forms-api branch uses a counter up from 0, so you could choose to use a counter from any large number, for example, or a reverse counter down from the max unsigned 32bit INT I think - but none of those will guarantee there aren't collisions. Alternatively, the best option might be to rewrite it for the forms-api branch, but since that's not yet finished you should probably wait until it's merged with master, and nobody will give you an ETA for that.

Infernus101 commented 6 years ago

I chose 3200 as counter, whats wrong with that? @Awzaw

Awzaw commented 6 years ago

Nothing wrong with that, but you still need to change the code so that line 71 doesn't change the formId in every ModalFormResponsePacket that's checked.

Infernus101 commented 6 years ago

Does this branch fix this issue?

Infernus101 commented 6 years ago

@Awzaw

Awzaw commented 6 years ago

I'd start by fixing the problem with overwriting formId (see PR), then worry about collisions.

Infernus101 commented 6 years ago

Collisions as in different players getting different player's outcomes/outputs? If so, i can fix that easily.. @Awzaw

Awzaw commented 6 years ago

I haven't yet looked closely, but will if I have time; I was thinking of formId collisions between plugins, and core too once the forms-api branch is merged, but now you mention it... is that not already handled?

Infernus101 commented 6 years ago

Done @Awzaw, btw the issue you are talking about can be handled if we could somehow check if the form id is taken/being used or not...

Awzaw commented 6 years ago

I've discussed this with PMMP staff, and don't think that's going to happen, although the forms-api branch isn't yet finished so anything is possible... But for now UI plugins will need to track their own formIds per player, and try to avoid using the same formIds as core and other non forms-api UI plugins.

Infernus101 commented 6 years ago

Ok so this issue can be closed?

Awzaw commented 6 years ago

Testing the latest submission to Poggit now. Ideally you want to avoid doing anything unnecessary in DataPacketReceiveEvent, and I can't see why you need to json_decode formData as early as this. Also I'd have checked formId and the player before you even call $window->handle($packet) here

Infernus101 commented 6 years ago

c191c3c?