[pve-devel] [RFC manager 2/2] ui: dc: backup: Add detail dialog, disks included
Thomas Lamprecht
t.lamprecht at proxmox.com
Fri Dec 20 09:52:57 CET 2019
Am 12/12/19 um 11:27 AM schrieb Aaron Lauterer:
> Adds a 'Detail' button which opens a new dialog to show which disks
> and mountpoints will be included the selected backup job.
>
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---
>
> For this RFC the new detail dialog is a bit bare and only shows the
> treepanel for the included status.
>
> It probably would be nice to have a short summary above the tree panel
> that shows the other details of the job as well.
Yes, the next scheduled run and target storage would be nice to see, maybe
even the last really scheduled run (would need some more backend changes to
find that info (maybe from task logs)) could be nice to have.
> If we do not want that
> we should find a better name instead of `Detail` to indicate that it
> will only show what is included in the backup.
>
> www/manager6/Utils.js | 23 +++++++
> www/manager6/dc/Backup.js | 124 +++++++++++++++++++++++++++++++++++++-
> 2 files changed, 146 insertions(+), 1 deletion(-)
>
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index e61e2693..81127013 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -207,6 +207,29 @@ Ext.define('PVE.Utils', { utilities: {
>
> },
>
> + render_disk_backup_status: function(value) {
> + if (typeof value == 'undefined') {
> + return "";
> + }
> +
> + let iconCls = 'check-circle good';
> + let text = gettext('Yes');
> +
> + switch (value) {
> + case 'false':
> + iconCls = 'times-circle critical';
> + text = gettext('No');
> + break;
> + case 'not possible':
> + iconCls = 'minus-circle warning';
> + text = gettext('Not possible');
> + break;
> + default: //unknown
> + }
> +
> + return '<i class="fa fa-' + iconCls + '"></i> ' + text;
With 6.0 we can use ES6 as we dropped support for the IE11 browser. There
you can use template literals for above to reduce some syntax noise:
return `<i class="fa fa-${iconCls}"></i> ${text}`
but not too hard feeling here :) There are a few other places here
which could make use of this.
[0]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals
> + },
> +
> get_kvm_osinfo: function(value) {
> var info = { base: 'Other' }; // default
> if (value) {
> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
> index 28275f01..f3f5faa1 100644
> --- a/www/manager6/dc/Backup.js
> +++ b/www/manager6/dc/Backup.js
> @@ -377,6 +377,88 @@ Ext.define('PVE.dc.BackupEdit', {
> });
>
>
> +Ext.define('PVE.dc.BackupDiskTree', {
> + extend: 'Ext.tree.Panel',
> + xtype: 'pveBackDiskTree',
this xtype name is really not telling.. it should always follow the class name,
so why dropping the "up" from Backup? We normally also use alias and do not set
the xtype directly, maybe:
alias: 'widget.pveBackupDetailTree'
add empty line separation between the class metainformation definitions (above)
and it's config (below).
> + stateful: true,
> + stateId: 'grid-node-backupinfo',
for what do you use the stateful here? IMO it's not a good idea to save the
sate of each backup details window in the state managers backing storage, lots
of data for no reak gain (especially if you add a collapse/expand all tool,
like suggested bellow)
> + folderSort: true,
> + rootVisible: false,
> +
> + columns: [
> + {
> + xtype: 'treecolumn',
> + text: gettext('Guest/Disk/Mount point'),
Maybe simply one of:
* 'Guest Volume'
* 'Guest Image'
> + dataIndex: 'id',
> + flex: 1,
> + },
> + {
> + text: gettext('Guest Type'),
We use just 'Type' in the VMIDSelector used for various places (Bulk action,
DC->Backup Add, HA Add, ..) so I'd like to use that here too, for consistency.
> + dataIndex: 'type',
> + width: 120,
> + },
> + {
> + text: gettext('Included in backups'),
IMO using 'Backup Job' could be enough for a column here, else it somewhat
suggest that there are existing backups of that volume already out there -
why may not be the case.
> + renderer: PVE.Utils.render_disk_backup_status,
> + dataIndex: 'status',
> + width: 150,
I'd flex this to, normally I try to flex all columns and keep only those
fixed which have only a checkbox, or boolean status, as else even translations
can be "to long" - while we do not can cater for all of them, I'd like to have
some good defaults which scale automatically for most.
Here one could _maybe_ (as in I did not tested it, but played possibly to often
with those to have a rough feeling what could be OK ^^) do:
id -> flex: 7
type -> flex: 1
status -> flex: 2
One reason for this is that I would like to have the reason for "not possible"
included in the column (e.g., "Not possible - Bindmount")
> + }
> + ],
> +
> + reload: function() {
> + var me = this;
> + var sm = me.getSelectionModel();
> +
> + Proxmox.Utils.API2Request({
> + url: "/cluster/backup/" + me.jobid + "/included_disks",
> + waitMsgTarget: me,
> + method: 'GET',
> + failure: function(response, opts) {
> + Proxmox.Utils.setErrorMask(me, response.htmlStatus);
> + },
> + success: function(response, oopts) {
> + sm.deselectAll();
> + me.setRootNode(response.result.data);
> + me.expandAll();
speaking of expand, we may want to have a "expand all" "collapse all" button,
see bottom of [1] and [2] followup for an example in our API docs viewer.
[1]: https://git.proxmox.com/?p=pve-docs.git;a=commitdiff;h=ab918a4fc7306d6dbb8746c281c3515259b2ecc6
[2]: https://git.proxmox.com/?p=pve-docs.git;a=commitdiff;h=afbe0fcce51a72a54b5d72e66d1d0f76c0eae61b
> + }
> + });
> + },
> +
> + initComponent: function() {
> + var me = this;
> +
> + if (!me.jobid) {
> + throw "no job id specified";
> + }
> +
> + var sm = Ext.create('Ext.selection.TreeModel', {});
> +
> + Ext.apply(me, {
> + selModel: sm,
> + fields: ['id', 'type',
> + {
> + type: 'string',
> + name: 'iconCls',
> + calculate: function(data) {
> + var txt = 'fa x-fa-tree fa-';
> + if (data.leaf) {
> + return txt + 'hdd-o';
> + } else if (data.type === 'VM') {
> + return txt + 'desktop';
> + } else if (data.type === 'CT') {
> + return txt + 'cube';
> + }
could belong in PVE.Utils with type as parameter and using an arrow function
here:
calculate: (data) => 'fa x-fa-tree ' + PVE.Utils.guest_icon_cls(data.leaf),
Doesn't makes it really that better but I could immagine that we have this
somewhere else too? If not, or to different to unify, you can ignore this.
I have rather a copy of similar code than a unified refactor method with
lots of edge case checks.
> + }
> + }
> + ],
> + });
> +
> + me.callParent();
> +
> + me.reload();
> + }
> +});
> +
> Ext.define('PVE.dc.BackupView', {
> extend: 'Ext.grid.GridPanel',
>
> @@ -417,6 +499,37 @@ Ext.define('PVE.dc.BackupView', {
> win.show();
> };
>
> + var run_detail = function() {
> + let rec = sm.getSelection()[0]
> + if (!rec) {
> + return;
> + }
> + var me = this;
> + var disktree = Ext.create('PVE.dc.BackupDiskTree', {
> + title: gettext('Included disks'),
> + flex: 1,
> + jobid: rec.data.id,
> + });
> +
> + var win = Ext.create('Ext.window.Window', {
> + modal: true,
> + width: 800,
> + height: 500,
> + resizable: true,
> + layout: 'fit',
> + title: gettext('Backup Info'),
> + items:[{
> + xtype: 'panel',
> + region: 'center',
> + layout: {
> + type: 'vbox',
> + align: 'stretch'
missing trailing comma
> + },
> + items: [disktree],
> + }]
> + }).show();
> + };
> +
> var run_backup_now = function(job) {
> job = Ext.clone(job);
>
> @@ -517,6 +630,13 @@ Ext.define('PVE.dc.BackupView', {
> }
> });
>
> + var detail_btn = new Proxmox.button.Button({
> + text: gettext('Detail'),
'Show Details' and/or add tooltip (search hint: qtip) with a longer message, e.g.:
'Shows which Guests and Volumes are affected by a backup job.'
(roughly)
> + disabled: true,
> + selModel: sm,
> + handler: run_detail
missing trailing comma
> + });
> +
> Proxmox.Utils.monStoreErrors(me, store);
>
> Ext.apply(me, {
> @@ -540,7 +660,9 @@ Ext.define('PVE.dc.BackupView', {
> remove_btn,
> edit_btn,
> '-',
> - run_btn
> + run_btn,
> + '-',
> + detail_btn
add trailing comma, we do not have the restriction of that anymore since 6.0 and
it's much nicer to have, as changes do not need to affect the previous line.
> ],
> columns: [
> {
>
More information about the pve-devel
mailing list