high level comment:

please separate a refactoring commit with the actual change

having the sheet factored out is probably a good thing,
but it's much easier to review if it's a separate patch, because one
does not have to track multiple changes at once

also some comments inline:

On 5/20/25 10:05, Shan Shaji wrote:
When the guest status is set to `paused` the guest was not showing in
the resources tab. Also there were no option in the resources filter to
select the `paused` status under the status section.

This commit fixes the issue by adding the `paused` status under the
status section in the resources filter sheet. Also the
`PveMobileResourceFilterSheet` has been splitted into muliple widgets
and moved to it's on file under the widgets folder.

Signed-off-by: Shan Shaji <s.sh...@proxmox.com>
---
  lib/pages/main_layout_slim.dart               | 179 +--------------
  .../pve_mobile_resource_filter_sheet.dart     | 215 ++++++++++++++++++
  2 files changed, 221 insertions(+), 173 deletions(-)
  create mode 100644 lib/widgets/pve_mobile_resource_filter_sheet.dart

diff --git a/lib/pages/main_layout_slim.dart b/lib/pages/main_layout_slim.dart
index ac5a6f9..5f4d34e 100644
--- a/lib/pages/main_layout_slim.dart
+++ b/lib/pages/main_layout_slim.dart
@@ -26,6 +26,7 @@ import 
'package:pve_flutter_frontend/widgets/proxmox_stream_builder_widget.dart'
  import 'package:pve_flutter_frontend/widgets/pve_file_selector_widget.dart';
  import 'package:pve_flutter_frontend/widgets/pve_guest_icon_widget.dart';
  import 
'package:pve_flutter_frontend/widgets/pve_help_icon_button_widget.dart';
+import 
'package:pve_flutter_frontend/widgets/pve_mobile_resource_filter_sheet.dart';
  import 
'package:pve_flutter_frontend/widgets/pve_resource_data_card_widget.dart';
  import 
'package:pve_flutter_frontend/widgets/pve_resource_status_chip_widget.dart';
  import 
