[pve-devel] [PATCH manager 2/2] ui: vm/usbedit: refactor usb3 checkbox handling

[Thomas Lamprecht]

On 9/25/19 2:30 PM, Aaron Lauterer wrote:
> USB3 checkbox was always checked if the device / port supports USB3
> even though USB3 was disabled on purpose.
> 
> The behaviour now is that for an existing configuration the checkbox
> reflects what is set. When selecting a device or port that is not the
> configured one the checkbox reflects the USB3 support of the selected
> device / port.

Assuming we keep changing it for the user, I'd just have omited the initial
changing on load, i.e., with a:
                change: function(field, newValue, oldValue) {
+                   if (!newValue || (!this.view.isCreate && !oldValue)) {
+                       return;
+                   }

hunk (the edit window also needs to pass isCreate to this panel for above to
work)

Then we would have at least consistent behaviour for all cases, if a user really
wants to go back to original settings they can always just Reset/close, but
actually I also have another proposal below and further comments inline.

> 
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---
> 
> What started out to fix the problem discussed in the previous patch
> series [0] turned into a refactoring of the controller.
> 
> I hope I got the regexes right.
> 
> Since we do not disable the USB3 checkbox anymore I am not sure why we
> kept the whole if else clause around `savedVal`.
> 
> 
> [0]: https://pve.proxmox.com/pipermail/pve-devel/2019-September/039111.html
> 
>  www/manager6/qemu/USBEdit.js | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/www/manager6/qemu/USBEdit.js b/www/manager6/qemu/USBEdit.js
> index f41c3d21..73df7b30 100644
> --- a/www/manager6/qemu/USBEdit.js
> +++ b/www/manager6/qemu/USBEdit.js
> @@ -17,19 +17,19 @@ Ext.define('PVE.qemu.USBInputPanel', {
>  		change: function(field, newValue, oldValue) {
>  		    var usb3field = this.lookupReference('usb3');
>  		    var usbval = field.getUSBValue();
> -		    var dev_is_usb3 = /usb3=1/.test(usbval);
> -
> -		    if (dev_is_usb3) {
> -			usb3field.savedVal = usb3field.getValue();
> -			usb3field.setValue(true);
> +		    var confid = this.view.confid;
> +		    var vmconfig = this.view.vmconfig[confid];
> +		    var dev_is_usb3 = false;
> +		    var hostregex = /host=([^,]*)/;
> +
> +		    if (/^host/.test(vmconfig) &&
> +			vmconfig.match(hostregex)[1] === usbval.match(hostregex)[1]) {

above probably could be replaced by 
(newValue === field.originalValue)

more expressive about what you even want to check and faster.

> +			dev_is_usb3 = /usb3=1/.test(vmconfig);

I'd rather avoid this, the checks/computations are all already done
by our parent panel in the store load event, so doing them again on
every change just seems weird..

But actually I have an alternative proposal.. Drop this whole controller
completely and set USB3 to true, it normally does no harm to have it this
way, we now do not have the "speed of dev must match the usb3 setting"
restriction anymore and it removes magic toggling which, with your proposed
semantic change may got even a bit weirder for users, if they have a lot of
devices they may not know anymore what the original selected was, so one
device does not auto-changes but all others do.

Discoupling the USB3 from all auto-changes and just letting the user select
the speed is IMO the easiest thing to do, especially as USB3 selection does
not changes USB1 or USB2 device for the VM.


>  		    } else {
> -			if (usb3field.savedVal !== undefined) {
> -			    usb3field.setValue(usb3field.savedVal);
> -			} else {
> -			    usb3field.setValue(usb3field.originalValue);
> -			}
> -			usb3field.setDisabled(false);
> +			dev_is_usb3 = /usb3=1/.test(usbval);
>  		    }
> +
> +		    usb3field.setValue(dev_is_usb3);
>  		}
>  	    }
>  	}
>