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

[Dominik Csapak]

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

the reason is one line in initcomponent (comment inline)

On 4/16/20 2:31 PM, Thomas Lamprecht wrote:
> This allows to write our often used:
> 
>> {
>>      xtype: me.isCreate ? 'someEditableField' : 'displayfield',
>>      ...
>> }
> 
> In a more schematic way, as it can now be controlled by either our
> CBind mixin or ExtJS native data binding.
> 
> Use a Field container to add both, they editable and they display,
> variants of a form field. As default use "textfield" for the editable
> and "displayfield" xtype for the read only one.
> 
> Pass all but the editConfig and editable members of our initial
> config to the display field, allow further to configure the editable
> field with an editConfig object, which overwrites the config
> properties inherited from the displayConfig/parent config.
> 
> This gives full control while not enforcing to specify anything extra
> for most default cases.
> 
> Enforce initial state of the fields even if the databinding would
> handle it to avoid glitches after first render for simple boolean expression
> cases.
> 
>> {
>>      xtype: 'pmxDisplayEditField',
>>      cbind: {
>>          editable: '{isCreate}',
>>      },
>>      name: 'tokenid',
>>      fieldLabel: gettext('Token ID'),
>>      value: me.tokenid,
>>      allowBlank: false,
>> }
> 
> Here, cbind could also be a bind or a native boolean expression.
> 
> For something else than a texfield one would use the editConfig, e.g.:
>> {
>>      ....
>>      editConfig: {
>>          xtype: 'pveUserSelector',
>>          allowBlank: false,
>>      },
>> },
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> ---
> 
> changes v1 -> v2:
> * fix applying data binding when the instances has it's own bind config already
> * bind value to ensure it's synced between both fields
> * drop array use from "alias" definition, but keep it as ExtJS uses it
>    primarily and we too for >3/4 of our definitions
> 
>   Makefile            |  1 +
>   form/DisplayEdit.js | 77 +++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 78 insertions(+)
>   create mode 100644 form/DisplayEdit.js
> 
> diff --git a/Makefile b/Makefile
> index 703b570..a729b95 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -15,6 +15,7 @@ JSSRC=					\
>   	data/ObjectStore.js		\
>   	data/RRDStore.js		\
>   	data/TimezoneStore.js		\
> +	form/DisplayEdit.js		\
>   	form/ExpireDate.js		\
>   	form/IntegerField.js		\
>   	form/TextField.js		\
> diff --git a/form/DisplayEdit.js b/form/DisplayEdit.js
> new file mode 100644
> index 0000000..f146f91
> --- /dev/null
> +++ b/form/DisplayEdit.js
> @@ -0,0 +1,77 @@
> +Ext.define('Proxmox.form.field.DisplayEdit', {
> +    extend: 'Ext.form.FieldContainer',
> +    alias: 'widget.pmxDisplayEditField',
> +
> +    viewModel: {
> +	parent: null,
> +	data: {
> +	    editable: false,
> +	    value: undefined,
> +	},
> +    },
> +
> +    displayType: 'displayfield',
> +
> +    editConfig: {},
> +    editable: false,
> +    setEditable: function(editable) {
> +	let me = this;
> +	let vm = me.getViewModel();
> +
> +	me.editable = editable;
> +	vm.set('editable', editable);
> +    },
> +
> +    layout: 'hbox',
> +    defaults: {
> +	hideLabel: true
> +    },
> +
> +    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

with that change consider it:

Reviewed-By: Dominik Csapak <d.csapak at proxmox.com>
Tested-By: Dominik Csapak <d.csapak at proxmox.com>

> +	Ext.applyIf(editConfig, {
> +	    xtype: 'textfield',
> +	    bind: {},
> +	});
> +	Ext.applyIf(editConfig, displayConfig);
> +
> +	Ext.applyIf(displayConfig.bind, {
> +	    hidden: '{editable}',
> +	    disabled: '{editable}',
> +	    value: '{value}',
> +	});
> +	Ext.applyIf(editConfig.bind, {
> +	    hidden: '{!editable}',
> +	    disabled: '{!editable}',
> +	    value: '{value}',
> +	});
> +
> +	// avoid glitch, start off correct even before viewmodel fixes it
> +	editConfig.disabled = editConfig.hidden = !me.editable;
> +	displayConfig.disabled = displayConfig.hidden = !!me.editable;
> +
> +	editConfig.name = displayConfig.name = me.name;
> +
> +	Ext.apply(me, {
> +	    items: [
> +		displayConfig,
> +		editConfig,
> +	    ],
> +	});
> +
> +	me.callParent();
> +
> +	me.getViewModel().set('editable', me.editable);
> +    },
> +
> +});
>