[pbs-devel] [PATCH proxmox-backup 1/2] gui: remove document.execCommand calls

[pbs-devel] [PATCH proxmox-backup 1/2] gui: remove document.execCommand calls

[Gabriel Goller]

The `document.execCommand` call is deprecated since a few years [0] so I
went ahead and removed it. We only use it to copy stuff to the clipboard
and the recommended way now is to use `navigator.clipboard.writeText`
[1]. `writeText` is kind of new, but I think we'll be alright regarding
compatibility (Compat table is also available at [1]).

[0]: https://developer.mozilla.org/en-US/docs/Web/API/document/execCommand
[1]: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 www/ServerStatus.js             | 8 +++-----
 www/panel/NodeInfo.js           | 5 ++---
 www/window/DatastoreRepoInfo.js | 5 ++---
 www/window/TokenEdit.js         | 6 +++---
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/www/ServerStatus.js b/www/ServerStatus.js
index 0c105c63..770deecb 100644
--- a/www/ServerStatus.js
+++ b/www/ServerStatus.js
@@ -76,11 +76,9 @@ Ext.define('PBS.ServerStatus', {
 		{
 		    xtype: 'button',
 		    iconCls: 'fa fa-clipboard',
-		    handler: function(button) {
-			window.getSelection().selectAllChildren(
-			    document.getElementById('pkgversions'),
-			);
-			document.execCommand("copy");
+		    handler: async function(button) {
+			let el = document.getElementById('pkgversions');
+			await navigator.clipboard.writeText(el.textContent);
 		    },
 		    text: gettext('Copy'),
 		},
diff --git a/www/panel/NodeInfo.js b/www/panel/NodeInfo.js
index 454986ef..9d741e45 100644
--- a/www/panel/NodeInfo.js
+++ b/www/panel/NodeInfo.js
@@ -45,10 +45,9 @@ Ext.define('PBS.NodeInfoPanel', {
 		    {
 			xtype: 'button',
 			iconCls: 'fa fa-clipboard',
-			handler: function(b) {
+			handler: async function(b) {
 			    var el = document.getElementById('fingerprintField');
-			    el.select();
-			    document.execCommand("copy");
+			    await navigator.clipboard.writeText(el.value);
 			},
 			text: gettext('Copy'),
 		    },
diff --git a/www/window/DatastoreRepoInfo.js b/www/window/DatastoreRepoInfo.js
index e862d7ad..2f2db477 100644
--- a/www/window/DatastoreRepoInfo.js
+++ b/www/window/DatastoreRepoInfo.js
@@ -113,15 +113,14 @@ Ext.define('PBS.form.CopyField', {
 	    iconCls: 'fa fa-clipboard x-btn-icon-el-default-toolbar-small',
 	    baseCls: 'x-btn',
 	    cls: 'x-btn-default-toolbar-small proxmox-inline-button',
-	    handler: function() {
+	    handler: async function() {
 		let me = this;
 		let field = me.up('pbsCopyField');
 		let el = field.getComponent('inputField')?.inputEl;
 		if (!el?.dom) {
 		    return;
 		}
-		el.dom.select();
-		document.execCommand("copy");
+		await navigator.clipboard.writeText(el.dom.value);
 	    },
 	    text: gettext('Copy'),
 	},
diff --git a/www/window/TokenEdit.js b/www/window/TokenEdit.js
index 80540212..c1856be8 100644
--- a/www/window/TokenEdit.js
+++ b/www/window/TokenEdit.js
@@ -203,9 +203,9 @@ Ext.define('PBS.window.TokenShow', {
     ],
     buttons: [
 	{
-	    handler: function(b) {
-		document.getElementById('token-secret-value').select();
-		document.execCommand("copy");
+	    handler: async function(b) {
+		let el = document.getElementById('token-secret-value');
+		await navigator.clipboard.writeText(el.value);
 	    },
 	    text: gettext('Copy Secret Value'),
 	},
-- 
2.43.0

[pbs-devel] [PATCH proxmox-backup 2/2] fix #5188: gui: add copy to clipboard on snapshots

[Gabriel Goller]

When navigating to Datastores -> Content, it is now possible to
right-click on a snapshot/group and copy the name to the clipboard.
This makes the proxmox-backup-client much easier to use, especially when
restoring archives.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 www/datastore/Content.js | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/www/datastore/Content.js b/www/datastore/Content.js
index fa0b7d2c..b2d6a053 100644
--- a/www/datastore/Content.js
+++ b/www/datastore/Content.js
@@ -546,6 +546,10 @@ Ext.define('PBS.DataStoreContent', {
 	    });
 	},
 
+	onCopy: async function(view, rI, cI, item, e, { data }) {
+	    await navigator.clipboard.writeText(data.text);
+	},
+
 	onNotesEdit: function(view, data) {
 	    let me = this;
 
@@ -884,6 +888,7 @@ Ext.define('PBS.DataStoreContent', {
 	    if (record.data.ty === 'group') {
 		menu = Ext.create('PBS.datastore.GroupCmdMenu', {
 		    title: gettext('Group'),
+		    onCopy: createControllerCallback('onCopy'),
 		    onVerify: createControllerCallback('onVerify'),
 		    onChangeOwner: createControllerCallback('onChangeOwner'),
 		    onPrune: createControllerCallback('onPrune'),
@@ -892,6 +897,7 @@ Ext.define('PBS.DataStoreContent', {
 	    } else if (record.data.ty === 'dir') {
 		menu = Ext.create('PBS.datastore.SnapshotCmdMenu', {
 		    title: gettext('Snapshot'),
+		    onCopy: createControllerCallback('onCopy'),
 		    onVerify: createControllerCallback('onVerify'),
 		    onProtectionChange: createControllerCallback('onProtectionChange'),
 		    onForget: createControllerCallback('onForget'),
@@ -1284,12 +1290,21 @@ Ext.define('PBS.datastore.GroupCmdMenu', {
     extend: 'Ext.menu.Menu',
     mixins: ['Proxmox.Mixin.CBind'],
 
+    onCopy: undefined,
     onVerify: undefined,
     onChangeOwner: undefined,
     onPrune: undefined,
     onForget: undefined,
 
     items: [
+	{
+	    text: gettext('Copy'),
+	    iconCls: 'fa fa-clipboard',
+	    handler: function() { this.up('menu').onCopy(); },
+	    cbind: {
+		hidden: '{!onCopy}',
+	    },
+	},
 	{
 	    text: gettext('Verify'),
 	    iconCls: 'pve-icon-verify-lettering',
@@ -1330,11 +1345,21 @@ Ext.define('PBS.datastore.SnapshotCmdMenu', {
     extend: 'Ext.menu.Menu',
     mixins: ['Proxmox.Mixin.CBind'],
 
+    onCopy: undefined,
     onVerify: undefined,
     onProtectionChange: undefined,
     onForget: undefined,
 
     items: [
+	{
+	    text: gettext('Copy'),
+	    iconCls: 'fa fa-clipboard',
+	    handler: function() { this.up('menu').onCopy(); },
+	    cbind: {
+		hidden: '{!onCopy}',
+		disabled: '{!onCopy}',
+	    },
+	},
 	{
 	    text: gettext('Verify'),
 	    iconCls: 'pve-icon-verify-lettering',
-- 
2.43.0

Re: [pbs-devel] [PATCH proxmox-backup 1/2] gui: remove document.execCommand calls

[Dominik Csapak]

hi,

just to note: changing to an async function can be rather dangerous,
sometimes extjs not only calls the handlers/events/etc. but does things
after them (expecting the function to be finished) or waits for the return value.
(most of the extjs code was from before async/await was a thing in js)

so here it seems to work out fine, but we have to be careful with
sprinkling async function in the code, otherwise we'll get
very unexpected results

in general, i'd like to see that mentioned in the commit message
why it's ok to do that (no hard feelings though)

otherwise LGTM

Re: [pbs-devel] [PATCH proxmox-backup 2/2] fix #5188: gui: add copy to clipboard on snapshots

[Dominik Csapak]

same comment as the previous patch, some note about why async is
ok would be nice

one high level comment:

i'd probably rename the button from 'copy' to something like 'copy name to clipboard'

otherwise it can be confusing as to what the button does, since 'copy' could
also mean duplicate the snapshot ;)

rest of the patch LGTM

On 3/13/24 17:17, Gabriel Goller wrote:
> When navigating to Datastores -> Content, it is now possible to
> right-click on a snapshot/group and copy the name to the clipboard.
> This makes the proxmox-backup-client much easier to use, especially when
> restoring archives.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>   www/datastore/Content.js | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/www/datastore/Content.js b/www/datastore/Content.js
> index fa0b7d2c..b2d6a053 100644
> --- a/www/datastore/Content.js
> +++ b/www/datastore/Content.js
> @@ -546,6 +546,10 @@ Ext.define('PBS.DataStoreContent', {
>   	    });
>   	},
>   
> +	onCopy: async function(view, rI, cI, item, e, { data }) {
> +	    await navigator.clipboard.writeText(data.text);
> +	},
> +
>   	onNotesEdit: function(view, data) {
>   	    let me = this;
>   
> @@ -884,6 +888,7 @@ Ext.define('PBS.DataStoreContent', {
>   	    if (record.data.ty === 'group') {
>   		menu = Ext.create('PBS.datastore.GroupCmdMenu', {
>   		    title: gettext('Group'),
> +		    onCopy: createControllerCallback('onCopy'),
>   		    onVerify: createControllerCallback('onVerify'),
>   		    onChangeOwner: createControllerCallback('onChangeOwner'),
>   		    onPrune: createControllerCallback('onPrune'),
> @@ -892,6 +897,7 @@ Ext.define('PBS.DataStoreContent', {
>   	    } else if (record.data.ty === 'dir') {
>   		menu = Ext.create('PBS.datastore.SnapshotCmdMenu', {
>   		    title: gettext('Snapshot'),
> +		    onCopy: createControllerCallback('onCopy'),
>   		    onVerify: createControllerCallback('onVerify'),
>   		    onProtectionChange: createControllerCallback('onProtectionChange'),
>   		    onForget: createControllerCallback('onForget'),
> @@ -1284,12 +1290,21 @@ Ext.define('PBS.datastore.GroupCmdMenu', {
>       extend: 'Ext.menu.Menu',
>       mixins: ['Proxmox.Mixin.CBind'],
>   
> +    onCopy: undefined,
>       onVerify: undefined,
>       onChangeOwner: undefined,
>       onPrune: undefined,
>       onForget: undefined,
>   
>       items: [
> +	{
> +	    text: gettext('Copy'),
> +	    iconCls: 'fa fa-clipboard',
> +	    handler: function() { this.up('menu').onCopy(); },
> +	    cbind: {
> +		hidden: '{!onCopy}',
> +	    },
> +	},
>   	{
>   	    text: gettext('Verify'),
>   	    iconCls: 'pve-icon-verify-lettering',
> @@ -1330,11 +1345,21 @@ Ext.define('PBS.datastore.SnapshotCmdMenu', {
>       extend: 'Ext.menu.Menu',
>       mixins: ['Proxmox.Mixin.CBind'],
>   
> +    onCopy: undefined,
>       onVerify: undefined,
>       onProtectionChange: undefined,
>       onForget: undefined,
>   
>       items: [
> +	{
> +	    text: gettext('Copy'),
> +	    iconCls: 'fa fa-clipboard',
> +	    handler: function() { this.up('menu').onCopy(); },
> +	    cbind: {
> +		hidden: '{!onCopy}',
> +		disabled: '{!onCopy}',
> +	    },
> +	},
>   	{
>   	    text: gettext('Verify'),
>   	    iconCls: 'pve-icon-verify-lettering',

Re: [pbs-devel] [PATCH proxmox-backup 1/2] gui: remove document.execCommand calls

[Gabriel Goller]

On Thu Mar 14, 2024 at 8:59 AM CET, Dominik Csapak wrote:
> hi,
>
> just to note: changing to an async function can be rather dangerous,
> sometimes extjs not only calls the handlers/events/etc. but does things
> after them (expecting the function to be finished) or waits for the return value.

Hmm, but does it actually **await** the return value? I'm not a ext.js
expert but I don't think it is. Anyways it's fine because js promises
are executed eagerly + promise returned directly.

> (most of the extjs code was from before async/await was a thing in js)
>
> so here it seems to work out fine, but we have to be careful with
> sprinkling async function in the code, otherwise we'll get
> very unexpected results
>
> in general, i'd like to see that mentioned in the commit message
> why it's ok to do that (no hard feelings though)
>
> otherwise LGTM

What do you think about adding this to the commit message:

    Making the handler functions async is not a problem, because
    promises in js are executed eagerly (not lazily) and nothing
    depends/waits on the result of this handler.

Re: [pbs-devel] [PATCH proxmox-backup 2/2] fix #5188: gui: add copy to clipboard on snapshots

[Gabriel Goller]

On Thu Mar 14, 2024 at 9:01 AM CET, Dominik Csapak wrote:
> same comment as the previous patch, some note about why async is
> ok would be nice
>
> one high level comment:
>
> i'd probably rename the button from 'copy' to something like 'copy name to clipboard'
> 
> otherwise it can be confusing as to what the button does, since 'copy' could
> also mean duplicate the snapshot ;)

I agree, will rename it in v2.

> rest of the patch LGTM

Thanks for the review!

Re: [pbs-devel] [PATCH proxmox-backup 1/2] gui: remove document.execCommand calls

[Dominik Csapak]

On 3/14/24 10:08, Gabriel Goller wrote:
> On Thu Mar 14, 2024 at 8:59 AM CET, Dominik Csapak wrote:
>> hi,
>>
>> just to note: changing to an async function can be rather dangerous,
>> sometimes extjs not only calls the handlers/events/etc. but does things
>> after them (expecting the function to be finished) or waits for the return value.
> 
> Hmm, but does it actually **await** the return value? I'm not a ext.js
> expert but I don't think it is. Anyways it's fine because js promises
> are executed eagerly + promise returned directly.

no it does not awaits anything and thats exactly the problem,
for example (not a real one)

extjs expects a callback foo
after it calls it, it expects foo to be finished to do further work
(e.g. a cleanup after an event handler)

we give an async function as foo, with multiple await calls in there

now it's not guaranteed the callback finished when extjs expects it to

> 
>> (most of the extjs code was from before async/await was a thing in js)
>>
>> so here it seems to work out fine, but we have to be careful with
>> sprinkling async function in the code, otherwise we'll get
>> very unexpected results
>>
>> in general, i'd like to see that mentioned in the commit message
>> why it's ok to do that (no hard feelings though)
>>
>> otherwise LGTM
> 
> What do you think about adding this to the commit message:
> 
>      Making the handler functions async is not a problem, because
>      promises in js are executed eagerly (not lazily) and nothing
>      depends/waits on the result of this handler.

as i mentioned above i don't think that's the reason why it's okay.

it should be okay because extjs executes the handler and does not expect
any result from it, nor does it need to do some work afterwards

Re: [pbs-devel] [PATCH proxmox-backup 1/2] gui: remove document.execCommand calls

[Gabriel Goller]

Thanks for the explanation, expect a v2 shortly!