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 9F8AB64E58 for ; Fri, 4 Mar 2022 14:29:36 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8D7CB5220 for ; Fri, 4 Mar 2022 14:29:06 +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 id 7B20D5215 for ; Fri, 4 Mar 2022 14:29:05 +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 4DB9946ED5 for ; Fri, 4 Mar 2022 14:29:05 +0100 (CET) Message-ID: <01a4001b-4f8e-1678-d460-b1846373085d@proxmox.com> Date: Fri, 4 Mar 2022 14:29:03 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Content-Language: en-US To: Dominik Csapak , Proxmox VE development discussion References: <20211115150209.717122-1-a.lauterer@proxmox.com> <20211115150209.717122-2-a.lauterer@proxmox.com> From: Aaron Lauterer In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.012 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 v2 manager 1/3] ui: lxc/qemu: add disk reassign 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, 04 Mar 2022 13:29:36 -0000 On 2/21/22 16:44, Dominik Csapak wrote: > sorry for the late review > > some comments inline > > On 11/15/21 16:02, Aaron Lauterer wrote: [...] >> diff --git a/www/manager6/lxc/Resources.js >> b/www/manager6/lxc/Resources.js index 15ee3c67..bec7cf14 100644 --- >> a/www/manager6/lxc/Resources.js +++ >> b/www/manager6/lxc/Resources.js @@ -156,6 +156,11 @@ >> Ext.define('PVE.lxc.RessourceView', { return; } + if >> (rec.data.key.match(/^unused/)) { + Ext.Msg.alert('Error', >> gettext('Not yet supported for unused volumes')); + return; >> + } + > > since we already hide/disable that button accordingly, why have that > error message at all? if the user somehow shows the button via the > browser console, the api will return an error anyway.. > > that way, we'd save a gettext good point > [..] >> @@ -227,12 +250,40 @@ Ext.define('PVE.lxc.RessourceView', { }, }); >> - var move_btn = new Proxmox.button.Button({ + let >> reassign_menuitem = new Ext.menu.Item({ + text: >> gettext('Reassign volume'), + tooltip: gettext('Reassign >> volume to another VM'), + handler: run_reassign, + >> iconCls: 'fa fa-mail-forward', + reference: >> 'reassing_item', + }); + + let move_btn = new >> PVE.button.Split({ text: gettext('Move Volume'), selModel: >> me.selModel, disabled: true, dangerous: true, handler: run_move, + >> menu: { + items: [reassign_menuitem], + }, + }); >> + + // needed until we can move unused volumes to other >> storages + let reassign_btn = new Proxmox.button.Button({ + >> text: gettext('Reassign volume'), + tooltip: >> gettext('Reassign volume to another VM'), + handler: >> run_reassign, + selModel: me.selModel, + hidden: >> true, + listeners: { + render: function(btn) { + >> // hack: avoid layout-flickering due to size change, use max width >> for both + let maxWidth = Math.max(btn.getSize().width, >> move_btn.getSize().width); + move_btn.setSize({ width: >> maxWidth }); + btn.setSize({ width: maxWidth }); > > mhmm... while for remove/detach disks this seems to work ok, for this > the gettext are so different, the reassign button look very weird.. > > maybe putting both in a menu (where the top button itself has no > action) and disable accordingly would make more sense here? > > i.e. Volume/Disk Actions - Resize - Move - relocate > > ? Sounds like a good plan and as quickly discussed off list, will also make it easier to look consistent in different transla tions. > [...] >> diff --git a/www/manager6/qemu/HDReassign.js >> b/www/manager6/qemu/HDReassign.js new file mode 100644 index >> 00000000..e6b59cb4 [..] >> + + reassign_disk: function(values) { > > this should probably be 'reassignDisk' according to our usual naming > scheme thx for catching that > >> + let me = this; + let view = me.getView(); + >> let qemu = view.qemu; + let params = { + vmid: >> view.vmid, + 'target-vmid': values.targetVmid, + }; >> + + params[qemu ? 'disk' : 'volume'] = view.disk; + + >> if (view.qemu) { + params['target-disk'] = >> `${values.controller}${values.deviceid}`; + } else { + >> params['target-volume'] = `${values.mpType}${values.mpId}`; + >> } + + let url = >> `/nodes/${view.nodename}/${view.type}/${view.vmid}/`; + url >> += qemu ? 'move_disk' : 'move_volume'; + + >> Proxmox.Utils.API2Request({ + params: params, + url: >> url, + waitMsgTarget: me.getView(), + method: >> 'POST', + failure: response => Ext.Msg.alert('Error', >> response.htmlStatus), + success: function(response, options) >> { + let upid = response.result.data; + >> view.hide(); + Ext.create('Proxmox.window.TaskProgress', >> { + upid, + autoShow: true, + >> taskDone: success => success ? view.close() : view.show(), > > so if the task is not successful, we show ourselves again (over the > error of the progress window)? what about the task log window (if > opened)? As discussed off list, closing the window always is what we want. In case of errors, we do see the error window or have the task log opened. > >> + }); + }, + }); + }, + + >> validateForm: function(fp, isValid) { + >> this.getView().lookup('submitButton').setDisabled(!isValid); + >> }, + + onReassignClick: function() { + let me = this; + >> let view = me.getView(); + let form = >> view.lookup('moveFormPanel').getForm(); + if >> (form.isValid()) { + >> me.reassign_disk(form.getValues()); + } + }, + + >> onMpTypeChange: function(value) { + >> this.getView().getViewModel().set('mpType', value.getValue()); + >> this.getView().lookup('mpIdSelector').validate(); + }, > > these functions make me question if it wouldn't have been easier to > use an 'edit window' instead? any reason for that? It has been a while since I sent these patches, but I think there were a few reasons to go with a regular window. One of them IIRC was that, similar to the resize and move windows, we also want to show a custom labeled button "Reassign {Volume|Disk}" instead of "OK" or one of the options we have with the edit window. > >> + + onTargetVMChange: function(f, vmid) { + let me = >> this; + let view = me.getView(); + let diskSelector = >> view.lookup('diskSelector'); + if (!vmid) { + >> diskSelector.setVMConfig(null); + me.VMConfig = null; + >> return; + } + + let type = view.type === 'qemu' ? >> 'qemu' : 'lxc'; > > here you use 'view.type' but above you use 'view.qemu' ? i'd prefer > to have a consistent way of checking the type True, I also found another instance and aligned them. [...] >> listeners: { + validitychange: 'validateForm', + }, + >> items: [ + { + xtype: 'displayfield', + >> name: 'sourceDisk', + cbind: { + name: get => >> get('isQemu') ? 'disk' : 'volume', + fieldLabel: get => >> get('isQemu') + ? gettext('Source Disk') + >> : gettext('Source Mount Point'), > > wouldn't 'Source' suffice for both? IMHO the context is enough that > the user knows that it's a disk/mp Good point