[pve-devel] [PATCH widget-toolkit 0/2] notification: set 'Remove' button text to 'Reset to default' for built-ins

[pve-devel] [PATCH widget-toolkit 0/2] notification: set 'Remove' button text to 'Reset to default' for built-ins

[Lukas Wagner]

Deleting a built-in target/matcher does not remove it, but resets it 
to its default settings. This was not really obvious from the UI. 
This patch changes the 'Remove' button text based on the
selected target/matcher. If it is a built-in, the button text is
changed to 'Reset to default'. Also, if the built-in is not actually
modified, the button is disabled.

Lukas Wagner (2):
  remove button: allow to set custom confirmation message
  notify: change 'Remove' button to 'Reset to default' for built-ins

 src/button/Button.js                | 10 ++++-
 src/panel/NotificationConfigView.js | 63 +++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 1 deletion(-)

-- 
2.39.2

[pve-devel] [PATCH widget-toolkit 1/2] remove button: allow to set custom confirmation message

[Lukas Wagner]

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/button/Button.js | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/button/Button.js b/src/button/Button.js
index 1f53869..a59a999 100644
--- a/src/button/Button.js
+++ b/src/button/Button.js
@@ -110,6 +110,7 @@ Ext.define('Proxmox.button.StdRemoveButton', {
 
     config: {
 	baseurl: undefined,
+	customConfirmationMessage: undefined,
     },
 
     getUrl: function(rec) {
@@ -133,7 +134,14 @@ Ext.define('Proxmox.button.StdRemoveButton', {
 	let me = this;
 
 	let name = me.getRecordName(rec);
-	return Ext.String.format(gettext('Are you sure you want to remove entry {0}'), `'${name}'`);
+
+	let text;
+	if (me.customConfirmationMessage) {
+	    text = me.customConfirmationMessage;
+	} else {
+	    text = gettext('Are you sure you want to remove entry {0}');
+	}
+	return Ext.String.format(text, `'${name}'`);
     },
 
     handler: function(btn, event, rec) {
-- 
2.39.2

[pve-devel] [PATCH widget-toolkit 2/2] notify: change 'Remove' button to 'Reset to default' for built-ins

[Lukas Wagner]

A HTTP DELETE for a built-in target/matcher acts as a reset to its
defaults. This patch changes the 'Remove' button text based on the
selected target/matcher. If it is a built-in, the button text is
changed to 'Reset to default'. Also, if the built-in is not actually
modified, the button is disabled.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/panel/NotificationConfigView.js | 63 +++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/src/panel/NotificationConfigView.js b/src/panel/NotificationConfigView.js
index 4d3ee46..2e2eede 100644
--- a/src/panel/NotificationConfigView.js
+++ b/src/panel/NotificationConfigView.js
@@ -1,3 +1,32 @@
+Ext.define('Proxmox.panel.NotificationConfigViewModel', {
+    extend: 'Ext.app.ViewModel',
+
+    alias: 'viewmodel.pmxNotificationConfigPanel',
+
+    formulas: {
+	builtinSelected: function(get) {
+	    let origin = get('selection')?.get('origin');
+	    return origin === 'modified-builtin' || origin === 'builtin';
+	},
+	removeButtonText: function(get) {
+	    if (get('builtinSelected')) {
+		return gettext('Reset to default');
+	    } else {
+		return gettext('Remove');
+	    }
+	},
+	removeButtonConfirmMessage: function(get) {
+	    if (get('builtinSelected')) {
+		return gettext('Do you want to reset {0} to its default settings?');
+	    } else {
+		// Use default message provided by the button
+		return undefined;
+	    }
+	},
+    },
+
+});
+
 Ext.define('Proxmox.panel.NotificationConfigView', {
     extend: 'Ext.panel.Panel',
     alias: 'widget.pmxNotificationConfigView',
@@ -36,6 +65,14 @@ Ext.define('Proxmox.panel.NotificationEndpointView', {
 
     title: gettext('Notification Targets'),
 
+    viewModel: {
+	type: 'pmxNotificationConfigPanel',
+    },
+
+    bind: {
+	selection: '{selection}',
+    },
+
     controller: {
 	xclass: 'Ext.app.ViewController',
 
@@ -70,6 +107,7 @@ Ext.define('Proxmox.panel.NotificationEndpointView', {
 	    let me = this;
 	    let view = me.getView();
 	    view.getStore().rstore.load();
+	    this.getView().setSelection(null);
 	},
 
 	testEndpoint: function() {
@@ -205,9 +243,17 @@ Ext.define('Proxmox.panel.NotificationEndpointView', {
 		{
 		    xtype: 'proxmoxStdRemoveButton',
 		    callback: 'reload',
+		    bind: {
+			text: '{removeButtonText}',
+			customConfirmationMessage: '{removeButtonConfirmMessage}',
+		    },
 		    getUrl: function(rec) {
 			return `${me.baseUrl}/endpoints/${rec.data.type}/${rec.getId()}`;
 		    },
+		    enableFn: (rec) => {
+			let origin = rec.get('origin');
+			return origin === 'user-created' || origin === 'modified-builtin';
+		    },
 		},
 		'-',
 		{
@@ -260,9 +306,18 @@ Ext.define('Proxmox.panel.NotificationMatcherView', {
 
 	reload: function() {
 	    this.getView().getStore().rstore.load();
+	    this.getView().setSelection(null);
 	},
     },
 
+    viewModel: {
+	type: 'pmxNotificationConfigPanel',
+    },
+
+    bind: {
+	selection: '{selection}',
+    },
+
     listeners: {
 	itemdblclick: 'openEditForSelectedItem',
 	activate: 'reload',
@@ -342,7 +397,15 @@ Ext.define('Proxmox.panel.NotificationMatcherView', {
 		{
 		    xtype: 'proxmoxStdRemoveButton',
 		    callback: 'reload',
+		    bind: {
+			text: '{removeButtonText}',
+			customConfirmationMessage: '{removeButtonConfirmMessage}',
+		    },
 		    baseurl: `${me.baseUrl}/matchers`,
+		    enableFn: (rec) => {
+			let origin = rec.get('origin');
+			return origin === 'user-created' || origin === 'modified-builtin';
+		    },
 		},
 	    ],
 	});
-- 
2.39.2

Re: [pve-devel] [PATCH widget-toolkit 0/2] notification: set 'Remove' button text to 'Reset to default' for built-ins

[Lukas Wagner]

ping

On 12/14/23 10:48, Lukas Wagner wrote:
> Deleting a built-in target/matcher does not remove it, but resets it
> to its default settings. This was not really obvious from the UI.
> This patch changes the 'Remove' button text based on the
> selected target/matcher. If it is a built-in, the button text is
> changed to 'Reset to default'. Also, if the built-in is not actually
> modified, the button is disabled.
> 
> Lukas Wagner (2):
>    remove button: allow to set custom confirmation message
>    notify: change 'Remove' button to 'Reset to default' for built-ins
> 
>   src/button/Button.js                | 10 ++++-
>   src/panel/NotificationConfigView.js | 63 +++++++++++++++++++++++++++++
>   2 files changed, 72 insertions(+), 1 deletion(-)
> 

-- 
- Lukas

Re: [pve-devel] [PATCH widget-toolkit 0/2] notification: set 'Remove' button text to 'Reset to default' for built-ins

[Lukas Wagner]

ping - still applies on master. Retested it quickly, also seems to work fine.

On 1/8/24 11:43, Lukas Wagner wrote:
> ping
> 
> On 12/14/23 10:48, Lukas Wagner wrote:
>> Deleting a built-in target/matcher does not remove it, but resets it
>> to its default settings. This was not really obvious from the UI.
>> This patch changes the 'Remove' button text based on the
>> selected target/matcher. If it is a built-in, the button text is
>> changed to 'Reset to default'. Also, if the built-in is not actually
>> modified, the button is disabled.
>>
>> Lukas Wagner (2):
>>    remove button: allow to set custom confirmation message
>>    notify: change 'Remove' button to 'Reset to default' for built-ins
>>
>>   src/button/Button.js                | 10 ++++-
>>   src/panel/NotificationConfigView.js | 63 +++++++++++++++++++++++++++++
>>   2 files changed, 72 insertions(+), 1 deletion(-)
>>
> 

-- 
- Lukas

[pve-devel] applied-series: [PATCH widget-toolkit 0/2] notification: set 'Remove' button text to 'Reset to default' for built-ins

[Thomas Lamprecht]

Am 14/12/2023 um 10:48 schrieb Lukas Wagner:
> Deleting a built-in target/matcher does not remove it, but resets it 
> to its default settings. This was not really obvious from the UI. 
> This patch changes the 'Remove' button text based on the
> selected target/matcher. If it is a built-in, the button text is
> changed to 'Reset to default'. Also, if the built-in is not actually
> modified, the button is disabled.
> 
> Lukas Wagner (2):
>   remove button: allow to set custom confirmation message
>   notify: change 'Remove' button to 'Reset to default' for built-ins
> 
>  src/button/Button.js                | 10 ++++-
>  src/panel/NotificationConfigView.js | 63 +++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+), 1 deletion(-)
> 


applied series, thanks!

But I reduced the text from "Reset to default" to just "Reset", the
confirmation message is telling enough for this nice case to avoid all
to much jumping of UI elements.

Note that we normally even use the proxmoxAltTextButton, which sets its
width such, that it doesn't have to change size when changing between
two (statically set) texts – like for "Remove" and "Detach" in the VM
hardware panel for non-disks vs. disks.

But this could be done afterwards, personally it's not a no-go for layout
changes, not sure how good we're currently to avoid that, but maybe
reusing that component could be worth it here.