[pve-devel] [PATCH proxmox_login_manager/pve_flutter_frontend 0/2] refactor: ui: add new settings page in `pve_flutter_frontend`

[pve-devel] [PATCH proxmox_login_manager/pve_flutter_frontend 0/2] refactor: ui: add new settings page in `pve_flutter_frontend`

[Shan Shaji]

Note: This is POC PATCH

Since the primary responsibility of the login manager is to handle 
login-related features, having the settings screen code defined within
the package causes unnecessary coupling. Whenever a new configuration 
or app-related setting  
(e.g., privacy policy URL, help link, app version string) needs to be 
added, the package must be modified—even if the change is unrelated 
to login functionality. When we had only one config (SSL) it was fine. 
Since new widgets needs to be added it might be better to seperate it. 

To avoid this and to make it easier to add global settings 
configuration widgets in the future, the direct navigation to 
`ProxmoxGeneralSettingsForm` from `proxmox_login_manager` has been
removed. Instead, the screen is now defined within the 
`pve_flutter_frontend`, and the `ProxmoxGeneralSettingsForm` is used 
as a widget inside that screen.

pve_flutter_frontend:

Shan Shaji (1):
  refactor: ui: add new settings page and use existing ssl toggle widget

 lib/main.dart                    |  8 ++++++++
 lib/pages/pve_settings_page.dart | 23 +++++++++++++++++++++++
 2 files changed, 31 insertions(+)
 create mode 100644 lib/pages/pve_settings_page.dart


proxmox_login_manager:

Shan Shaji (1):
  refactor: ui: move settings page to `pve_flutter_frontend`

 lib/proxmox_general_settings_form.dart | 58 +++++++++++---------------
 lib/proxmox_login_form.dart            |  5 +--
 lib/proxmox_login_selector.dart        | 13 +++---
 3 files changed, 31 insertions(+), 45 deletions(-)


Summary over all repositories:
  5 files changed, 62 insertions(+), 45 deletions(-)

-- 
Generated by git-murpp 0.8.1


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

[pve-devel] [PATCH pve_flutter_frontend 1/1] refactor: ui: add new settings page and use existing ssl toggle widget

[Shan Shaji]

The settings page UI was previously implemented in
proxmox_login_manager. A new screen has now been created  with an added
route and removed the settings page from `proxmox_login_manager`.
This screen reuses the SSL toggle form from the
`proxmox_login_manager` package.

Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
---
 lib/main.dart                    |  8 ++++++++
 lib/pages/pve_settings_page.dart | 23 +++++++++++++++++++++++
 2 files changed, 31 insertions(+)
 create mode 100644 lib/pages/pve_settings_page.dart

diff --git a/lib/main.dart b/lib/main.dart
index 6a7c5a9..0ffcae7 100644
--- a/lib/main.dart
+++ b/lib/main.dart
@@ -1,6 +1,7 @@
 import 'package:flutter/foundation.dart';
 import 'package:flutter/material.dart';
 import 'package:provider/provider.dart';
+import 'package:pve_flutter_frontend/pages/pve_settings_page.dart';
 import 'package:pve_flutter_frontend/widgets/pve_first_welcome_screen.dart';
 import 'package:shared_preferences/shared_preferences.dart';
 import 'package:proxmox_login_manager/proxmox_login_manager.dart';
@@ -227,6 +228,13 @@ class MyApp extends StatelessWidget {
             );
           }
 
