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

[Daniel Tschlatscher]

On 4/1/22 17:18, Thomas Lamprecht wrote:
> 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 at 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.

Alright, I read in the developer documentation that line width should 
not exceed 80 characters
apart from some special cases. I thought that going for 100 chars 
wouldn't increase read-
ability by enough here.

So, can I safely make statements up to 100 chars or do you mean to just 
change it here?

I didn't want to use the UPID as a filename because I think it is not 
very human friendly and
can be quite hard to identify, especially if there are multiple tasklog 
files in the same folder.
The main reason for the usage here is because I wanted to include a 
human readable way
of telling the date and time in the filename (as opposed to UNIX seconds 
in the UPID)

>> +
>> +		    Proxmox.Utils.textToFile(fileName, fileContent);
>> +		}
>> +	    });
>> +	};
>> +
>> +	copyBtn.handler = function() {
> why not defined the handler directly when instantiating above, would improve code locality
> a bit.

Declaring the handlers down here was actually a deliberate decision as 
these "3 parts" of the
code above all depend on each other. Going from bottom to top:

1 - _downloadLogFull()_ function needs _let logView_ to be declared.
2 - _let logView_ needs the _let copyBtn_ and _let downloadBtn_ to be 
declared
3 - The button handlers need function _downloadLogFull()_ to be declared

In the button declaration, the handlers would be declared before the 
function is. Therefore
I moved the declaration of the handlers below the function declaration 
as to break this chain.

I actually included a comment for this but have accidentally deleted it.


Also, thanks for the review! I will adjust things for v2

>> +	    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();