Re: [pve-devel] [PATCH pve_flutter_frontend v2] feat: ui: add edit button in guests options page

["Shan Shaji" <s.shaji@proxmox.com>]

Thanks for the review. Will send an updated patch.  

On Tue Sep 2, 2025 at 1:09 PM CEST, Michael Köppl wrote:
> Gave this a spin in my Android emulator. Works as expected. I also
> checked that there's no state change, etc. when rotating to landscape
> and back and that options are locked again upon re-opening the VM/LXC
> options view. Did not notice anything off.
>
> Consider this
> Tested-by: Michael Köppl <m.koeppl@proxmox.com>
>
>> +class LockLxcOptions extends PveLxcOverviewEvent {
>> +  final bool isLockOptions;
>
> nit: I think isOptionsLocked would work better here as well.

Yeah, makes sense. will update it accordingly. 

>>    @override
>>    Widget build(BuildContext context) {
>> @@ -26,7 +28,11 @@ class PveConfigSwitchListTile extends StatelessWidget {
>>      return SwitchListTile(
>>        title: _getTitle(),
>>        value: pBool ?? value ?? defaultValue!,
>> -      onChanged: pending != null ? null : onChanged,
>> +      onChanged: disable
>> +          ? null
>> +          : pending != null
>> +              ? null
>> +              : onChanged,
>
> Wouldn't
>
>     disable || pending != null ? null : onChanged
>
> work here as well?
>
>>      );
>>    }

Yes, Thank you. Will update it. 
>>  
>> diff --git a/lib/widgets/pve_icon_button_widget.dart b/lib/widgets/pve_icon_button_widget.dart
>> new file mode 100644
>> index 0000000..0d30ce8
>> --- /dev/null
>> +++ b/lib/widgets/pve_icon_button_widget.dart
>> @@ -0,0 +1,43 @@
>> +import 'package:flutter/material.dart';
>> +
>> +class PveIconButton extends StatelessWidget {
>> +  final IconData icon;
>> +  final String label;
>> +  final Color? color;
>> +  final VoidCallback? onPressed;
>> +
>> +  const PveIconButton({
>> +    super.key,
>> +    required this.icon,
>> +    required this.label,
>> +    this.color,
>> +    this.onPressed,
>> +  });
>> +
>> +  const PveIconButton.edit({
>> +    super.key,
>> +    this.color,
>> +    this.onPressed,
>> +  })  : icon = Icons.edit,
>> +        label = 'Edit';
>
> I don't know what @Thomas initially suggested,  but I think what would
> work well here is a "lock", so either a button or a toggle to unlock and
> lock the settings, so you could also revert back to a read-only view
> without closing the view and re-opening it. It's just a suggestion from
> my side though. I think a button would have the advantage that it could
> also either show an unlocked or a locked icon depending on the state,
> which would illustrate to the user what the button does without
> additional text (though a tooltip thingy when long-pressing on it could
> be added). It would work with a toggle as well, but I think icons and
> toggles are not usually used together (at least in Android).

IMHO, I think toggle might not be nice,  i liked your idea on using a lock and
unlock icon with the text. I think it's more user friendly than a toggle
with just icon. It will be like "<icon> (lock | unlock)". I will create 
another patch including that change. 

>> +
>> +  @override
>> +  Widget build(BuildContext context) {
>> +    return TextButton(
>> +      onPressed: onPressed,
>> +      child: Row(
>> +        spacing: 2,
>> +        children: [
>> +          Icon(
>> +            icon,
>> +            color: color,
>> +          ),
>> +          Text(
>> +            label,
>> +            style: TextStyle(color: color),
>> +          ),
>> +        ],
>> +      ),
>> +    );
>> +  }
>> +}
>
> nit: was this changed by the formatter? I see that you added the
> ..events.add, but the formatting change from

This was not an intented change. Thanks for pointing out.

>     () => ...
>
> to
>
>     () {
>         return ...
>     }
>
> seems kind of unrelated.



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel