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 EFEE0A01E for ; Mon, 4 Apr 2022 11:26:51 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E0D47AF2D for ; Mon, 4 Apr 2022 11:26:21 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id D1454AF19 for ; Mon, 4 Apr 2022 11:26:19 +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 8A27E46015 for ; Mon, 4 Apr 2022 11:26:19 +0200 (CEST) Message-ID: <0cdb2378-9ca4-9348-303c-bab46acbc1d6@proxmox.com> Date: Mon, 4 Apr 2022 11:25:57 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Content-Language: en-US To: Thomas Lamprecht , Proxmox VE development discussion References: <20220401140758.1997754-1-d.tschlatscher@proxmox.com> <133ace32-ee00-e923-30b4-f33747dc3f1c@proxmox.com> From: Daniel Tschlatscher In-Reply-To: <133ace32-ee00-e923-30b4-f33747dc3f1c@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.478 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% HTML_MESSAGE 0.001 HTML included in message KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.631 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 - Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Mon, 04 Apr 2022 09:26:52 -0000 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 >> --- >> 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(); >From t.lamprecht@proxmox.com Mon Apr 4 12:02:13 2022 Return-Path: X-Original-To: pve-devel@lists.proxmox.com Delivered-To: pve-devel@lists.proxmox.com 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 351F0A089 for ; Mon, 4 Apr 2022 12:02:13 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 33386B7F7 for ; Mon, 4 Apr 2022 12:02:13 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 79066B7EE for ; Mon, 4 Apr 2022 12:02:12 +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 5132B4586A for ; Mon, 4 Apr 2022 12:02:12 +0200 (CEST) Message-ID: <2a4863fa-2d27-f088-f381-09e02f9180c4@proxmox.com> Date: Mon, 4 Apr 2022 12:01:55 +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: Daniel Tschlatscher , Proxmox VE development discussion References: <20220401140758.1997754-1-d.tschlatscher@proxmox.com> <133ace32-ee00-e923-30b4-f33747dc3f1c@proxmox.com> <0cdb2378-9ca4-9348-303c-bab46acbc1d6@proxmox.com> From: Thomas Lamprecht In-Reply-To: <0cdb2378-9ca4-9348-303c-bab46acbc1d6@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.370 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.631 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: Mon, 04 Apr 2022 10:02:13 -0000 On 04.04.22 11:25, Daniel Tschlatscher wrote: >> note=C2=A0that=C2=A0in=C2=A0JavaScript=C2=A0we=C2=A0go=C2=A0for=C2=A01= 00=C2=A0characters=C2=A0text-width.=C2=A0Also,=C2=A0template-strings=C2=A0= can >> make=C2=A0a=C2=A0mix=C2=A0of=C2=A0string=C2=A0literals=C2=A0plus=C2=A0= variables=C2=A0often=C2=A0shorter=C2=A0or=C2=A0at=C2=A0least=C2=A0easier=C2= =A0to=C2=A0read: >> >> let=C2=A0fileName=C2=A0=3D=C2=A0`${rec.user}@${rec.node}-${rec.type}..= =2E` >> >> albeit=C2=A0I'd=C2=A0maybe=C2=A0just=C2=A0go=C2=A0for=C2=A0the=C2=A0up= id=C2=A0as=C2=A0is,=C2=A0at=C2=A0least=C2=A0avoid=C2=A0encoding=C2=A0non-= identifying=C2=A0info >> like=C2=A0exit=C2=A0status=C2=A0in=C2=A0the=C2=A0file=C2=A0name. >=20 > Alright, I read in the developer documentation that line width should n= ot=C2=A0exceed=C2=A080=C2=A0characters > apart from some special cases. I thought that going for 100 chars would= n't=C2=A0increase=C2=A0read- > ability=C2=A0by=C2=A0enough=C2=A0here. I mean, you also did not really maxed out 80cc either FWICT ;-) But anyho= w, thanks for the pointer about the outdated style guides, I updated those in that regard. >=20 > So, can I safely make statements up to 100 chars or do you mean to just= change=C2=A0it=C2=A0here? >=20 yes, please do. > I didn't want to use the UPID as a filename because I think it is not v= ery=C2=A0human=C2=A0friendly=C2=A0and > can be quite hard to identify, especially if there are multiple tasklog= files=C2=A0in=C2=A0the=C2=A0same=C2=A0folder. > The main reason for the usage here is because I wanted to include a hum= an=C2=A0readable=C2=A0way > of telling the date and time in the filename (as opposed to UNIX second= s in=C2=A0the=C2=A0UPID) The UPID is used in many places and isn't included in the task log itself= , so loosing that single full ID of a task can make cross-correlation harder. So lets please included it, if you want to have humand readable info in a= ddition I'd go for something like (pseudo code): `${taskUPID}-${taskStartDateISO8601}.log` >=20 >>> + >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0Proxmox.Utils.textToFile(fileName,=C2=A0fileContent); >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}); >>> +=C2=A0=C2=A0=C2=A0=C2=A0}; >>> + >>> +=C2=A0=C2=A0=C2=A0=C2=A0copyBtn.handler=C2=A0=3D=C2=A0function()=C2=A0= { >> why=C2=A0not=C2=A0defined=C2=A0the=C2=A0handler=C2=A0directly=C2=A0whe= n=C2=A0instantiating=C2=A0above,=C2=A0would=C2=A0improve=C2=A0code=C2=A0l= ocality >> a=C2=A0bit. >=20 > Declaring the handlers down here was actually a deliberate decision as = these=C2=A0"3=C2=A0parts"=C2=A0of=C2=A0the > code=C2=A0above=C2=A0all=C2=A0depend=C2=A0on=C2=A0each=C2=A0other.=C2=A0= Going=C2=A0from=C2=A0bottom=C2=A0to=C2=A0top: >=20 > 1=C2=A0-=C2=A0_downloadLogFull()_=C2=A0function=C2=A0needs=C2=A0_let=C2= =A0logView_=C2=A0to=C2=A0be=C2=A0declared. > 2 - _let logView_ needs the _let copyBtn_ and _let downloadBtn_ to be d= eclared > 3=C2=A0-=C2=A0The=C2=A0button=C2=A0handlers=C2=A0need=C2=A0function=C2=A0= _downloadLogFull()_=C2=A0to=C2=A0be=C2=A0declared I'd see such needs resulting from tight coupling rather as a sign of code= smell. And I only saw the direct access to private parts of the logView's viewMo= del now that you explicitly mentioned 1.), lets please don't do that, _if_ we req= uire the access to the data then lets go overt the actual public interfaces, reque= sting the viewModel from logView and then the data via viewModel's get(key) method.= But actually taking a step back and questioning the reason for being requ= ired to do this: We really don't care about limit, we just want all - so what we act= ually would like to have is telling the backend to give us everything via passing `li= mit =3D 0` explicitly. That requires trivial changes in PVE::Tools::dump_logfile but= would make for a cleaner approach. Besides from that, logs can get huge your current approach copies everyth= ing in memory, which may not be returned to the OS immediately as perl normally keeps al= located memory. While its mainly for reuse, huge and/or parallel-triggered allocations ca= n still make a big memory impact (easy DOS target for users with only very basic privs= =2E) So, for the limit=3D=3D0 "give me everything" case and no filter set in t= he task log api we may not even want to call the dump_logfile but just open the file and ret= urn in to the webserver so that it can stream it efficiently, directly from the file. We recently switched over the syslog/journal api call to that for similar= reasons. long story short, that not only would make this all much more efficient, = it'd also avoid some ugly coupling.