'package:pve_flutter_frontend/widgets/pve_subscription_alert_dialog.dart';
@@ -41,6 +42,7 @@ class MainLayoutSlim extends StatefulWidget {
class _MainLayoutSlimState extends State<MainLayoutSlim> {
    BehaviorSubject<int> pageSelector = BehaviorSubject.seeded(0);
+

as with the other patch, this commits adds some unrelated new lines...

    @override
    Widget build(BuildContext context) {
      final apiClient = Provider.of<ProxmoxApiClient>(context);
@@ -562,6 +564,7 @@ class PveNodeListTile extends StatelessWidget {
    final String type;
    final String? level;
    final String? ip;
+

also here

    const PveNodeListTile(
        {super.key,
        required this.name,
@@ -569,6 +572,7 @@ class PveNodeListTile extends StatelessWidget {
        required this.type,
        this.level,
        this.ip = ''});
+

and here

    @override
    Widget build(BuildContext context) {
      return ListTile(
@@ -606,7 +610,7 @@ class MobileResourceOverview extends StatelessWidget {
            },
            child: ColoredSafeArea(
                child: Scaffold(
-            endDrawer: _MobileResourceFilterSheet(),
+            endDrawer: PveMobileResourceFilterSheet(),
              appBar: AppBar(
                automaticallyImplyLeading: false,
                elevation: 0,
@@ -782,6 +786,7 @@ class AppbarSearchTextField extends StatefulWidget {
    final ValueChanged<String>? onChanged;
const AppbarSearchTextField({super.key, this.onChanged});
+

and here

    @override
    _AppbarSearchTextFieldState createState() => _AppbarSearchTextFieldState();
  }
@@ -844,178 +849,6 @@ class _AppbarSearchTextFieldState extends 
State<AppbarSearchTextField> {
    }
  }
-class _MobileResourceFilterSheet extends StatelessWidget {
-  @override
-  Widget build(BuildContext context) {
-    final rBloc = Provider.of<PveResourceBloc>(context);
-
-    return ProxmoxStreamBuilder<PveResourceBloc, PveResourceState>(
-      bloc: rBloc,
-      builder: (context, state) => Drawer(
-        child: SingleChildScrollView(
-          child: Column(
-            crossAxisAlignment: CrossAxisAlignment.start,
-            children: <Widget>[
-              Padding(
-                padding: const EdgeInsets.fromLTRB(8.0, 20.0, 8.0, 0),
-                child: ListTile(
-                  title: const Text(
-                    'Filter Results',
-                  ),
-                  trailing: rBloc.isFiltered
-                      ? TextButton(
-                          onPressed: () => rBloc.events.add(ResetFilter()),
-                          child: Text(
-                            'Reset',
-                            style: TextStyle(
-                              color: Theme.of(context).colorScheme.secondary,
-                            ),
-                          ),
-                        )
-                      : null,
-                ),
-              ),
-              const Divider(
-                indent: 0,
-                endIndent: 0,
-              ),
-              Padding(
-                padding: const EdgeInsets.fromLTRB(8.0, 0, 8.0, 0),
-                child: Column(
-                  children: [
-                    const ListTile(
-                      title: Text(
-                        'Type',
-                        style: TextStyle(fontWeight: FontWeight.bold),
-                      ),
-                    ),
-                    CheckboxListTile(
-                      dense: true,
-                      title: Text(
-                        'Nodes',
-                        style: TextStyle(
-                            color: Theme.of(context)
-                                .colorScheme
-                                .onSurface
-                                .withValues(alpha: 0.75)),
-                      ),
-                      value: state.typeFilter.contains('node'),
-                      onChanged: (v) => rBloc.events.add(FilterResources(
-                        typeFilter: addOrRemove(v!, 'node', state.typeFilter),
-                      )),
-                    ),
-                    CheckboxListTile(
-                      dense: true,
-                      title: Text(
-                        'Qemu',
-                        style: TextStyle(
-                            color: Theme.of(context)
-                                .colorScheme
-                                .onSurface
-                                .withValues(alpha: 0.75)),
-                      ),
-                      value: state.typeFilter.contains('qemu'),
-                      onChanged: (v) => rBloc.events.add(FilterResources(
-                        typeFilter: addOrRemove(v!, 'qemu', state.typeFilter),
-                      )),
-                    ),
-                    CheckboxListTile(
-                      dense: true,
-                      title: Text(
-                        'LXC',
-                        style: TextStyle(
-                            color: Theme.of(context)
-                                .colorScheme
-                                .onSurface
-                                .withValues(alpha: 0.75)),
-                      ),
-                      value: state.typeFilter.contains('lxc'),
-                      onChanged: (v) => rBloc.events.add(FilterResources(
-                        typeFilter: addOrRemove(v!, 'lxc', state.typeFilter),
-                      )),
-                    ),
-                    CheckboxListTile(
-                      dense: true,
-                      title: Text(
-                        'Storage',
-                        style: TextStyle(
-                            color: Theme.of(context)
-                                .colorScheme
-                                .onSurface
-                                .withValues(alpha: 0.75)),
-                      ),
-                      value: state.typeFilter.contains('storage'),
-                      onChanged: (v) => rBloc.events.add(FilterResources(
-                        typeFilter:
-                            addOrRemove(v!, 'storage', state.typeFilter),
-                      )),
-                    ),
-                  ],
-                ),
-              ),
-              Padding(
-                padding: const EdgeInsets.fromLTRB(8.0, 0, 8.0, 0),
-                child: Column(
-                  children: [
-                    const ListTile(
-                      title: Text(
-                        'Status',
-                        style: TextStyle(fontWeight: FontWeight.bold),
-                      ),
-                    ),
-                    CheckboxListTile(
-                      dense: true,
-                      title: Text(
-                        'Online',
-                        style: TextStyle(
-                            color: Theme.of(context)
-                                .colorScheme
-                                .onSurface
-                                .withValues(alpha: 0.75)),
-                      ),
-                      value: state.statusFilter
-                          .contains(PveResourceStatusType.running),
-                      onChanged: (v) => rBloc.events.add(FilterResources(
-                        statusFilter: addOrRemove(v!,
-                            PveResourceStatusType.running, state.statusFilter),
-                      )),
-                    ),
-                    CheckboxListTile(
-                      dense: true,
-                      title: Text(
-                        'Offline',
-                        style: TextStyle(
-                            color: Theme.of(context)
-                                .colorScheme
-                                .onSurface
-                                .withValues(alpha: 0.75)),
-                      ),
-                      value: state.statusFilter
-                          .contains(PveResourceStatusType.stopped),
-                      onChanged: (v) => rBloc.events.add(FilterResources(
-                        statusFilter: addOrRemove(v!,
-                            PveResourceStatusType.stopped, state.statusFilter),
-                      )),
-                    ),
-                  ],
-                ),
-              )
-            ],
-          ),
-        ),
-      ),
-    );
-  }
-
-  BuiltSet<S> addOrRemove<S>(bool value, S element, BuiltSet<S> filter) {
-    if (value) {
-      return filter.rebuild((b) => b..add(element));
-    } else {
-      return filter.rebuild((b) => b..remove(element));
-    }
-  }
-}
-
  class AppBarFilterIconButton extends StatelessWidget {
    const AppBarFilterIconButton({super.key});
diff --git a/lib/widgets/pve_mobile_resource_filter_sheet.dart b/lib/widgets/pve_mobile_resource_filter_sheet.dart
new file mode 100644
index 0000000..15b70f1
--- /dev/null
+++ b/lib/widgets/pve_mobile_resource_filter_sheet.dart
@@ -0,0 +1,215 @@
+import 'package:built_collection/built_collection.dart';
+import 'package:flutter/material.dart';
+import 'package:provider/provider.dart';
+import 'package:proxmox_dart_api_client/proxmox_dart_api_client.dart';
+import 'package:pve_flutter_frontend/bloc/pve_resource_bloc.dart';
+import 'package:pve_flutter_frontend/states/pve_resource_state.dart';
+import 
'package:pve_flutter_frontend/widgets/proxmox_stream_builder_widget.dart';
+
+class PveMobileResourceFilterSheet extends StatelessWidget {
+  const PveMobileResourceFilterSheet({super.key});
+
+  @override
+  Widget build(BuildContext context) {
+    final rBloc = Provider.of<PveResourceBloc>(context);
+    return ProxmoxStreamBuilder<PveResourceBloc, PveResourceState>(
+      bloc: rBloc,
+      builder: (context, state) => Drawer(
+        child: SingleChildScrollView(
+          child: Column(
+            crossAxisAlignment: CrossAxisAlignment.start,
+            children: [
+              Padding(
+                padding: const EdgeInsets.fromLTRB(8.0, 20.0, 8.0, 0),
+                child: ListTile(
+                  title: const Text(
+                    'Filter Results',
+                  ),
+                  trailing: rBloc.isFiltered
+                      ? TextButton(
+                          onPressed: () => rBloc.events.add(ResetFilter()),
+                          child: Text(
+                            'Reset',
+                            style: TextStyle(
+                              color: Theme.of(context).colorScheme.secondary,
+                            ),
+                          ),
+                        )
+                      : null,
+                ),
+              ),
+              const Divider(indent: 0, endIndent: 0),
+              _PveFilterSheetSection(
+                sectionTitle: 'Type',
+                items: [
+                  _ProxmoxResourceFilterListTile(
+                    title: 'Nodes',
+                    value: state.typeFilter.contains('node'),
+                    onChanged: (v) => rBloc.events.add(
+                      FilterResources(
+                        typeFilter: _addOrRemove(v!, 'node', state.typeFilter),
+                      ),
+                    ),
+                  ),
+                  _ProxmoxResourceFilterListTile(
+                    title: 'Qemu',
+                    value: state.typeFilter.contains('qemu'),
+                    onChanged: (v) => rBloc.events.add(
+                      FilterResources(
+                        typeFilter: _addOrRemove(v!, 'qemu', state.typeFilter),
+                      ),
+                    ),
+                  ),
+                  _ProxmoxResourceFilterListTile(
+                    title: 'LXC',
+                    value: state.typeFilter.contains('lxc'),
+                    onChanged: (v) => rBloc.events.add(
+                      FilterResources(
+                        typeFilter: _addOrRemove(v!, 'lxc', state.typeFilter),
+                      ),
+                    ),
+                  ),
+                  _ProxmoxResourceFilterListTile(
+                    title: 'Storage',
+                    value: state.typeFilter.contains('storage'),
+                    onChanged: (v) => rBloc.events.add(
+                      FilterResources(
+                        typeFilter:
+                            _addOrRemove(v!, 'storage', state.typeFilter),
+                      ),
+                    ),
+                  ),
+                ],
+              ),
+              _PveFilterSheetSection(
+                sectionTitle: 'Status',
+                items: [
+                  _ProxmoxResourceFilterListTile(
+                    title: 'Online',
+                    value: state.statusFilter
+                        .contains(PveResourceStatusType.running),
+                    onChanged: (v) => rBloc.events.add(
+                      FilterResources(
+                        statusFilter: _addOrRemove(
+                          v!,
+                          PveResourceStatusType.running,
+                          state.statusFilter,
+                        ),
+                      ),
+                    ),
+                  ),
+                  _ProxmoxResourceFilterListTile(
+                    title: 'Offline',
+                    value: state.statusFilter
+                        .contains(PveResourceStatusType.stopped),
+                    onChanged: (v) => rBloc.events.add(
+                      FilterResources(
+                        statusFilter: _addOrRemove(
+                          v!,
+                          PveResourceStatusType.stopped,
+                          state.statusFilter,
+                        ),
+                      ),
+                    ),
+                  ),
+                  _ProxmoxResourceFilterListTile(
+                    title: 'Paused',
+                    value: state.statusFilter
+                        .contains(PveResourceStatusType.paused),
+                    onChanged: (v) => rBloc.events.add(
+                      FilterResources(
+                        statusFilter: _addOrRemove(
+                          v!,
+                          PveResourceStatusType.paused,
+                          state.statusFilter,
+                        ),
+                      ),
+                    ),
+                  ),
+                ],
+              )
+            ],
+          ),
+        ),
+      ),
+    );
+  }
+
+  BuiltSet<S> _addOrRemove<S>(bool value, S element, BuiltSet<S> filter) {
+    if (value) {
+      return filter.rebuild((b) => b..add(element));
+    } else {
+      return filter.rebuild((b) => b..remove(element));
+    }
+  }
+}
+
+class _PveFilterSheetSection extends StatelessWidget {
+  const _PveFilterSheetSection({
+    required this.items,
+    required this.sectionTitle,
+  });
+
+  final List<Widget> items;
+  final String sectionTitle;
+
+  @override
+  Widget build(BuildContext context) {
+    return Padding(
+      padding: const EdgeInsets.symmetric(horizontal: 8),
+      child: Column(
+        children: [
+          _ProxmoxFilterSheetHeader(title: sectionTitle),
+          ...items,
+        ],
+      ),
+    );
+  }
+}
+
+class _ProxmoxFilterSheetHeader extends StatelessWidget {
+  const _ProxmoxFilterSheetHeader({
+    required this.title,
+  });
+
+  final String title;
+
+  @override
+  Widget build(BuildContext context) {
+    return ListTile(
+      title: Text(
+        title,
+        style: TextStyle(fontWeight: FontWeight.bold),
+      ),
+    );
+  }
+}

since you only use the _ProxmoxFilterSheetHeader once, you could also
inline this in the _PveFilterSheetSection. That would save more
code than having this as a separate widget.

+
+class _ProxmoxResourceFilterListTile extends StatelessWidget {
+  const _ProxmoxResourceFilterListTile({
+    required this.title,
+    this.onChanged,
+    this.value,
+  });
+
+  final String title;
+  final ValueChanged<bool?>? onChanged;
+  final bool? value;
+
+  @override
+  Widget build(BuildContext context) {
+    return CheckboxListTile(
+      dense: true,
+      title: Text(
+        title,
+        style: TextStyle(
+          color: Theme.of(context).colorScheme.onSurface.withValues(
+                alpha: 0.75,
+              ),
+        ),
+      ),
+      value: value,
+      onChanged: onChanged,
+    );
+  }
+}



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

Reply via email to