[pve-devel] [PATCH proxmox-widget-toolkit v2] form: add Proxmox.form.field.DisplayEdit

[Thomas Lamprecht]

On 4/16/20 3:19 PM, Dominik Csapak wrote:
> Looks good except one issue i found,
> the 'editConfig' gets modified globally for the class,
> leading to weird side effects
> 
> i changed the useredit to use this for the username
> it worked, but when i created a new user after
> editing an old one, the username field was filled
> with the last username i edited

aw, I even noticed this behavior too yesterday but got distracted
and forgot it again until today :/ thanks for investigating!

> 
> the reason is one line in initcomponent (comment inline)
> 
> On 4/16/20 2:31 PM, Thomas Lamprecht wrote:
>> ...
>> +    initComponent: function() {
>> +    let me = this;
>> +
>> +    let displayConfig = {
>> +        xtype: me.displayType,
>> +        bind: {},
>> +    };
>> +    Ext.applyIf(displayConfig, me.initialConfig);
>> +    delete displayConfig.editConfig;
>> +    delete displayConfig.editable;
>> +
>> +    let editConfig = me.editConfig;
> 
> since me.editConfig is an object, this is not a copy,
> but a reference
> 
> during the second run of initComponent, we get here
> the instanced object from before (with all its set values)
> 
> what we probably want is a shallow copy
> 
> es6 style:
> let editConfig = Object.assign({}, me.editConfig);
> 
> or extjs style:
> let editConfig = Ext.apply({}, me.editConfig);
> 
> (we use Ext.apply everywhere, but the es6
> style is probably faster, so i included it here


So I'm open to use the es6 one, but it differs a bit from the ExtJS one
in that it uses getters and setters to to the assignments:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Description

While Ext.apply does a more plain .hasOwnProperty + direct assignment:
https://docs.sencha.com/extjs/6.0.1/classic/src/Ext.js.html#Ext-method-apply

not sure if we can interchange them in all situations, or if it has some
side effects. Maybe we even want the Object.assign behavior?

For now I'll use Ext.apply, as the remaining initComponent makes rather
heavy use of that one, so Object.assign could look a bit out of place.

> 
> with that change consider it:
> 
> Reviewed-By: Dominik Csapak <d.csapak at proxmox.com>
> Tested-By: Dominik Csapak <d.csapak at proxmox.com>

Much thanks for checking this out.