hi,
high level comment:
I don't think it's necessary or wise to refactor upfront. You create new
layouting widgets
that are only used once(PveSettingsSection, PveSettingsItem) and create classes with functionality
that is never uses (the mode in openUrl?)
I think it's best to just use the regular flutter widgets inline and keep
methods
and classes small to do what is actually needed. We can still extend them when
more is required, but there is no advantage from having dead code.
a few comments inline
On 7/3/25 13:19, Shan Shaji wrote:
According to Apple's App Store review guidelines all apps must include a
link to their privacy policy within the App [0]. To fix the issue add a
new list item in the settings screen that will allow users to access the
privacy policy.
[0] - https://developer.apple.com/app-store/review/guidelines/#legal
Signed-off-by: Shan Shaji <s.sh...@proxmox.com>
---
lib/constants/pve_links.dart | 6 +
lib/proxmox_general_settings_form.dart | 32 ++++++
lib/utils/pve_url_launcher_util.dart | 24 ++++
lib/utils/result.dart | 34 ++++++
lib/widgets/pve_settings_item.dart | 61 ++++++++++
lib/widgets/pve_settings_section.dart | 43 +++++++
pubspec.lock | 150 ++++++++++++++++++-------
pubspec.yaml | 1 +
8 files changed, 308 insertions(+), 43 deletions(-)
create mode 100644 lib/constants/pve_links.dart
create mode 100644 lib/utils/pve_url_launcher_util.dart
create mode 100644 lib/utils/result.dart
create mode 100644 lib/widgets/pve_settings_item.dart
create mode 100644 lib/widgets/pve_settings_section.dart
diff --git a/lib/constants/pve_links.dart b/lib/constants/pve_links.dart
new file mode 100644
index 0000000..196b58c
--- /dev/null
+++ b/lib/constants/pve_links.dart
@@ -0,0 +1,6 @@
+class PveLinks {
+ PveLinks._();
+
+ static const String privacyPolicyUrl =
+
'https://pve.proxmox.com/wiki/Proxmox_VE_Mobile_Companion_Data_Protection';
+}
IMHO it does not really make sense to define a class to hold things just for a
single element.
I'd put the link where we actually call it, and only refactor it when it's
necessary,
e.g. when we often use the same link from different places, etc.
diff --git a/lib/proxmox_general_settings_form.dart
b/lib/proxmox_general_settings_form.dart
index cb0afef..b81af3a 100644
--- a/lib/proxmox_general_settings_form.dart
+++ b/lib/proxmox_general_settings_form.dart
@@ -1,5 +1,10 @@
import 'package:flutter/material.dart';
+import 'package:proxmox_login_manager/constants/pve_links.dart';
import 'package:proxmox_login_manager/proxmox_general_settings_model.dart';
+import 'package:proxmox_login_manager/utils/pve_url_launcher_util.dart';
+import 'package:proxmox_login_manager/utils/result.dart';
+import 'package:proxmox_login_manager/widgets/pve_settings_item.dart';
+import 'package:proxmox_login_manager/widgets/pve_settings_section.dart';
class ProxmoxGeneralSettingsForm extends StatefulWidget {
const ProxmoxGeneralSettingsForm({super.key});
@@ -21,6 +26,7 @@ class _ProxmoxGeneralSettingsFormState
@override
Widget build(BuildContext context) {
return Scaffold(
+ backgroundColor: Theme.of(context).colorScheme.surfaceContainer,
this change seems to be unrelated?
appBar: AppBar(
title: const Text('Settings'),
),
@@ -45,6 +51,32 @@ class _ProxmoxGeneralSettingsFormState
ProxmoxGeneralSettingsModel.fromLocalStorage();
});
},
+ ),
+ PveSettingsSection(
+ title: 'LEGAL',
+ children: [
+ PveSettingsItem(
+ titleString: 'Privacy Policy',
+ leadingIcon: Icons.privacy_tip_outlined,
+ trailingIcon: Icons.open_in_new,
+ onTap: () async {
+ final result = await PveUrlLauncherUtil.openUrl(
+ Uri.parse(PveLinks.privacyPolicyUrl),
+ );
+
+ if (result is Error) {
+ if (!context.mounted) return;
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(
+ content: Text(
+ result.error.toString(),
+ ),
+ ),
+ );
+ }
+ },
+ ),
+ ],
here you could simply use the column with 2 listtiles.
)
],
),
diff --git a/lib/utils/pve_url_launcher_util.dart
b/lib/utils/pve_url_launcher_util.dart
new file mode 100644
index 0000000..8820d75
--- /dev/null
+++ b/lib/utils/pve_url_launcher_util.dart
@@ -0,0 +1,24 @@
+import 'package:proxmox_login_manager/utils/result.dart';
+import 'package:url_launcher/url_launcher.dart';
+
+class PveUrlLauncherUtil {
+ PveUrlLauncherUtil._();
+
+ static Future<Result> openUrl(
+ Uri uri, {
+ LaunchMode mode = LaunchMode.inAppBrowserView,
FWICT the mode parameter is not used anywhere, so it's not needed.
Actually we already have a wrapper around url_launcher in pve_flutter_frontend
We don't have a good place to put things that are shared between
the pve_flutter_frontend and login_manager, maybe this is a sign
that we should create such a place.
Otherwise you could simply copy the tryLaunchUrl method here.
+ }) async {
+ const errorMessage = 'Could not launch url';
+ try {
+ if (await canLaunchUrl(uri)) {
+ final result = await launchUrl(uri, mode: mode);
+ if (result) return Result.ok(true);
+ return Result.error(Exception(errorMessage));
+ } else {
+ return Result.error(Exception(errorMessage));
+ }
+ } on Exception catch (e, _) {
+ return Result.error(e);
+ }
+ }
+}
diff --git a/lib/utils/result.dart b/lib/utils/result.dart
new file mode 100644
index 0000000..5454e9e
--- /dev/null
+++ b/lib/utils/result.dart
@@ -0,0 +1,34 @@
+/// Utility class that simplifies handling errors.
+///
+/// Return a [Result] from a function to indicate success or failure.
+///
+/// A [Result] is either an [Ok] with a value of type [T]
+/// or an [Error] with an [Exception].
+///
+/// Use [Result.ok] to create a successful result with a value of type [T].
+/// Use [Result.error] to create an error result with an [Exception].
+sealed class Result<T> {
+ const Result();
+
+ /// Creates an instance of Result containing a value
+ factory Result.ok(T value) => Ok(value);
+
+ /// Create an instance of Result containing an error
+ factory Result.error(Exception error) => Error(error);
+}
+
+/// Subclass of Result for values
+final class Ok<T> extends Result<T> {
+ const Ok(this.value);
+
+ /// Returned value in result
+ final T value;
+}
+
+/// Subclass of Result for errors
+final class Error<T> extends Result<T> {
+ const Error(this.error);
+
+ /// Returned error in result
+ final Exception error;
+}
while I appreciate the sentiment of having a result class akin to what we have
in rust.
creating this class just to use it in one or two places is overkill IMHO
If we want to do something like this, it would be better to introduce this in
it's
own patch and use it everywhere it makes sense (also in pve_flutter_frontent,
which
indicates we probably should invent a package that can be used by both...)
but I think we could just live with usual exception handling too..
diff --git a/lib/widgets/pve_settings_item.dart
b/lib/widgets/pve_settings_item.dart
new file mode 100644
index 0000000..a1648aa
--- /dev/null
+++ b/lib/widgets/pve_settings_item.dart
@@ -0,0 +1,61 @@
+import 'package:flutter/material.dart';
+
+class PveSettingsItem extends StatelessWidget {
+ const PveSettingsItem({
+ super.key,
+ this.titleString,
+ this.leadingIcon,
+ this.trailingIcon,
+ this.leading,
+ this.trailing,
+ this.title,
+ this.onTap,
+ }) : assert(
+ leading != null || leadingIcon != null,
+ "Either leading or leadingIcon must be provided",
+ ),
+ assert(
+ trailing != null || trailingIcon != null,
+ 'Either trailing or trailingIcon must be provided',
+ ),
+ assert(
+ titleString != null || title != null,
+ 'Either titleString or title must be provided',
+ );
+
+ /// The text to display as the title.
+ final String? titleString;
+
+ /// An icon to display before the title.
+ ///
+ /// If [leading] widget is not null, [leadingIcon] is ignored.
+ final IconData? leadingIcon;
+
+ /// An icon to display after the title.
+ ///
+ /// If [trailing] property is not null, [trailingIcon] is ignored.
+ final IconData? trailingIcon;
+
+ /// A widget to display before the title.
+ final Widget? leading;
+
+ /// A widget to display after the title.
+ final Widget? trailing;
+
+ /// A widget to display as the title.
+ final Widget? title;
+
+ /// A callback that is called when the user taps on the ListTile.
+ final VoidCallback? onTap;
+
+ @override
+ Widget build(BuildContext context) {
+ return ListTile(
+ shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(10)),
+ leading: leading ?? Icon(leadingIcon),
+ title: title ?? (titleString != null ? Text(titleString!) : null),
+ trailing: trailing ?? Icon(trailingIcon),
+ onTap: onTap,
+ );
+ }
+}
This widget is only used once, and mostly just passes through options to a
listtile,
so i don't think it's worthy of it's own widget...
diff --git a/lib/widgets/pve_settings_section.dart
b/lib/widgets/pve_settings_section.dart
new file mode 100644
index 0000000..d22c630
--- /dev/null
+++ b/lib/widgets/pve_settings_section.dart
@@ -0,0 +1,43 @@
+import 'package:flutter/material.dart';
+import 'package:proxmox_login_manager/widgets/pve_settings_item.dart';
+
+class PveSettingsSection extends StatelessWidget {
+ const PveSettingsSection({
+ super.key,
+ required this.title,
+ required this.children,
+ });
+
+ /// The title of the section.
+ final String title;
+
+ /// The list of items in the section.
+ final List<PveSettingsItem> children;
+
+ @override
+ Widget build(BuildContext context) {
+ return Column(
+ crossAxisAlignment: CrossAxisAlignment.start,
+ children: [
+ ListTile(
+ title: Text(
+ title,
+ style: const TextStyle(
+ fontWeight: FontWeight.bold,
+ ),
+ ),
+ ),
+ Container(
+ decoration: BoxDecoration(
+ color: Theme.of(context).colorScheme.surface,
+ borderRadius: BorderRadius.circular(10),
+ ),
+ margin: const EdgeInsets.symmetric(horizontal: 16),
+ child: Column(
+ children: children,
+ ),
+ )
+ ],
+ );
+ }
+}
similar here, it's only used once....
diff --git a/pubspec.lock b/pubspec.lock
please don't include unrelated pubspec.lock changes in your patches
index 06b82b6..6163a50 100644
--- a/pubspec.lock
+++ b/pubspec.lock
@@ -29,10 +29,10 @@ packages:
dependency: transitive
description:
name: async
- sha256:
"947bfcf187f74dbc5e146c9eb9c0f10c9f8b30743e341481c1e2ed3ecc18c20c"
+ sha256: d2872f9c19731c2e5f10444b14686eb7cc85c76274bd6c16e1816bff9a3bab63
url: "https://pub.dev";
source: hosted
- version: "2.11.0"
+ version: "2.12.0"
biometric_storage:
dependency: "direct main"
description:
@@ -45,10 +45,10 @@ packages:
dependency: transitive
description:
name: boolean_selector
- sha256:
"6cfb5af12253eaf2b368f07bacc5a80d1301a071c73360d746b7f2e32d762c66"
+ sha256:
"8aab1771e1243a5063b8b0ff68042d67334e3feab9e95b9490f9a6ebf73b42ea"
url: "https://pub.dev";
source: hosted
- version: "2.1.1"
+ version: "2.1.2"
build:
dependency: transitive
description:
@@ -125,10 +125,10 @@ packages:
dependency: transitive
description:
name: characters
- sha256:
"04a925763edad70e8443c99234dc3328f442e811f1d8fd1a72f1c8ad0f69a605"
+ sha256: f71061c654a3380576a52b451dd5532377954cf9dbd272a78fc8479606670803
url: "https://pub.dev";
source: hosted
- version: "1.3.0"
+ version: "1.4.0"
checked_yaml:
dependency: transitive
description:
@@ -141,10 +141,10 @@ packages:
dependency: transitive
description:
name: clock
- sha256: cb6d7f03e1de671e34607e909a7213e31d7752be4fb66a86d29fe1eb14bfb5cf
+ sha256: fddb70d9b5277016c77a80201021d40a2247104d9f4aa7bab7157b7e3f05b84b
url: "https://pub.dev";
source: hosted
- version: "1.1.1"
+ version: "1.1.2"
code_builder:
dependency: transitive
description:
@@ -157,10 +157,10 @@ packages:
dependency: "direct main"
description:
name: collection
- sha256: ee67cb0715911d28db6bf4af1026078bd6f0128b07a5f66fb2ed94ec6783c09a
+ sha256:
"2f5709ae4d3d59dd8f7cd309b4e023046b57d8a6c82130785d2b0e5868084e76"
url: "https://pub.dev";
source: hosted
- version: "1.18.0"
+ version: "1.19.1"
convert:
dependency: transitive
description:
@@ -189,10 +189,10 @@ packages:
dependency: transitive
description:
name: fake_async
- sha256:
"511392330127add0b769b75a987850d136345d9227c6b94c96a04cf4a391bf78"
+ sha256:
"6a95e56b2449df2273fd8c45a662d6947ce1ebb7aafe80e550a3f68297f3cacc"
url: "https://pub.dev";
source: hosted
- version: "1.3.1"
+ version: "1.3.2"
ffi:
dependency: transitive
description:
@@ -316,26 +316,26 @@ packages:
dependency: transitive
description:
name: leak_tracker
- sha256:
"78eb209deea09858f5269f5a5b02be4049535f568c07b275096836f01ea323fa"
+ sha256: c35baad643ba394b40aac41080300150a4f08fd0fd6a10378f8f7c6bc161acec
url: "https://pub.dev";
source: hosted
- version: "10.0.0"
+ version: "10.0.8"
leak_tracker_flutter_testing:
dependency: transitive
description:
name: leak_tracker_flutter_testing
- sha256: b46c5e37c19120a8a01918cfaf293547f47269f7cb4b0058f21531c2465d6ef0
+ sha256: f8b613e7e6a13ec79cfdc0e97638fddb3ab848452eff057653abd3edba760573
url: "https://pub.dev";
source: hosted
- version: "2.0.1"
+ version: "3.0.9"
leak_tracker_testing:
dependency: transitive
description:
name: leak_tracker_testing
- sha256: a597f72a664dbd293f3bfc51f9ba69816f84dcd403cdac7066cb3f6003f3ab47
+ sha256:
"6ba465d5d76e67ddf503e1161d1f4a6bc42306f9d66ca1e8f079a47290fb06d3"
url: "https://pub.dev";
source: hosted
- version: "2.0.1"
+ version: "3.0.1"
lints:
dependency: transitive
description:
@@ -356,26 +356,26 @@ packages:
dependency: transitive
description:
name: matcher
- sha256: d2323aa2060500f906aa31a895b4030b6da3ebdcc5619d14ce1aada65cd161cb
+ sha256: dc58c723c3c24bf8d3e2d3ad3f2f9d7bd9cf43ec6feaa64181775e60190153f2
url: "https://pub.dev";
source: hosted
- version: "0.12.16+1"
+ version: "0.12.17"
material_color_utilities:
dependency: transitive
description:
name: material_color_utilities
- sha256:
"0e0a020085b65b6083975e499759762399b4475f766c21668c4ecca34ea74e5a"
+ sha256: f7142bb1154231d7ea5f96bc7bde4bda2a0945d2806bb11670e30b850d56bdec
url: "https://pub.dev";
source: hosted
- version: "0.8.0"
+ version: "0.11.1"
meta:
dependency: transitive
description:
name: meta
- sha256: d584fa6707a52763a52446f02cc621b077888fb63b93bbcb1143a7be5a0c0c04
+ sha256: e3641ec5d63ebf0d9b41bd43201a66e3fc79a65db5f61fc181f04cd27aab950c
url: "https://pub.dev";
source: hosted
- version: "1.11.0"
+ version: "1.16.0"
mime:
dependency: transitive
description:
@@ -396,10 +396,10 @@ packages:
dependency: transitive
description:
name: path
- sha256:
"087ce49c3f0dc39180befefc60fdb4acd8f8620e5682fe2476afd0b3688bb4af"
+ sha256:
"75cca69d1490965be98c73ceaea117e8a04dd21217b37b292c9ddbec0d955bc5"
url: "https://pub.dev";
source: hosted
- version: "1.9.0"
+ version: "1.9.1"
path_provider_linux:
dependency: transitive
description:
@@ -555,7 +555,7 @@ packages:
dependency: transitive
description: flutter
source: sdk
- version: "0.0.99"
+ version: "0.0.0"
source_gen:
dependency: transitive
description:
@@ -568,26 +568,26 @@ packages:
dependency: transitive
description:
name: source_span
- sha256:
"53e943d4206a5e30df338fd4c6e7a077e02254531b138a15aec3bd143c1a8b3c"
+ sha256:
"254ee5351d6cb365c859e20ee823c3bb479bf4a293c22d17a9f1bf144ce86f7c"
url: "https://pub.dev";
source: hosted
- version: "1.10.0"
+ version: "1.10.1"
stack_trace:
dependency: transitive
description:
name: stack_trace
- sha256:
"73713990125a6d93122541237550ee3352a2d84baad52d375a4cad2eb9b7ce0b"
+ sha256:
"8b27215b45d22309b5cddda1aa2b19bdfec9df0e765f2de506401c071d38d1b1"
url: "https://pub.dev";
source: hosted
- version: "1.11.1"
+ version: "1.12.1"
stream_channel:
dependency: transitive
description:
name: stream_channel
- sha256: ba2aa5d8cc609d96bbb2899c28934f9e1af5cddbd60a827822ea467161eb54e7
+ sha256:
"969e04c80b8bcdf826f8f16579c7b14d780458bd97f56d107d3950fdbeef059d"
url: "https://pub.dev";
source: hosted
- version: "2.1.2"
+ version: "2.1.4"
stream_transform:
dependency: transitive
description:
@@ -600,26 +600,26 @@ packages:
dependency: transitive
description:
name: string_scanner
- sha256:
"556692adab6cfa87322a115640c11f13cb77b3f076ddcc5d6ae3c20242bedcde"
+ sha256:
"921cd31725b72fe181906c6a94d987c78e3b98c2e205b397ea399d4054872b43"
url: "https://pub.dev";
source: hosted
- version: "1.2.0"
+ version: "1.4.1"
term_glyph:
dependency: transitive
description:
name: term_glyph
- sha256: a29248a84fbb7c79282b40b8c72a1209db169a2e0542bce341da992fe1bc7e84
+ sha256:
"7f554798625ea768a7518313e58f83891c7f5024f88e46e7182a4558850a4b8e"
url: "https://pub.dev";
source: hosted
- version: "1.2.1"
+ version: "1.2.2"
test_api:
dependency: transitive
description:
name: test_api
- sha256:
"5c2f730018264d276c20e4f1503fd1308dfbbae39ec8ee63c5236311ac06954b"
+ sha256: fb31f383e2ee25fbbfe06b40fe21e1e458d14080e3c67e7ba0acfde4df4e0bbd
url: "https://pub.dev";
source: hosted
- version: "0.6.1"
+ version: "0.7.4"
timing:
dependency: transitive
description:
@@ -636,6 +636,70 @@ packages:
url: "https://pub.dev";
source: hosted
version: "1.3.2"
+ url_launcher:
+ dependency: "direct main"
+ description:
+ name: url_launcher
+ sha256:
"9d06212b1362abc2f0f0d78e6f09f726608c74e3b9462e8368bb03314aa8d603"
+ url: "https://pub.dev";
+ source: hosted
+ version: "6.3.1"
+ url_launcher_android:
+ dependency: transitive
+ description:
+ name: url_launcher_android
+ sha256:
"8582d7f6fe14d2652b4c45c9b6c14c0b678c2af2d083a11b604caeba51930d79"
+ url: "https://pub.dev";
+ source: hosted
+ version: "6.3.16"
+ url_launcher_ios:
+ dependency: transitive
+ description:
+ name: url_launcher_ios
+ sha256:
"7f2022359d4c099eea7df3fdf739f7d3d3b9faf3166fb1dd390775176e0b76cb"
+ url: "https://pub.dev";
+ source: hosted
+ version: "6.3.3"
+ url_launcher_linux:
+ dependency: transitive
+ description:
+ name: url_launcher_linux
+ sha256:
"4e9ba368772369e3e08f231d2301b4ef72b9ff87c31192ef471b380ef29a4935"
+ url: "https://pub.dev";
+ source: hosted
+ version: "3.2.1"
+ url_launcher_macos:
+ dependency: transitive
+ description:
+ name: url_launcher_macos
+ sha256:
"17ba2000b847f334f16626a574c702b196723af2a289e7a93ffcb79acff855c2"
+ url: "https://pub.dev";
+ source: hosted
+ version: "3.2.2"
+ url_launcher_platform_interface:
+ dependency: transitive
+ description:
+ name: url_launcher_platform_interface
+ sha256:
"552f8a1e663569be95a8190206a38187b531910283c3e982193e4f2733f01029"
+ url: "https://pub.dev";
+ source: hosted
+ version: "2.3.2"
+ url_launcher_web:
+ dependency: transitive
+ description:
+ name: url_launcher_web
+ sha256:
"4bd2b7b4dc4d4d0b94e5babfffbca8eac1a126c7f3d6ecbc1a11013faa3abba2"
+ url: "https://pub.dev";
+ source: hosted
+ version: "2.4.1"
+ url_launcher_windows:
+ dependency: transitive
+ description:
+ name: url_launcher_windows
+ sha256:
"3284b6d2ac454cf34f114e1d3319866fdd1e19cdc329999057e44ffe936cfa77"
+ url: "https://pub.dev";
+ source: hosted
+ version: "3.1.4"
vector_math:
dependency: transitive
description:
@@ -648,10 +712,10 @@ packages:
dependency: transitive
description:
name: vm_service
- sha256: b3d56ff4341b8f182b96aceb2fa20e3dcb336b9f867bc0eafc0de10f1048e957
+ sha256:
"0968250880a6c5fe7edc067ed0a13d4bae1577fe2771dcf3010d52c4a9d3ca14"
url: "https://pub.dev";
source: hosted
- version: "13.0.0"
+ version: "14.3.1"
watcher:
dependency: transitive
description:
@@ -701,5 +765,5 @@ packages:
source: hosted
version: "3.1.2"
sdks:
- dart: ">=3.3.0 <4.0.0"
- flutter: ">=3.19.0"
+ dart: ">=3.7.0-0 <4.0.0"
+ flutter: ">=3.27.0"
diff --git a/pubspec.yaml b/pubspec.yaml
index 99f9870..1e48217 100644
--- a/pubspec.yaml
+++ b/pubspec.yaml
@@ -14,6 +14,7 @@ dependencies:
biometric_storage: ^5.0.0
built_value: ^8.4.1
built_collection: ^5.0.0
+ url_launcher: ^6.3.1
proxmox_dart_api_client:
path: ../proxmox_dart_api_client
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
