Re: [pve-devel] [PATCH widget-toolkit] fix #3971: Download and copy button in the TaskViewer

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 01.04.22 16:07, Daniel Tschlatscher wrote:
> The taskviewer now has 2 more buttons which implement
> functionality for downloading the current tasklog as a file
> or copying it to the clipboard. The code for saving the log
> to a file was taken from the pve System Report class and
> moved to its own function in the Utils file.

just fyi, 70cc is a good text-width limit for commit messages, neither too
long nor too short is ideal to read, as said, fyi as I'm surely not going to
nack stuff just based on to narrow limits though, albeit I may fix those up
with an amend on apply ;-)

> Tested on Firefox 91.7.0esr and Chromium Version 99

Which products did you test? The task log viewer is available on PVE, PMG and
PBS after all.

Assuming it works on all products the semantics look OK, I see quite some
improvements style wise and some chance to drop legacy code now, commented both
inline.

> 
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
> ---
>  src/Utils.js             | 17 +++++++++++
>  src/window/TaskViewer.js | 65 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/src/Utils.js b/src/Utils.js
> index 6a03057..3fff3da 100644
> --- a/src/Utils.js
> +++ b/src/Utils.js
> @@ -1272,6 +1272,23 @@ utilities: {
>  	    .map(val => val.charCodeAt(0)),
>  	);
>      },
> +
> +    // Convert the given string to a file and "download" it locally
> +    textToFile: function(fileName, fileContent) {

I'd call that 'downloadTextAsFile' or maybe more JS "type" related:
'downloadStringAsFile' and s/fileContent/text/

> +	// Internet Explorer

I think as we dropped IE support with adopting modern es6 a while ago we can now
drop such things too.

> +	if (window.navigator.msSaveOrOpenBlob) {
> +	    navigator.msSaveOrOpenBlob(new Blob([fileContent]), fileName);
> +	} else {
> +	    var element = document.createElement('a');

please not only use `let` for local variables in new code but also transform
existing code from var to let if you have to touch it anyway. `var` just has
horrible scoping rules (becomes visible in the parent's block scope) and should
be avoided on principle, if possible.

> +	    element.setAttribute('href', 'data:text/plain;charset=utf-8,' +
> +		encodeURIComponent(fileContent));
> +	    element.setAttribute('download', fileName);
> +	    element.style.display = 'none';
> +	    document.body.appendChild(element);
> +	    element.click();
> +	    document.body.removeChild(element);

you do not need to append the created element to the body anymore, at least
I didn't when adapting such a function for the PVE PBS-storage UI key-secret stuff.

https://git.proxmox.com/?p=pve-manager.git;a=blob;f=www/manager6/storage/PBSEdit.js;h=b46ddf71cc5d#l87

> +	}
> +    },
>  },
>  
>      singleton: true,
> diff --git a/src/window/TaskViewer.js b/src/window/TaskViewer.js
> index 996a41b..bf4533d 100644
> --- a/src/window/TaskViewer.js
> +++ b/src/window/TaskViewer.js
> @@ -229,13 +229,75 @@ Ext.define('Proxmox.window.TaskViewer', {
>  	    border: false,
>  	});
>  
> +	let downloadBtn = new Ext.Button({
> +		text: gettext('Download'),

missing download icon class, also move handler here (see below)

> +	});
> +
> +	let copyBtn = new Ext.Button({
> +		text: gettext('Copy'),
> +		iconCls: 'fa fa-clipboard',
> +	});
> +
>  	let logView = Ext.create('Proxmox.panel.LogView', {
>  	    title: gettext('Output'),
> -	    tbar: [stop_btn2],
> +	    tbar: [stop_btn2, '->', downloadBtn, copyBtn],
>  	    border: false,
>  	    url: "/api2/extjs/nodes/" + task.node + "/tasks/" + encodeURIComponent(me.upid) + "/log",
>  	});
>  
> +	const downloadLogFull = function(callback) {> +	    Proxmox.Utils.API2Request({
> +		url: "/nodes/" + task.node + "/tasks/"
> +		    + encodeURIComponent(me.upid) + '/log',

please use template strings for such things, shorter and easier to read, fits into a
single 100cc line too.

> +		waitMsgTarget: me,
> +		method: 'GET',
> +		params: {
> +		    'limit': logView.viewModel.data.data.total,
> +		},
> +		failure: function(response, opts) {
> +		    callback(response.htmlStatus, null);
> +		},

we normally use modern arrow fn for such edge-case oneliner's nowadays:

failure: response => callback(response.htmlStatus, null),

but actually, why even bother with a callback if all instances using this just Ext.alert
the error anyway? Only complicates things here and in the instances too, just do:

failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),

and you can drop the error handling in the handlers too..

> +		success: function(response) {
> +		    let fileContent = "";
> +
> +		    response.result.data.forEach((line) => {
> +			fileContent += `${line.t}\n`;
> +		    });
> +
> +		    callback(null, fileContent);
> +		},
> +	    });
> +	};
> +
> +	downloadBtn.handler = function() {
why not defined the handler directly when instantiating above, would improve code locality
a bit.

> +	    downloadLogFull((errStatus, fileContent) => {
> +		if (errStatus) {
> +		    Ext.Msg.alert(gettext('Error'), errStatus);
> +		} else {
> +		    let record = statstore.getRecord().data;
> +		    let fileName = record.user + "@" + record.node + "_" +
> +			record.type + "_" + record.pid + "_" +
> +			Proxmox.Utils.render_timestamp(record.starttime) +
> +			"_" + record.exitstatus + ".log";

note that in JavaScript we go for 100 characters text-width. Also, template-strings can
make a mix of string literals plus variables often shorter or at least easier to read:

let fileName = `${rec.user}@${rec.node}-${rec.type}...`

albeit I'd maybe just go for the upid as is, at least avoid encoding non-identifying info
like exit status in the file name. 

> +
> +		    Proxmox.Utils.textToFile(fileName, fileContent);
> +		}
> +	    });
> +	};
> +
> +	copyBtn.handler = function() {

why not defined the handler directly when instantiating above, would improve code locality
a bit.

> +	    downloadLogFull((errStatus, response) => {
> +		if (errStatus) {
> +		    Ext.Msg.alert(gettext('Error'), errStatus);
> +		} else {
> +		    navigator.clipboard.writeText(response)
> +		    .catch((err) => {
> +			Ext.Msg.alert(gettext('Error'), err);
> +		    });
> +		}
> +	    });
> +	};
> +
>  	me.mon(statstore, 'load', function() {
>  	    let status = statgrid.getObjectValue('status');
>  
> @@ -248,6 +310,7 @@ Ext.define('Proxmox.window.TaskViewer', {
>  
>  	    stop_btn1.setDisabled(status !== 'running');
>  	    stop_btn2.setDisabled(status !== 'running');
> +	    downloadBtn.setDisabled(status === 'running');
>  	});
>  
>  	statstore.startUpdate();