[pve-devel] applied: [PATCH manager v4 0/2] fix #474: allow transfer from container/vms

[Thomas Lamprecht]

Am 14/08/2023 um 12:42 schrieb Dominik Csapak:
> On 8/14/23 12:36, Wolfgang Bumiller wrote:
>> applied, thanks
>>
>> @Dominik: does extjs have an 'enableFn' for rows in a grid?
>> IMO we should either disable the ones with pools when the transfer
>> checkbox is not checked, or hide them (but when hiding them after
>> already checking them... it's weird)
>> Or disable the 'Add' button if a VM with a pool is checked?
>>
> 
> 'enableFn' is our invention ;) and no that only works for some of our components
> 
> 
> looking just now at the gui patch, i would have approached it a bit differently:
> 
> always enable the 'transfer' property but show a 'warning' box when one is selected
> with an old pool
> 
> since 'Allow Transfer' is rather non-descriptive (and no documentation is included)

+1, FWIW I had no idea what this series is about from just reading the subject,
as "pool" is not mentioned there.

> and it adds needless friction on change
> (i select a vm, click, get an error, have to select the vm again, click transfer, click button..)



We normally use "move" or "migrate", not "transfer", or "reassign" (like for
moving a guest disk to another guest) and it has some merits to not expand the
commonly used (parameter) naming scheme to much, but oh well it's already released
and a naming nit that doesn't matters _that_ much.

But the default isn't declared in the schema, please send a follow up for that.

And I agree with Dominik, UX isn't ideal, a warning that one or more VMID will
be moved out of there old Pool, if any, would be sufficient. Not sure if it'd be
better if that's a per-row hint, shown if the row is ticked (e.g., instead of the
Pool column) or a edit-window wide warning hint that gets made visible if any of
the selected VMIDs is in a Pool already.

FWIW, and not directly related (i.e., can be it's own series), you could also fix
the s/Virtual Machine/Virtual Guest/ wording to avoid the confusion that one also
adds Container over this interface.