foundryvtt icon indicating copy to clipboard operation
foundryvtt copied to clipboard

Prototype token config fails to save when calling _onSubmit

Open theripper93 opened this issue 2 years ago • 12 comments

What happened?

When calling the _onSubmit method on the Prototype token form, the data is not saved.

What ways of accessing Foundry can you encounter this issue in?

  • [X] Native App (Electron)
  • [ ] Chrome
  • [ ] Firefox
  • [ ] Safari
  • [ ] Other

Reproduction Steps

sample code

Hooks.on("init", () => {
    function addApplyButton(app,html){
        if(!app.options.closeOnSubmit) return;
        const injectPoint = html.find(".sheet-footer");
        const btn = $(`<button type="button"><i class="fas fa-check"></i> Apply</button>`);
        injectPoint.append(btn);
        btn.click((e) => {
            app._onSubmit(e, {preventClose: true, preventRender: true});
            $(e.currentTarget).focus(false);
        })
    }

    Hooks.on("renderTokenConfig", addApplyButton)

})

This code creates a simple apply button. While this works correctly on all other forms, it seems to have issues in the prototype token form. Running this code and attempting to "apply" the Prototype token config, then closing and reopening the config will show that the data is not saved most of the times.

Side note for other possibly related issue:

  • Open prototype token config and edit something
  • Don't save or close the form
  • Drop the corresponding token. The token will use the currently unsaved data.

What core version are you reporting this for?

Version 10 288

Relevant log output

No response

Bug Checklist

  • [X] The issue occurs while all Modules are disabled

theripper93 avatar Oct 11 '22 17:10 theripper93

Does this occur when submitting the form using its normal (non-modified) submit button? Or does reproducing this issue rely upon injecting a secondary button to the form which calls the _onSubmit workflow without actually being a form submission event?

The _onSubmit function is designed to transact a form submission event, but you are passing it a click event. Is that part of the problem?

aaclayton avatar Oct 12 '22 13:10 aaclayton

It seems to happen only with the injected button and only with the prototype token config , that’s why I thought it was odd

theripper93 avatar Oct 12 '22 18:10 theripper93

I looked into this and it's quite specific to this dialog. When using the normal 'update token' button, operations are performed in the following order:

  1. Preview is reset to original, updating the actor's in-memory source.
  2. The update is dispatched to the server.
  3. The server responds, the actor's in-memory source is updated appropriately.
  4. Because closeOnSubmit is true, the dialog closes.

What happens with the apply button added by the snippet is:

  1. The update is dispatched to the server.
  2. The server responds, the actor's in-memory source is updated appropriately.
  3. The user then clicks 'close'.
  4. The preview is reset to original, updating the actor's in-memory source back to the original, and overwriting the changes from the earlier update (in-memory only).

So the changes are being persisted, but the client state becomes out-of-sync. You can verify this with:

  1. Open prototype token config.
  2. Change a value.
  3. Click 'apply'.
  4. Click 'close'.
  5. Refresh the page.
  6. Open the prototype token config again.
  7. Observe that the change was actually persisted and applied.

Unclear whether this prompts a core change here, or whether the onus is on the 3rd party modifying this dialog's behaviour to appropriately account for this with their modifications. In this case, awaiting _onSubmit and updating the TokenConfig#original to match the new state should resolve it.

Fyorl avatar Oct 14 '22 18:10 Fyorl

It's not a huge issue for me, since my button serves no purpouse on the protitype token, it's there simply because it's injected into the renderTokenConfig hook, but i found it very strange. This said, i think it is something that could affect (and confuse) other module\system developers as it's the only dialog to display this behaviour. So if possible adressing it in core would be the best option.

theripper93 avatar Oct 15 '22 00:10 theripper93

This issue seems to extend to other modules i have where i create a separate Form to do some extra configuration on the token. This just calls a setFlag when it's done but displays the same problematic behaviour. Any news on wherther this is gonna be handled by core or if i need to hack something in order to make it work?

theripper93 avatar Oct 23 '22 17:10 theripper93

I wouldn't expect this to occur with a separate form. This behaviour is specific to TokenConfig (and AmbientLightConfig which also maintains an original copy to reset back to).

Fyorl avatar Oct 24 '22 11:10 Fyorl

Yes, but I still need to have it working for other modules that update the proto while the window is open. That’s why I’m asking if I should expect a core solution to this bug or not

theripper93 avatar Oct 24 '22 12:10 theripper93

Can you please provide specific repro steps or elaborate on the issue you're experiencing (assuming it's different to the _onSubmit issue) because I am not able to distil this information from your comments thus far. At the moment, the dialog is working as intended so it's not a bug, but we can still make changes if it turns out to be something that downstream code cannot reasonably be expected to accommodate by itself.

Fyorl avatar Oct 24 '22 13:10 Fyorl

  • open prototype dialog
  • call a setFlag on the prototype token while the dialog is open
  • close the dialog
  • check the actor data, note how the data from the setFlag is not present
  • refresh the page with F5
  • check the actor data, the data from the setFlag is now present

theripper93 avatar Oct 24 '22 13:10 theripper93

  • open prototype dialog
  • call a setFlag on the prototype token while the dialog is open
  • close the dialog
  • check the actor data, note how the data from the setFlag is not present
  • refresh the page with F5
  • check the actor data, the data from the setFlag is now present

This is issue #8022 and not exclusive to the prototype token config. AmbientLightConfig has the same issue. I had to patch TokenConfig/AmbientLightConfig#_previewChanges to prevent it from overriding my flags that aren't injected into the config and may be changed while the config is open.

dev7355608 avatar Oct 24 '22 16:10 dev7355608

OK, I think I understand a bit better what the issue is here now, thanks. I think there might be a reasonably straightforward way to address this in core without having to bend over backwards to accommodate every possible edge case. I will see if that's actually the case and what side effects it has, if any.

Fyorl avatar Oct 24 '22 17:10 Fyorl

Alright, it's straightforward for normal token config and ambient light config, but not for prototype token config.

Fyorl avatar Oct 24 '22 18:10 Fyorl