From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 65B109935 for ; Fri, 1 Apr 2022 17:19:16 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 50F1928C7C for ; Fri, 1 Apr 2022 17:18:46 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 2BC6B28C6E for ; Fri, 1 Apr 2022 17:18:44 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id E5A6B459CC for ; Fri, 1 Apr 2022 17:18:43 +0200 (CEST) Message-ID: <133ace32-ee00-e923-30b4-f33747dc3f1c@proxmox.com> Date: Fri, 1 Apr 2022 17:18:27 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:99.0) Gecko/20100101 Thunderbird/99.0 Content-Language: en-US To: Proxmox VE development discussion , Daniel Tschlatscher References: <20220401140758.1997754-1-d.tschlatscher@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20220401140758.1997754-1-d.tschlatscher@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.055 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH widget-toolkit] fix #3971: Download and copy button in the TaskViewer X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Apr 2022 15:19:16 -0000 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 > --- > 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();