+          if (context.name == PveSettingsPage.routePath) {
+            return MaterialPageRoute(
+              settings: context,
+              builder: (context) => PveSettingsPage(),
+            );
+          }
+
           if (authbloc!.state.value is Unauthenticated ||
               context.name == '/login') {
             return MaterialPageRoute(
diff --git a/lib/pages/pve_settings_page.dart b/lib/pages/pve_settings_page.dart
new file mode 100644
index 0000000..0d405c1
--- /dev/null
+++ b/lib/pages/pve_settings_page.dart
@@ -0,0 +1,23 @@
+import 'package:flutter/material.dart';
+import 'package:proxmox_login_manager/proxmox_general_settings_form.dart';
+import 'package:pve_flutter_frontend/widgets/pve_app_bar.dart';
+
+class PveSettingsPage extends StatelessWidget {
+  static final routePath = '/settings';
+
+  const PveSettingsPage({super.key});
+
+  @override
+  Widget build(BuildContext context) {
+    return Scaffold(
+      appBar: PveAppBar(),
+      body: SingleChildScrollView(
+        child: Column(
+          children: [
+            ProxmoxGeneralSettingsForm(),
+          ],
+        ),
+      ),
+    );
+  }
+}
-- 
2.39.5



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

[pve-devel] [PATCH proxmox_login_manager 1/1] refactor: ui: move settings page to `pve_flutter_frontend`

[Shan Shaji]

The setting screen UI was previously defined in this repo. If a new app
settings needs to be added eg: privacy policy link, app version string.
The change had to be done in the package even though the change is not
package specific.

Inorder to fix that, move settings page to `pve_flutter_frontend` and use
`ProxmoxGeneralSettingsForm` as a widget in the new settings page.

Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
---
 lib/proxmox_general_settings_form.dart | 58 +++++++++++---------------
 lib/proxmox_login_form.dart            |  5 +--
 lib/proxmox_login_selector.dart        | 13 +++---
 3 files changed, 31 insertions(+), 45 deletions(-)

diff --git a/lib/proxmox_general_settings_form.dart b/lib/proxmox_general_settings_form.dart
index cb0afef..e39f91d 100644
--- a/lib/proxmox_general_settings_form.dart
+++ b/lib/proxmox_general_settings_form.dart
@@ -12,6 +12,7 @@ class ProxmoxGeneralSettingsForm extends StatefulWidget {
 class _ProxmoxGeneralSettingsFormState
     extends State<ProxmoxGeneralSettingsForm> {
   Future<ProxmoxGeneralSettingsModel>? _settings;
+
   @override
   void initState() {
     super.initState();
@@ -20,41 +21,30 @@ class _ProxmoxGeneralSettingsFormState
 
   @override
   Widget build(BuildContext context) {
-    return Scaffold(
-      appBar: AppBar(
-        title: const Text('Settings'),
-      ),
-      body: FutureBuilder<ProxmoxGeneralSettingsModel>(
-          future: _settings,
-          builder: (context, snaptshot) {
-            if (snaptshot.hasData) {
-              final settings = snaptshot.data!;
-              return SingleChildScrollView(
-                child: Column(
-                  children: [
-                    SwitchListTile(
-                      title: const Text('Validate SSL connections'),
-                      subtitle: const Text('e.g. validates certificates'),
-                      value: settings.sslValidation!,
-                      onChanged: (value) async {
-                        await settings
-                            .rebuild((b) => b.sslValidation = value)
-                            .toLocalStorage();
-                        setState(() {
-                          _settings =
-                              ProxmoxGeneralSettingsModel.fromLocalStorage();
-                        });
-                      },
-                    )
-                  ],
-                ),
-              );
-            }
+    return FutureBuilder<ProxmoxGeneralSettingsModel>(
+      future: _settings,
+      builder: (context, snapShot) {
+        if (snapShot.hasData) {
+          final settings = snapShot.data!;
+          return SwitchListTile(
+            title: const Text('Validate SSL connections'),
+            subtitle: const Text('e.g. validates certificates'),
+            value: settings.sslValidation!,
+            onChanged: (value) async {
+              await settings
+                  .rebuild((b) => b.sslValidation = value)
+                  .toLocalStorage();
+              setState(() {
+                _settings = ProxmoxGeneralSettingsModel.fromLocalStorage();
+              });
+            },
+          );
+        }
 
-            return const Center(
-              child: CircularProgressIndicator(),
-            );
-          }),
+        return const Center(
+          child: CircularProgressIndicator(),
+        );
+      },
     );
   }
 }
diff --git a/lib/proxmox_login_form.dart b/lib/proxmox_login_form.dart
index 5a2db3b..db9994b 100644
--- a/lib/proxmox_login_form.dart
+++ b/lib/proxmox_login_form.dart
@@ -6,7 +6,6 @@ import 'package:collection/collection.dart';
 import 'package:proxmox_dart_api_client/proxmox_dart_api_client.dart'
     as proxclient;
 import 'package:proxmox_dart_api_client/proxmox_dart_api_client.dart';
-import 'package:proxmox_login_manager/proxmox_general_settings_form.dart';
 import 'package:proxmox_login_manager/proxmox_general_settings_model.dart';
 import 'package:proxmox_login_manager/proxmox_login_model.dart';
 import 'package:proxmox_login_manager/proxmox_tfa_form.dart';
@@ -829,9 +828,7 @@ class ProxmoxCertificateErrorDialog extends StatelessWidget {
           child: const Text('Close'),
         ),
         TextButton(
-          onPressed: () => Navigator.of(context).pushReplacement(
-              MaterialPageRoute(
-                  builder: (context) => const ProxmoxGeneralSettingsForm())),
+          onPressed: () => Navigator.popAndPushNamed(context, '/settings'),
           child: const Text('Settings'),
         )
       ],
diff --git a/lib/proxmox_login_selector.dart b/lib/proxmox_login_selector.dart
index ba84d7d..c8d9813 100644
--- a/lib/proxmox_login_selector.dart
+++ b/lib/proxmox_login_selector.dart
@@ -1,6 +1,5 @@
 import 'package:flutter/material.dart';
 import 'package:built_collection/built_collection.dart';
-import 'package:proxmox_login_manager/proxmox_general_settings_form.dart';
 import 'package:proxmox_login_manager/proxmox_login_form.dart';
 import 'package:proxmox_login_manager/proxmox_login_model.dart';
 import 'package:proxmox_dart_api_client/proxmox_dart_api_client.dart'
@@ -52,12 +51,12 @@ class _ProxmoxLoginSelectorState extends State<ProxmoxLoginSelector> {
         ),
         actions: [
           IconButton(
-              icon: const Icon(Icons.settings),
-              onPressed: () {
-                Navigator.of(context).push(MaterialPageRoute(
-                  builder: (context) => const ProxmoxGeneralSettingsForm(),
-                ));
-              })
+            icon: const Icon(Icons.settings),
+            onPressed: () => Navigator.pushNamed(
+              context,
+              '/settings',
+            ),
+          )
         ],
       ),
       body: FutureBuilder<ProxmoxLoginStorage?>(
-- 
2.39.5



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

Re: [pve-devel] [PATCH proxmox_login_manager/pve_flutter_frontend 0/2] refactor: ui: add new settings page in `pve_flutter_frontend`

[Dominik Csapak]

changes look good to me

something I noticed (that's not your fault at all) which irks me a bit
is that we don't really have any dependency management between

proxmox_login_manager and pve_flutter_frontend

as in, we don't ever bump the version of proxmox_login_manager
(and the dart_api_client of course)

while most of the time it's not a problem i guess (as long
as the current master still builds), when we at one point
want to go back in time (e.g. for bisecting) we don't know anymore
which state the other repositories were (and have to guess)

Would it make sense to you to start using proper versioning for the
lower level repositories/packages? or is it possible
to track git revisions at least in the pubspec.yaml?
(that way we would just have to update the git revision
in pve_flutter_frontend)

if we now update the flutter app more regularly and with more
people, it may make sense to do this properly

I noticed now, because this is technically a breaking change between
the packages, and just applying one patch would break the other

what do you say?
@Thomas, do you have an opinion here?


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

Re: [pve-devel] [PATCH proxmox_login_manager/pve_flutter_frontend 0/2] refactor: ui: add new settings page in `pve_flutter_frontend`

[Shan Shaji]

Hi Dominik, Thank you so much for the review. I believe that 
would be nice. Can we use the git tags and reference
the package from git.proxmox.com? [0] May be we can also add 
a CHANGELOG file which will list all the tags and the changes
made in each tag. 

- [0] https://dart.dev/tools/pub/dependencies#git-packages

On Wed Aug 13, 2025 at 3:56 PM CEST, Dominik Csapak wrote:
> changes look good to me
>
> something I noticed (that's not your fault at all) which irks me a bit
> is that we don't really have any dependency management between
>
> proxmox_login_manager and pve_flutter_frontend
>
> as in, we don't ever bump the version of proxmox_login_manager
> (and the dart_api_client of course)
>
> while most of the time it's not a problem i guess (as long
> as the current master still builds), when we at one point
> want to go back in time (e.g. for bisecting) we don't know anymore
> which state the other repositories were (and have to guess)
>
> Would it make sense to you to start using proper versioning for the
> lower level repositories/packages? or is it possible
> to track git revisions at least in the pubspec.yaml?
> (that way we would just have to update the git revision
> in pve_flutter_frontend)
>
> if we now update the flutter app more regularly and with more
> people, it may make sense to do this properly
>
> I noticed now, because this is technically a breaking change between
> the packages, and just applying one patch would break the other
>
> what do you say?
> @Thomas, do you have an opinion here?



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

Re: [pve-devel] [PATCH proxmox_login_manager/pve_flutter_frontend 0/2] refactor: ui: add new settings page in `pve_flutter_frontend`

[Thomas Lamprecht]

On 13/08/2025 15:56, Dominik Csapak wrote:
> changes look good to me
> 
> something I noticed (that's not your fault at all) which irks me a bit
> is that we don't really have any dependency management between
> 
> proxmox_login_manager and pve_flutter_frontend
> 
> as in, we don't ever bump the version of proxmox_login_manager
> (and the dart_api_client of course)
> 
> while most of the time it's not a problem i guess (as long
> as the current master still builds), when we at one point
> want to go back in time (e.g. for bisecting) we don't know anymore
> which state the other repositories were (and have to guess)
> 
> Would it make sense to you to start using proper versioning for the
> lower level repositories/packages? or is it possible
> to track git revisions at least in the pubspec.yaml?
> (that way we would just have to update the git revision
> in pve_flutter_frontend)

alternative would be using proxmox_login_manager through a git
submodule, which is not _that_ great for developer experience
though...

> if we now update the flutter app more regularly and with more
> people, it may make sense to do this properly
> 
> I noticed now, because this is technically a breaking change between
> the packages, and just applying one patch would break the other
> 
> what do you say?
> @Thomas, do you have an opinion here?

Explicit versioning and optionally a CHANGELOG (as per Shan's
follow-up reply) sound good to me, as it's not directly user facing
I'd keep the changelog rather short/low-overhead in any way.


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

Re: [pve-devel] [PATCH proxmox_login_manager/pve_flutter_frontend 0/2] refactor: ui: add new settings page in `pve_flutter_frontend`

[Shan Shaji]

Superseded by: https://lore.proxmox.com/pve-devel/20250829114846.88507-4-s.shaji@proxmox.com/T/#u


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