[pve-devel] [PATCH pve_flutter_frontend v2] feat: ui: add edit button in guests options page
Shan Shaji
s.shaji at proxmox.com
Wed Sep 3 10:42:44 CEST 2025
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 at 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.
More information about the pve-devel
mailing list