Forms API #1476

dktapps · 2017-10-17T20:34:40Z

No description provided.

all forms have titles

…ross the board

this makes it completely clear what both of these do, and also makes it clear that buttons can't be put on custom forms.

SOF3

There seems to be an inconsistency, where calling getters of form input without form submission sometimes gets a null but sometimes gets an InvalidStateException.
Throwing an exception may also result in some dependence (the assumption that "if I don't get this exception, I am OK"), which does not work as expected when someone tries to get the value from a form they resent but is not yet resubmitted.

// I have not reviewed the code under pocketmine\form\element yet.

SOF3 · 2017-10-18T06:09:50Z

src/pocketmine/form/MenuForm.php

+	}
+
+	/**
+	 * Returns the index of the option selected by the user. Pass this to {@link getButton} to get the button object


Dead link to getButton. I believe this should be Pass this to {@link MenuForm::getOption} to get the MenuOption object.

SOF3 · 2017-10-18T06:13:23Z

src/pocketmine/form/MenuForm.php

+	public function getSelectedOption() : MenuOption{
+		$index = $this->getSelectedOptionIndex();
+		if($index === null){
+			throw new \InvalidStateException("No option selected, maybe the form hasn't been submitted yet");


Consider changing the message to Form is closed or has not been submitted yet to be more precise.

SOF3 · 2017-10-18T06:14:01Z

src/pocketmine/form/MenuForm.php

+	 * Sets the selected option to the specified index or null. null = no selection.
+	 * @param int $option
+	 */
+	public function setSelectedOptionIndex(int $option) : void{


Is there a practical reason to expose this method publicly?

SOF3 · 2017-10-18T06:16:01Z

src/pocketmine/form/ModalForm.php

+	 *
+	 * @param bool $choice
+	 */
+	public function setChoice(bool $choice) : void{


Is there a reason to expose this method publicly?

Plugins may want to modify form result data when it's submitted (there will be events for this)

SOF3 · 2017-10-18T06:37:08Z

src/pocketmine/form/Form.php

+	/**
+	 * Clears response data from a form, useful if you want to reuse the same form object several times.
+	 */
+	public function clearResponseData() : void{


This method is very confusing. It is not called by the server, but relies on the developer to call it.

This leads to inconsistency:

Is a Form object expected to be reused for a single player?

Is a Form object expected to be reused for multiple players?

Can the developer use the form values outside onSubmit()?

The answer is:

If the same Form instance is only sent once, you can use the form values outside onSubmit() and hold the Form instance forever.

If the same Form instance is sent more than once (same player or not), it would be a very bad practice to depend on the value held in the Form instance outside onSubmit(), even in the same tick.

It is particularly tempting to hold a CustomForm instance and use the values there later, since the data cannot be easily extracted. I can foresee this is going to cause a lot of concurrency bugs that can be very difficult to identify. Therefore, it is suggested that, starting from the core, one of either approach ("resend same instance" or "new instance each resend") be adopted to avoid confusion.

Adopting the "resend same instance" approach should automatically call the clearResponseData() method after each onSubmit() call.

Adopting the "new instance each resend" approach should remove this method and add a flag to identify a Form as sent and throw an exception when attempting to use sendForm() with it.

Deciding which approach to adopt should require extensive consultation.

SOF3 · 2017-10-18T06:50:56Z

src/pocketmine/form/MenuOption.php

+		return $json;
+	}
+
+}


What about also adding an empty public function onSelected(), which is called before/after MenuForm::onSubmit() is called? This can allow developers to write their handlers in the menu options instead of from the big MenuForm object, resulting in extra routing work. Without adding this in the core, developers have to add a few lines of routing code + assertion that the MenuOption::onSelected() method exists in order to put the handler code with the MenuOption.

before, or after? If a developer does it themselves, they are explicit about when they want it to be called, which can be any time - if the core does it, then developers have to get used to BEFORE, or AFTER, no in-betweens.

SOF3 · 2017-10-18T06:52:31Z

src/pocketmine/form/ServerSettingsForm.php

+ * Represents a custom form which can be shown in the Settings menu on the client. This is exactly the same as a regular
+ * CustomForm, except that this type can also have an icon which can be shown on the settings section button.
+ */
+class ServerSettingsForm extends CustomForm{


All other Form classes are abstract. Why is this suddenly non-abstract?

inxomnyaa · 2017-10-21T12:25:11Z

src/pocketmine/form/element/Dropdown.php

+	 * @param string[] $steps
+	 * @param int      $defaultStepIndex
+	 */
+	public function __construct(string $text, array $steps, int $defaultStepIndex = 0){


Calling it Steps makes it confusing - If possible, rename to $options. A dropdown is a list of options that you can choose from, not a slider

inxomnyaa · 2017-10-21T14:21:15Z

src/pocketmine/Player.php

@@ -3279,6 +3279,11 @@ public function sendWhisper(string $sender, string $message){
 	}

 	public function sendForm(Form $form) : void{
+		if($form->hasBeenSent()){
+			throw new \InvalidArgumentException("Cannot send the same form more than once, create a new one instead");


But: Why? So i can not cache a Form and send it to a player? So it has to be recreated (rebuildt in the core) every time? Isn't that just a waste of calculation if i have a form with never changing contents?

The core code has no foolproof way to tell if your form is immutable (so to speak). You can clone the form object if you're concerned about the overhead.

I have already explained in my previous comment:

It is particularly tempting to hold a CustomForm instance and use the values there later, since the data cannot be easily extracted. I can foresee this is going to cause a lot of concurrency bugs that can be very difficult to identify. Therefore, it is suggested that, starting from the core, one of either approach ("resend same instance" or "new instance each resend") be adopted to avoid confusion.

Adopting the "resend same instance" approach should automatically call the clearResponseData() method after each onSubmit() call.

Adopting the "new instance each resend" approach should remove this method and add a flag to identify a Form as sent and throw an exception when attempting to use sendForm() with it.

As it has been noticed that resetting contents each time makes it very inconvenient to retain data (especially inconvenient for custom form data), the "new instance each resend" approach is considered more appropriate.

Regarding the overhead of form creation overhead, perhaps you should compare it against encoding the form, sending it to the client, making the client display it and sending it back. As one wouldn't reasonably send forms very frequently, the impact on performance is negligible, so trying to cache forms for the sake of performance would be premature optimization, and therefore your comment is 4√(all evil), and so you are 8√(all evil).

inxomnyaa · 2017-10-21T14:24:48Z

"Offtoppic" question: Could you merge the changes from master into the branch / update it?

inxomnyaa · 2017-10-21T16:13:23Z

src/pocketmine/form/ModalForm.php

+abstract class ModalForm extends Form{
+
+	/** @var string */
+	private $content;


Please concider adding a function to get/set the content

AyyItzJ4y · 2017-10-22T07:40:10Z

how do you make a player open a form when they hold click a specific block

inxomnyaa · 2017-10-22T11:49:31Z

Please, for questions head over to the Forums. This is a place to discuss the changes and issues, not some random guy's issues about not reading the docs / commits

YaBoiLewis · 2017-10-22T12:00:47Z

I believe image handling on UI has been added, correct me if I'm wrong. If so, I suggest adding an image class. No unnecessary replies required. A yes or no would suffice.

dktapps · 2017-10-22T12:06:04Z

Have eyes been added yet, if so, they should be used.

dktapps · 2017-10-22T12:16:30Z

wishes github had a facepalm reaction

dktapps · 2017-12-07T10:49:24Z

Aside from the bugs, some things we're not happy with yet:

We don't allow sending the same form instance twice. I don't think this is reasonable, a better restriction would be to only allow sending to one player at a time (makes more sense).
Leading on from the above point, the disposability of forms leads to the problem that a form cannot be easily copied, i.e. duplicating the form and erasing the data for reuse. This requires, then, that a form object must always be recreated. This is a little inconvenient.
Plugins wanting to access their owners inside a form currently have to implement their own methods for getting the plugin's PluginBase instance. For example, the following code:

$player->sendForm(new class($this, $username, $error) extends ModalForm{
	/** @var Main */
	private $plugin;

	public function __construct(Main $plugin, string $username, string $error){
		$this->plugin = $plugin;
		parent::__construct("Account transfer failed!", "Failed to transfer \"$username\" account to yours.\nError: $error\n\nWould you like to try again?");
	}

	public function onSubmit(Player $player) : ?Form{
		if($this->getChoice()){
			$this->plugin->requestAccountDetails($player);
		}else{
			$this->plugin->warnCancelTransfer($player);
		}
		return null;
	}
});

requires that the form being sent must declare a $plugin property and assign it in the constructor, in order to access it in onSubmit(). This leads to boilerplate code. However, I don't want to restrict forms to being a plugin-only thing - the server should be able to use them as well.

inxomnyaa · 2017-12-07T11:10:18Z

Plugins wanting to access their owners inside a form currently have to implement their own methods for getting the plugin's PluginBase instance. For example, the following code:

Actually that applies to any parameter. Passing a variable that a player input into a form and displaying it in another form requires constructor rewrites. I suggest an additional parameter array $arguments = []

SOF3 · 2017-12-13T16:47:22Z

@thebigsmileXD Explicitly requiring passing the Plugin also has the advantage that it's possible to know the scope of the form and manage it with plugin enable/disable stuff.

NyaomiDEV · 2017-12-15T23:24:06Z

Why don't make some events like PlayerFormSubmitEvent and PlayerFormCloseEvent to handle form submitting and closing? It would be a lot simple, especially if forms' classes are not abstract so we can make Form objects right out of the box and we can send them easily, without managing them by extending them with our own classes in plugins. For example, this piece of code in a class that extends Listener:

public function onSubmit(PlayerFormSubmitEvent $e){
    // assuming we are handling a Toggle element in index 0 that enables something
    /** @var Toggle $toggle */
    $toggle = $e->getForm()->getElement(0);
    $this->doSomethingToPlayer($e->getPlayer(), $toggle->getValue());
    $e->getPlayer()->sendForm(new CustomForm("Test Form", array(
        new Toggle("Test toggle 1", false),
        new Toggle("Test toggle 2", true)
    )));
}

will be MUCH simpler to implement for plugin developers as we don't have to pass the owner to our form to do things on onSubmit().
Also, if you think of a way to make forms instantiable out of the box then when we are sending a form to a player it would be a good idea to have a custom internal form name or ID to keep track of what form is being sent to the player; for example this piece of code:

public function onSubmit(PlayerFormSubmitEvent $e){
    // first we want to make sure the form we are receiving is a "TestToggleForm1" form
    if($e->getForm()->getInternalName() !== "TestToggleForm1") return;
    // then we manage to do something...

    // assuming we are handling a Toggle element in index 0 that enables something
    /** @var Toggle $toggle */
    $toggle = $e->getForm()->getElement(0);
    $this->doSomethingToPlayer($e->getPlayer(), $toggle->getValue());
    // maybe the constructor for a CustomForm object can be
    // "string $internalName, string $title, array $elements"
    $e->getPlayer()->sendForm(new CustomForm("TestToggleForm2", "Test Form 2", array(
        new Toggle("Test toggle 1", false),
        new Toggle("Test toggle 2", true)
    )));
}

Obviously this is more straight-forward but it will certainly have problems in more complicated plugins that use forms to do complicated things but I wrote this suggestion in a couple of minutes so it definitely needs some refining here and there but the main idea is there.

SOF3 · 2017-12-16T02:30:40Z

@AryToNeX This will certainly make it much harder to pass extra data through the form.

inxomnyaa · 2017-12-16T14:37:00Z

@AryToNeX I highly say NO THANKS to events. customui was discontinued just because events created plugin-crossover calls, and every plugin reacted to the event.

dktapps · 2017-12-16T14:40:46Z

Events are a reasonable ask (since they are just, well, events) but definitely a bad idea to make them the primary way to handle a form.

public function onSubmit(PlayerFormSubmitEvent $e){
    // first we want to make sure the form we are receiving is a "TestToggleForm1" form
    if($e->getForm()->getInternalName() !== "TestToggleForm1") return;
    ...

here's the problem already.

pranavbhuv · 2017-12-24T13:21:39Z

Not meant to be rude, but, can we get this merged by New Year?

xhaeffer · 2017-12-29T11:47:26Z

Not meant to be rude, but this PR make teaspoon plugin error

Sandertv · 2017-12-29T11:48:41Z

Plugins should adapt to the core, not the other way around. Furthermore, this is a development branch.

dktapps · 2017-12-29T11:48:58Z

@WaldoFS that's entirely irrelevant to the discussion. TeaSpoon is its own project.

Thunder33345 · 2017-12-29T11:51:51Z

@WaldoFS Not meant to be rude, plugin are suppose to make themself work with the core, not the other way around

xhaeffer · 2017-12-29T11:57:19Z

Ok, thanks all 😘

GooglePlugin · 2018-01-18T11:55:49Z

I tested @thebigsmileXD 's WarpUI with this branch, but it didn't work properly.
Here are some bugs I found:

Forms doesn't appear after sending a form.
Form::onSubmit is not called after submitting a form.

@dktapps This has been W.I.P For almost THREE months. When will it be done?

robske110 · 2018-01-18T15:40:18Z

@GooglePlugin When somebody got time for it. ~~Also, i think the problems you mentioned are probably WarpUI's problems.~~

eqMFqfFd · 2018-01-18T15:42:02Z

How do you change the ‘Submit’ button to other text?

Thunder33345 · 2018-01-18T15:43:03Z

This is not a plugin support forum
please use forums.pmmp.io

dktapps · 2018-01-18T16:28:30Z

When will it be done?

you must be new round here...

GooglePlugin · 2018-01-19T10:35:36Z

I haven't fully understand the rule here. Not meant to be rude, but I really look forward to this ;)

SOF3 · 2018-01-19T10:37:35Z

It appears no constructive discussion is going on in this pull request, and therefore I am locking. If you have any further comments, please create a thread on the forums.

dktapps · 2018-04-14T20:50:30Z

I'm going to close this since there hasn't been any movement on it recently. A new PR will be created when we have something worth shipping.

dktapps added 11 commits October 6, 2017 09:49

Some base work for forms API

96cba44

Implemented ListForm, refactored some stuff

9f963dd

fix inspections

8ce7cb0

Change namespace to 'form' because @SOF3 is fussy

8f40482

Move "title" property up to Form base class

383cb7c

all forms have titles

ModalForm: fix typo

d0690b8

Added some documentation

fa7eadb

Some changes to how response data is handled to make it consistent ac…

ecd51b2

…ross the board

Refactor ListForm -> MenuForm and Button -> MenuOption

ca107a6

this makes it completely clear what both of these do, and also makes it clear that buttons can't be put on custom forms.

Added CustomForm and its components

5b84b43

Added ServerSettingsForm class, refactor icon handling

a7df2ca

dktapps added Category: API Related to the plugin API PR: Contribution Status: Incomplete Work in progress labels Oct 17, 2017

SOF3 self-requested a review October 18, 2017 05:04

SOF3 previously requested changes Oct 18, 2017

View reviewed changes

fix some issues

4877f96

ZeroDevOfficial mentioned this pull request Oct 18, 2017

Discussion ZeroDevOfficial/Aether#3

Closed

inxomnyaa suggested changes Oct 21, 2017

View reviewed changes

dktapps added 3 commits October 21, 2017 13:49

don't allow re-using form objects

a0a9eb9

refactor some confusing shit

04965a1

change some exception messages to be more clear

17ad96f

inxomnyaa reviewed Oct 21, 2017

View reviewed changes

inxomnyaa suggested changes Oct 21, 2017

View reviewed changes

Added debug for form ID mismatch

402acf5

SOF3 mentioned this pull request Jan 4, 2018

Asynchronous events #1881

Closed

pmmp locked as too heated and limited conversation to collaborators Jan 19, 2018

Merge branch 'master' into forms-api

eadc44d

dktapps closed this Apr 14, 2018

dktapps added Resolution: Stale and removed Status: Incomplete Work in progress labels Apr 14, 2018

dktapps added Type: Enhancement Contributes features or other improvements to PocketMine-MP and removed Type: Contribution labels Nov 26, 2022

Forms API #1476

Forms API #1476

Conversation

dktapps commented Oct 17, 2017

SOF3 left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SOF3 Oct 18, 2017 • 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

inxomnyaa Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

inxomnyaa Oct 21, 2017 • 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

inxomnyaa commented Oct 21, 2017

Choose a reason for hiding this comment

AyyItzJ4y commented Oct 22, 2017

inxomnyaa commented Oct 22, 2017

YaBoiLewis commented Oct 22, 2017 • edited Loading

dktapps commented Oct 22, 2017

dktapps commented Oct 22, 2017

dktapps commented Dec 7, 2017 • edited Loading

inxomnyaa commented Dec 7, 2017

SOF3 commented Dec 13, 2017

NyaomiDEV commented Dec 15, 2017 • edited Loading

SOF3 commented Dec 16, 2017

inxomnyaa commented Dec 16, 2017

dktapps commented Dec 16, 2017

pranavbhuv commented Dec 24, 2017

xhaeffer commented Dec 29, 2017

Sandertv commented Dec 29, 2017

dktapps commented Dec 29, 2017

Thunder33345 commented Dec 29, 2017

xhaeffer commented Dec 29, 2017

GooglePlugin commented Jan 18, 2018 • edited Loading

robske110 commented Jan 18, 2018 • edited Loading

eqMFqfFd commented Jan 18, 2018

Thunder33345 commented Jan 18, 2018

dktapps commented Jan 18, 2018

GooglePlugin commented Jan 19, 2018

SOF3 commented Jan 19, 2018

dktapps commented Apr 14, 2018

SOF3 Oct 18, 2017 •

edited

Loading

inxomnyaa Oct 21, 2017 •

edited

Loading

inxomnyaa Oct 21, 2017 •

edited

Loading

YaBoiLewis commented Oct 22, 2017 •

edited

Loading

dktapps commented Dec 7, 2017 •

edited

Loading

NyaomiDEV commented Dec 15, 2017 •

edited

Loading

GooglePlugin commented Jan 18, 2018 •

edited

Loading

robske110 commented Jan 18, 2018 •

edited

Loading