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 5019A9E228 for ; Mon, 27 Nov 2023 11:05:22 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2F00D3FAF for ; Mon, 27 Nov 2023 11:04:52 +0100 (CET) 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 for ; Mon, 27 Nov 2023 11:04:51 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5CCBA44A4B; Mon, 27 Nov 2023 11:04:51 +0100 (CET) Message-ID: <895e307e-94f7-45d2-b094-f4fe464d4ae9@proxmox.com> Date: Mon, 27 Nov 2023 11:04:50 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Content-Language: en-US To: Thomas Lamprecht , Proxmox Backup Server development discussion References: <20230801092954.1686860-1-d.csapak@proxmox.com> <20230801092954.1686860-4-d.csapak@proxmox.com> <6d286eab-604e-408a-b0d1-8ba5990af97f@proxmox.com> From: Dominik Csapak In-Reply-To: <6d286eab-604e-408a-b0d1-8ba5990af97f@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.017 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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: [pbs-devel] [PATCH proxmox-backup 3/3] ui: datastore content: add action to show upload statistics X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Nov 2023 10:05:22 -0000 On 11/27/23 10:28, Thomas Lamprecht wrote: > On 01.08.23 11:29, Dominik Csapak wrote: >> inspired by how we show volume statistics for tape drives >> >> Signed-off-by: Dominik Csapak >> --- >> we could also add it as a tooltip somewhere else, eg the size column >> >> also, this pattern for the window could be refactored into a >> 'keyvalueinfowindow' (or something like that), since we already use that >> pattern a few time >> >> www/datastore/Content.js | 56 ++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 56 insertions(+) >> >> diff --git a/www/datastore/Content.js b/www/datastore/Content.js >> index 9fc07d49..bb2d76ee 100644 >> --- a/www/datastore/Content.js >> +++ b/www/datastore/Content.js >> @@ -711,6 +711,56 @@ Ext.define('PBS.DataStoreContent', { >> }); >> }, >> >> + showUploadStatistics: function(view, rI, cI, item, e, rec) { >> + let me = this; >> + >> + let list = []; >> + >> + let keyMap = { >> + count: gettext('Chunk Count'), >> + duplicates: gettext('Duplicate Chunks'), >> + size: gettext('Size'), >> + compressed_size: gettext('Compressed Size'), >> + }; >> + >> + for (let [key, val] of Object.entries(rec.data['upload-statistic'])) { > > should we explicitly sort those? (e.g., by a predefined weight) yes, that would be better > >> + if (key === 'size' || key === 'compressed_size') { >> + val = Proxmox.Utils.format_size(val); >> + } > > why mix a declarative map and some ad-hoc if checks? > > Rather reuse the map above for a renderer: > > let schema = { > count: { > label: gettext('Chunk Count'), > }, > duplicates: { > label: gettext('Duplicate Chunks'), > }, > size: { > label: gettext('Size'), > renderer: Proxmox.Utils.format_size, > }, > compressed_size: { > label: gettext('Compressed Size'), > renderer: Proxmox.Utils.format_size, > }, > }; > yes makes sense >> + >> + list.push({ key: keyMap[key] ?? key, value: val }); >> + } >> + >> + Ext.create('Ext.window.Window', { >> + title: gettext('Upload Statistics'), >> + modal: true, >> + width: 600, >> + height: 250, >> + layout: 'fit', >> + scrollable: true, >> + items: [ >> + { >> + xtype: 'grid', >> + store: { >> + data: list, >> + }, >> + columns: [ >> + { >> + text: gettext('Property'), >> + dataIndex: 'key', >> + flex: 1, >> + }, >> + { >> + text: gettext('Value'), >> + dataIndex: 'value', >> + flex: 1, >> + }, > > I'd not bother showing that header, basically just noise, and for > such small row count even sorting would not make that much sense. > >> + ], >> + }, >> + ], >> + }).show(); >> + }, >> + >> onForget: function(table, rI, cI, item, e, { data }) { >> let me = this; >> let view = this.getView(); >> @@ -974,6 +1024,12 @@ Ext.define('PBS.DataStoreContent', { >> }, >> isActionDisabled: (v, r, c, i, rec) => rec.data.ty !== 'dir', >> }, >> + { >> + handler: 'showUploadStatistics', >> + getTip: (v, m, rec) => Ext.String.format(gettext("Show Upload Statistics of '{0}'"), v), > > for tooltips it probably makes more sense to use sentence case. > >> + getClass: (v, m, rec) => rec.data.ty === 'dir' ? 'fa fa-info-circle' : 'pmx-hidden', > > info-circle is not a good icon for some specific stats, i.e., not a > general info about the backup snapshot.. A stop-watch could be nice, > but there doesn't seem to be any, so possibly "fa-file-text-o" (a > sheet of stat lines, so to say), not ideal too but IMO better than > the info-circle. > > ps. maybe injecting some more general info like duration could be > nice (didn't check if we even have that available already here > though). > > That said maybe one could even make this an actual info dialog, > with the stats only be one part of that, then the info-circle > could be OK too and we'd not add a core UI element for a rather > niche information that most won't look at often. here we basically have only the info we have in the grid already, but we could provide it in a nicer way maybe: backup time, files (+sizes), last verification info (+link to task log), etc. or did you mean we add a new api endpoint that returns more info about the snapshot altogether? (which could also make sense) > >> + isActionDisabled: (v, r, c, i, rec) => rec.data.ty !== 'dir', >> + }, >> { >> handler: 'onForget', >> getTip: (v, m, { data }) => { >