[pve-devel] [PATCH manager 1/2] ui: BulkActions: rework filters and include tags

[pve-devel] [PATCH manager 1/2] ui: BulkActions: rework filters and include tags

[Dominik Csapak]

This moves the filters out of the grid header for the BulkActions and
puts them into their own fieldset above the grid. With that, we can
easily include a tags filter (one include and one exclude list).

The filter fieldset is collapsible and shows the active filters in
parenthesis. aside from that the filter should be the same as before.

To achieve the result, we regenerate the filterFn on every change of
every filter field, and set it with an 'id' so that only that filter is
overridden each time.

To make this work, we have to change three tiny details:
* manually set the labelWidths for the fields, otherwise it breaks
  the ones in the fieldset.
* change the counting in the 'getErrors' of the VMSelector, so that we
  actually get the count of selected VMs, not the one from the
  selectionModel
* override the plugins to '' in the BulkAction windows, so that e.g. in
  the backup window we still have the filters in the grid header
  (we could add a filter box there too, but that is already very crowded
  and would take up too much space for now)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/form/VMSelector.js   |  10 +-
 www/manager6/window/BulkAction.js | 261 +++++++++++++++++++++++++++++-
 2 files changed, 269 insertions(+), 2 deletions(-)

diff --git a/www/manager6/form/VMSelector.js b/www/manager6/form/VMSelector.js
index d59847f2..8186ea99 100644
--- a/www/manager6/form/VMSelector.js
+++ b/www/manager6/form/VMSelector.js
@@ -18,6 +18,8 @@ Ext.define('PVE.form.VMSelector', {
 	sorters: 'vmid',
     },
 
+    userCls: 'proxmox-tags-full',
+
     columnsDeclaration: [
 	{
 	    header: 'ID',
@@ -80,6 +82,12 @@ Ext.define('PVE.form.VMSelector', {
 		},
 	    },
 	},
+	{
+	    header: gettext('Tags'),
+	    dataIndex: 'tags',
+	    renderer: tags => PVE.Utils.renderTags(tags, PVE.UIOptions.tagOverrides),
+	    flex: 1,
+	},
 	{
 	    header: 'HA ' + gettext('Status'),
 	    dataIndex: 'hastate',
@@ -186,7 +194,7 @@ Ext.define('PVE.form.VMSelector', {
     getErrors: function(value) {
 	let me = this;
 	if (!me.isDisabled() && me.allowBlank === false &&
-	    me.getSelectionModel().getCount() === 0) {
+	    me.getValue().length === 0) {
 	    me.addBodyCls(['x-form-trigger-wrap-default', 'x-form-trigger-wrap-invalid']);
 	    return [gettext('No VM selected')];
 	}
diff --git a/www/manager6/window/BulkAction.js b/www/manager6/window/BulkAction.js
index 949e167e..333d5d30 100644
--- a/www/manager6/window/BulkAction.js
+++ b/www/manager6/window/BulkAction.js
@@ -59,12 +59,14 @@ Ext.define('PVE.window.BulkAction', {
 		    name: 'target',
 		    disallowedNodes: [me.nodename],
 		    fieldLabel: gettext('Target node'),
+		    labelWidth: 300,
 		    allowBlank: false,
 		    onlineValidator: true,
 		},
 		{
 		    xtype: 'proxmoxintegerfield',
 		    name: 'maxworkers',
+		    labelWidth: 300,
 		    minValue: 1,
 		    maxValue: 100,
 		    value: 1,
@@ -75,6 +77,7 @@ Ext.define('PVE.window.BulkAction', {
 		    xtype: 'fieldcontainer',
 		    fieldLabel: gettext('Allow local disk migration'),
 		    layout: 'hbox',
+		    labelWidth: 300,
 		    items: [{
 			xtype: 'proxmoxcheckbox',
 			name: 'with-local-disks',
@@ -112,6 +115,7 @@ Ext.define('PVE.window.BulkAction', {
 		{
 		    xtype: 'proxmoxcheckbox',
 		    name: 'force-stop',
+		    labelWidth: 120,
 		    fieldLabel: gettext('Force Stop'),
 		    boxLabel: gettext('Force stop guest if shutdown times out.'),
 		    checked: true,
@@ -121,6 +125,7 @@ Ext.define('PVE.window.BulkAction', {
 		    xtype: 'proxmoxintegerfield',
 		    name: 'timeout',
 		    fieldLabel: gettext('Timeout (s)'),
+		    labelWidth: 120,
 		    emptyText: '180',
 		    minValue: 0,
 		    maxValue: 7200,
@@ -129,6 +134,260 @@ Ext.define('PVE.window.BulkAction', {
 	    );
 	}
 
+	let statusMap = [];
+	let poolMap = [];
+	let haMap = [];
+	let tagMap = [];
+	PVE.data.ResourceStore.each((rec) => {
+	    if (['qemu', 'lxc'].indexOf(rec.data.type) !== -1) {
+		statusMap[rec.data.status] = true;
+	    }
+	    if (rec.data.type === 'pool') {
+		poolMap[rec.data.pool] = true;
+	    }
+	    if (rec.data.hastate !== "") {
+		haMap[rec.data.hastate] = true;
+	    }
+	    if (rec.data.tags !== "") {
+		rec.data.tags.split(/[,; ]/).forEach((tag) => {
+		    if (tag !== '') {
+			tagMap[tag] = true;
+		    }
+		});
+	    }
+	});
+
+	let statusList = Object.keys(statusMap).map(key => [key, key]);
+	let poolList = Object.keys(poolMap).map(key => [key, key]);
+	let tagList = Object.keys(tagMap).map(key => ({ value: key }));
+	let haList = Object.keys(haMap).map(key => [key, key]);
+
+	let filterChange = function() {
+	    let nameValue = me.down('#namefilter').getValue();
+	    let filterCount = 0;
+
+	    if (nameValue !== '') {
+		filterCount++;
+	    }
+
+	    let arrayFiltersData = [];
+	    ['status', 'pool', 'type', 'hastate'].forEach((filter) => {
+		let selected = me.down(`#${filter}filter`).getValue() ?? [];
+		if (selected.length) {
+		    filterCount++;
+		    arrayFiltersData.push([filter, [...selected]]);
+		}
+	    });
+	    let includeTags = me.down('#includetagfilter').getValue() ?? [];
+	    if (includeTags.length) {
+		filterCount++;
+	    }
+	    let excludeTags = me.down('#excludetagfilter').getValue() ?? [];
+	    if (excludeTags.length) {
+		filterCount++;
+	    }
+	    let vmid = me.down('#vmidfilter').getValue();
+	    if (vmid) {
+		filterCount++;
+	    }
+
+	    let fieldSet = me.down('#filters');
+	    if (filterCount) {
+		fieldSet.setTitle(Ext.String.format(gettext('Filters ({0})'), filterCount));
+	    } else {
+		fieldSet.setTitle(gettext('Filters'));
+	    }
+
+	    let filterFn = function(value) {
+		let name = value.data.name.toLowerCase().indexOf(nameValue.toLowerCase()) !== -1;
+		let arrayFilters = arrayFiltersData.every(([filter, selected]) =>
+		    !selected.length || selected.indexOf(value.data[filter]) !== -1);
+		let tags = value.data.tags.split(/[;, ]/).filter(t => !!t);
+		let includeFilter = !includeTags.length || tags.some(tag => includeTags.indexOf(tag) !== -1);
+		let excludeFilter = !excludeTags.length || tags.every(tag => excludeTags.indexOf(tag) === -1);
+		let vmidFilter = !vmid || value.data.vmid === vmid;
+
+		return name && arrayFilters && includeFilter && excludeFilter && vmidFilter;
+	    };
+	    me.down('#vms').getStore().setFilters({
+		id: 'customFilter',
+		filterFn,
+	    });
+	    me.down('#vms').checkChange();
+	};
+
+	items.push({
+	    xtype: 'fieldset',
+	    itemId: 'filters',
+	    collapsible: true,
+	    title: gettext('Filters'),
+	    layout: 'hbox',
+	    items: [
+		{
+		    xtype: 'container',
+		    flex: 1,
+		    padding: 5,
+		    layout: {
+			type: 'vbox',
+			align: 'stretch',
+		    },
+		    defaults: {
+			listeners: {
+			    change: filterChange,
+			},
+			isFormField: false,
+		    },
+		    items: [
+			{
+			    fieldLabel: gettext("Name"),
+			    itemId: 'namefilter',
+			    xtype: 'textfield',
+			},
+			{
+			    xtype: 'combobox',
+			    itemId: 'statusfilter',
+			    fieldLabel: gettext("Status"),
+			    emptyText: gettext('All'),
+			    multiSelect: true,
+			    editable: false,
+			    store: statusList,
+			},
+			{
+			    xtype: 'combobox',
+			    itemId: 'poolfilter',
+			    fieldLabel: gettext("Pool"),
+			    emptyText: gettext('All'),
+			    editable: false,
+			    multiSelect: true,
+			    store: poolList,
+			},
+		    ],
+		},
+		{
+		    xtype: 'container',
+		    layout: {
+			type: 'vbox',
+			align: 'stretch',
+		    },
+		    flex: 1,
+		    padding: 5,
+		    defaults: {
+			listeners: {
+			    change: filterChange,
+			},
+			isFormField: false,
+		    },
+		    items: [
+			{
+			    xtype: 'combobox',
+			    itemId: 'typefilter',
+			    fieldLabel: gettext("Type"),
+			    emptyText: gettext('All'),
+			    editable: false,
+			    multiSelect: true,
+			    store: [
+				['lxc', gettext('CT')],
+				['qemu', gettext('VM')],
+			    ],
+			},
+			{
+			    xtype: 'proxmoxComboGrid',
+			    itemId: 'includetagfilter',
+			    fieldLabel: gettext("Include Tags"),
+			    emptyText: gettext('All'),
+			    editable: false,
+			    multiSelect: true,
+			    valueField: 'value',
+			    displayField: 'value',
+			    listConfig: {
+				userCls: 'proxmox-tags-full',
+				columns: [
+				    {
+					dataIndex: 'value',
+					flex: 1,
+					renderer: value =>
+					    PVE.Utils.renderTags(value, PVE.UIOptions.tagOverrides),
+				    },
+				],
+			    },
+			    store: {
+				data: tagList,
+			    },
+			    listeners: {
+				change: filterChange,
+			    },
+			},
+			{
+			    xtype: 'proxmoxComboGrid',
+			    itemId: 'excludetagfilter',
+			    fieldLabel: gettext("Exclude Tags"),
+			    emptyText: gettext('None'),
+			    multiSelect: true,
+			    editable: false,
+			    valueField: 'value',
+			    displayField: 'value',
+			    listConfig: {
+				userCls: 'proxmox-tags-full',
+				columns: [
+				    {
+					dataIndex: 'value',
+					flex: 1,
+					renderer: value =>
+					    PVE.Utils.renderTags(value, PVE.UIOptions.tagOverrides),
+				    },
+				],
+			    },
+			    store: {
+				data: tagList,
+			    },
+			    listeners: {
+				change: filterChange,
+			    },
+			},
+		    ],
+		},
+		{
+		    xtype: 'container',
+		    layout: {
+			type: 'vbox',
+			align: 'stretch',
+		    },
+		    flex: 1,
+		    padding: 5,
+		    defaults: {
+			listeners: {
+			    change: filterChange,
+			},
+			isFormField: false,
+		    },
+		    items: [
+			{
+			    xtype: 'combobox',
+			    itemId: 'hastatefilter',
+			    fieldLabel: gettext("HA status"),
+			    emptyText: gettext('All'),
+			    multiSelect: true,
+			    editable: false,
+			    store: haList,
+			    listeners: {
+				change: filterChange,
+			    },
+			},
+			{
+			    xtype: 'pveGuestIDSelector',
+			    itemId: 'vmidfilter',
+			    fieldLabel: gettext('VMID'),
+			    allowBlank: true,
+			    name: 'vmid',
+			    listeners: {
+				change: filterChange,
+			    },
+			},
+		    ],
+		},
+	    ],
+	});
+
 	items.push({
 	    xtype: 'vmselector',
 	    itemId: 'vms',
@@ -137,6 +396,7 @@ Ext.define('PVE.window.BulkAction', {
 	    height: 300,
 	    selectAll: true,
 	    allowBlank: false,
+	    plugins: '',
 	    nodename: me.nodename,
 	    action: me.action,
 	    listeners: {
@@ -159,7 +419,6 @@ Ext.define('PVE.window.BulkAction', {
 		align: 'stretch',
 	    },
 	    fieldDefaults: {
-		labelWidth: me.action === 'migrateall' ? 300 : 120,
 		anchor: '100%',
 	    },
 	    items: items,
-- 
2.39.2

[pve-devel] [PATCH manager 2/2] ui: BulkAction: add clear filters button

[Dominik Csapak]

to be able to clear all of them at once

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
sending this as a separate patch since i'm not sure if the button is
worth it. if deemed appropriate could also be squashed into the previous
patch
 www/manager6/window/BulkAction.js | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/www/manager6/window/BulkAction.js b/www/manager6/window/BulkAction.js
index 333d5d30..44171390 100644
--- a/www/manager6/window/BulkAction.js
+++ b/www/manager6/window/BulkAction.js
@@ -162,6 +162,13 @@ Ext.define('PVE.window.BulkAction', {
 	let tagList = Object.keys(tagMap).map(key => ({ value: key }));
 	let haList = Object.keys(haMap).map(key => [key, key]);
 
+	let clearFilters = function() {
+	    me.down('#namefilter').setValue('');
+	    ['name', 'status', 'pool', 'type', 'hastate', 'includetag', 'excludetag', 'vmid'].forEach((filter) => {
+		me.down(`#${filter}filter`).setValue('');
+	    });
+	};
+
 	let filterChange = function() {
 	    let nameValue = me.down('#namefilter').getValue();
 	    let filterCount = 0;
@@ -192,10 +199,13 @@ Ext.define('PVE.window.BulkAction', {
 	    }
 
 	    let fieldSet = me.down('#filters');
+	    let clearBtn = me.down('#clearBtn');
 	    if (filterCount) {
 		fieldSet.setTitle(Ext.String.format(gettext('Filters ({0})'), filterCount));
+		clearBtn.setDisabled(false);
 	    } else {
 		fieldSet.setTitle(gettext('Filters'));
+		clearBtn.setDisabled(true);
 	    }
 
 	    let filterFn = function(value) {
@@ -383,6 +393,22 @@ Ext.define('PVE.window.BulkAction', {
 				change: filterChange,
 			    },
 			},
+			{
+			    xtype: 'container',
+			    layout: {
+				type: 'vbox',
+				align: 'end',
+			    },
+			    items: [
+				{
+				    xtype: 'button',
+				    itemId: 'clearBtn',
+				    text: gettext('Clear Filters'),
+				    disabled: true,
+				    handler: clearFilters,
+				},
+			    ],
+			},
 		    ],
 		},
 	    ],
-- 
2.39.2

Re: [pve-devel] [PATCH manager 1/2] ui: BulkActions: rework filters and include tags

[Thomas Lamprecht]

for the commit subject please: s/BulkActions/bulk actions/

Am 30/10/2023 um 13:58 schrieb Dominik Csapak:
> This moves the filters out of the grid header for the BulkActions and
> puts them into their own fieldset above the grid. With that, we can
> easily include a tags filter (one include and one exclude list).
> 
> The filter fieldset is collapsible and shows the active filters in
> parenthesis. aside from that the filter should be the same as before.
> 

basic tests seem to work, and I did not really check the code closely,
so only a higher level review, and some stuff is even pre-existing (but
sticks a bit more out now):

- the CT/VM ID filter is a bit odd, especially if tuned to match all,
  not only parts of the VMID (which would not be *that* much better
  either IMO), when I want to migrate/start/stop a single VM I can just
  do so, no need for opening the bulk actions.

- The migration one should move target and jobs into two columns, as of
  now there's just to much vertical space used.

- Maybe we can also merge the "allow local disk" check box and the
  warning for "ct will use restart mode" into columns (the former could
  loose the box label, that note is not really that useful anyway)

- In the spirit of the last two points, the shutdown action might also
  benefit from having force-stop and timeout on the same row in two
  columns

- We have a mix of title case and sentence case for the fields, they
  should all use title case (I find https://titlecaseconverter.com/
  nice). E.g., should be "HA Status" in the filters, and Parallel Jobs
  for migration, and so on.

- tag space is very limited, maybe default to using circles or even
  dense (I'm still wishing a bit for seeing the tag value on hover, like
  a tool tip), or re-use the tree-style shape.
  One alternative might be to add vertical scrolling here, but that is
  probably rather odd (and not sure if that would even work easily
  here).

- disallowing multi-select for Type (maybe better labeled "Guest Type"?)
  might improve UX, as if there are only two choices anyway a "All",
  "VM" "CT" selection might be simpler – but no hard feelings here.

Re: [pve-devel] [PATCH manager 1/2] ui: BulkActions: rework filters and include tags

[Dominik Csapak]

On 11/6/23 17:01, Thomas Lamprecht wrote:
> for the commit subject please: s/BulkActions/bulk actions/
> 
> Am 30/10/2023 um 13:58 schrieb Dominik Csapak:
>> This moves the filters out of the grid header for the BulkActions and
>> puts them into their own fieldset above the grid. With that, we can
>> easily include a tags filter (one include and one exclude list).
>>
>> The filter fieldset is collapsible and shows the active filters in
>> parenthesis. aside from that the filter should be the same as before.
>>
> 
> basic tests seem to work, and I did not really check the code closely,
> so only a higher level review, and some stuff is even pre-existing (but
> sticks a bit more out now):
> 
> - the CT/VM ID filter is a bit odd, especially if tuned to match all,
>    not only parts of the VMID (which would not be *that* much better
>    either IMO), when I want to migrate/start/stop a single VM I can just
>    do so, no need for opening the bulk actions.

counter argument:

if i want to migrate/start/stop a specific list of vmid it may be faster
if i go to the bulk window, search vmid -> select, search vmid -> select, etc.
than go to vm -> click stop -> go to vm -> click stop

but no hard feelings, if you want i'll remove it

> 
> - The migration one should move target and jobs into two columns, as of
>    now there's just to much vertical space used.
> 
> - Maybe we can also merge the "allow local disk" check box and the
>    warning for "ct will use restart mode" into columns (the former could
>    loose the box label, that note is not really that useful anyway)
> 

yeah i'll try those two

> - In the spirit of the last two points, the shutdown action might also
>    benefit from having force-stop and timeout on the same row in two
>    columns
> 

make sense

> - We have a mix of title case and sentence case for the fields, they
>    should all use title case (I find https://titlecaseconverter.com/
>    nice). E.g., should be "HA Status" in the filters, and Parallel Jobs
>    for migration, and so on.

sry for that, i'll fix it

> 
> - tag space is very limited, maybe default to using circles or even
>    dense (I'm still wishing a bit for seeing the tag value on hover, like
>    a tool tip), or re-use the tree-style shape.
>    One alternative might be to add vertical scrolling here, but that is
>    probably rather odd (and not sure if that would even work easily
>    here).

scrolling in two dimensions for different containers is always a bit weird
imho, but yeah making the tags 'dense' 'circle' or reusing the treestyle makes sense

i agree with the tooltip, but last time i looked at it that was not so easy
because we already have a tooltip in the tree (and we reuse the rendering
for that) but i could see if i could add a tooltip to the whole cell
here that prints all tags in a nice way, what do you think?

> 
> - disallowing multi-select for Type (maybe better labeled "Guest Type"?)
>    might improve UX, as if there are only two choices anyway a "All",
>    "VM" "CT" selection might be simpler – but no hard feelings here.

ah yeah, i'm not really sure why i did it this way, dropdown
with 3 distinct values makes much more sense here...

Re: [pve-devel] [PATCH manager 1/2] ui: BulkActions: rework filters and include tags

[Thomas Lamprecht]

Am 08/11/2023 um 13:14 schrieb Dominik Csapak:
> On 11/6/23 17:01, Thomas Lamprecht wrote:
>> - the CT/VM ID filter is a bit odd, especially if tuned to match all,
>>    not only parts of the VMID (which would not be *that* much better
>>    either IMO), when I want to migrate/start/stop a single VM I can just
>>    do so, no need for opening the bulk actions.
> 
> counter argument:
> 
> if i want to migrate/start/stop a specific list of vmid it may be faster
> if i go to the bulk window, search vmid -> select, search vmid -> select, etc.
> than go to vm -> click stop -> go to vm -> click stop

It's faster to use the context menu of the tree or the one from the
global search though.
But faster is IMO the wrong thing to look at here anyway, as using
bulk actions for a single action feels always wrong conceptually,
so devfinitively won't be a common path for users.
If, there would need ot be a situation when someone wants to do a bulk
action, but circumstances change midway, after they opened the dialog,
so that they only need to stop/start/... a single guest now.
And even then they can still do that without the extra field (deselect
all, select single guest).

So IMO adding a whole field for this niche use case seems rather
overkill to me.

>> - tag space is very limited, maybe default to using circles or even
>>    dense (I'm still wishing a bit for seeing the tag value on hover, like
>>    a tool tip), or re-use the tree-style shape.
>>    One alternative might be to add vertical scrolling here, but that is
>>    probably rather odd (and not sure if that would even work easily
>>    here).
> 
> scrolling in two dimensions for different containers is always a bit weird
> imho, but yeah making the tags 'dense' 'circle' or reusing the treestyle makes sense

Yeah, additional scrolling is indeed not the nicest thing to add here.

> 
> i agree with the tooltip, but last time i looked at it that was not so easy
> because we already have a tooltip in the tree (and we reuse the rendering
> for that) but i could see if i could add a tooltip to the whole cell
> here that prints all tags in a nice way, what do you think?

I mean, for here there would not be a tool tip yet and we could have
slightly different behavior here than in the tree any way.

But for the tree: How about reducing the scope of the current status
tooltip from the whole row to the icon + text part only, so tags can
have a separate, per tag one.

Re: [pve-devel] [PATCH manager 1/2] ui: BulkActions: rework filters and include tags

[Dominik Csapak]

On 11/8/23 13:38, Thomas Lamprecht wrote:
> Am 08/11/2023 um 13:14 schrieb Dominik Csapak:
>> On 11/6/23 17:01, Thomas Lamprecht wrote:
>>> - the CT/VM ID filter is a bit odd, especially if tuned to match all,
>>>     not only parts of the VMID (which would not be *that* much better
>>>     either IMO), when I want to migrate/start/stop a single VM I can just
>>>     do so, no need for opening the bulk actions.
>>
>> counter argument:
>>
>> if i want to migrate/start/stop a specific list of vmid it may be faster
>> if i go to the bulk window, search vmid -> select, search vmid -> select, etc.
>> than go to vm -> click stop -> go to vm -> click stop
> 
> It's faster to use the context menu of the tree or the one from the
> global search though.
> But faster is IMO the wrong thing to look at here anyway, as using
> bulk actions for a single action feels always wrong conceptually,
> so devfinitively won't be a common path for users.
> If, there would need ot be a situation when someone wants to do a bulk
> action, but circumstances change midway, after they opened the dialog,
> so that they only need to stop/start/... a single guest now.
> And even then they can still do that without the extra field (deselect
> all, select single guest).
> 
> So IMO adding a whole field for this niche use case seems rather
> overkill to me.

ok, i'll remove it

> 
>>> - tag space is very limited, maybe default to using circles or even
>>>     dense (I'm still wishing a bit for seeing the tag value on hover, like
>>>     a tool tip), or re-use the tree-style shape.
>>>     One alternative might be to add vertical scrolling here, but that is
>>>     probably rather odd (and not sure if that would even work easily
>>>     here).
>>
>> scrolling in two dimensions for different containers is always a bit weird
>> imho, but yeah making the tags 'dense' 'circle' or reusing the treestyle makes sense
> 
> Yeah, additional scrolling is indeed not the nicest thing to add here.
> 
>>
>> i agree with the tooltip, but last time i looked at it that was not so easy
>> because we already have a tooltip in the tree (and we reuse the rendering
>> for that) but i could see if i could add a tooltip to the whole cell
>> here that prints all tags in a nice way, what do you think?
> 
> I mean, for here there would not be a tool tip yet and we could have
> slightly different behavior here than in the tree any way.
> 
> But for the tree: How about reducing the scope of the current status
> tooltip from the whole row to the icon + text part only, so tags can
> have a separate, per tag one.

sadly including the icon is only possible if we overwrite the whole template of the treecolumn
here, since the icon div is seperated from the text div without a possibility
to set additional data there

(you can check out the template here: 
https://docs.sencha.com/extjs/7.0.0/classic/src/Column.js-2.html maybe you see a way to do this easier?)