* [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen
@ 2025-07-03 11:19 Shan Shaji
2025-07-03 14:00 ` Dominik Csapak
0 siblings, 1 reply; 9+ messages in thread
From: Shan Shaji @ 2025-07-03 11:19 UTC (permalink / raw)
To: pve-devel
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.shaji@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';
+}
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,
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(),
+ ),
+ ),
+ );
+ }
+ },
+ ),
+ ],
)
],
),
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,
+ }) 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;
+}
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,
+ );
+ }
+}
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,
+ ),
+ )
+ ],
+ );
+ }
+}
diff --git a/pubspec.lock b/pubspec.lock
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
--
2.39.5 (Apple Git-154)
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen
2025-07-03 11:19 [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen Shan Shaji
@ 2025-07-03 14:00 ` Dominik Csapak
2025-07-04 7:16 ` Shan Shaji
2025-07-04 14:40 ` Shan Shaji
0 siblings, 2 replies; 9+ messages in thread
From: Dominik Csapak @ 2025-07-03 14:00 UTC (permalink / raw)
To: Proxmox VE development discussion, Shan Shaji
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.shaji@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen
2025-07-03 14:00 ` Dominik Csapak
@ 2025-07-04 7:16 ` Shan Shaji
2025-07-04 7:31 ` Dominik Csapak
2025-07-04 14:40 ` Shan Shaji
1 sibling, 1 reply; 9+ messages in thread
From: Shan Shaji @ 2025-07-04 7:16 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
Thanks for the review. I will update the changes according to the
reviews. I have some doubts that i have added as inline comments.
On Thu Jul 3, 2025 at 4:00 PM CEST, Dominik Csapak wrote:
> 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.shaji@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.
Rather than defining the class to hold the constant string. Would like
to know your opinion about just defining the `privacyPolicyUrl` as variable in the
file? Usualy we do seperate constants like label strings, links,
spacings etc.. to its own file as it can be re-used and new values can
be directly added to those files.
> > 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.
That makes sense. We will need a seperate package that shares
common things between the login manager and the pve front end.
This is just my doubt. Can i ask why we seperated the
proxmox_login_manager as a seperate package?
> > + }) 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..
Makes sense. Thank you
> > 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
The .lock file is not up to date with our new flutter version we are
using. I will create a seperate patch for this.
> > 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen
2025-07-04 7:16 ` Shan Shaji
@ 2025-07-04 7:31 ` Dominik Csapak
2025-07-04 9:34 ` Thomas Lamprecht
0 siblings, 1 reply; 9+ messages in thread
From: Dominik Csapak @ 2025-07-04 7:31 UTC (permalink / raw)
To: Shan Shaji, Proxmox VE development discussion
On 7/4/25 09:16, Shan Shaji wrote:
> Thanks for the review. I will update the changes according to the
> reviews. I have some doubts that i have added as inline comments.
>
[snip]
>>> 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.
>
> Rather than defining the class to hold the constant string. Would like
> to know your opinion about just defining the `privacyPolicyUrl` as variable in the
> file? Usualy we do seperate constants like label strings, links,
> spacings etc.. to its own file as it can be re-used and new values can
> be directly added to those files.
>
yes that sounds like a good middle ground..
[snip]
>>> 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.
>
> That makes sense. We will need a seperate package that shares
> common things between the login manager and the pve front end.
>
> This is just my doubt. Can i ask why we seperated the
> proxmox_login_manager as a seperate package?
I'm not sure why we did this, but my guess would be that it was planned to maybe have
multiple apps in the future?
@Tim i think you were intially the one doing that, do you remember why we did the split?
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen
2025-07-04 7:31 ` Dominik Csapak
@ 2025-07-04 9:34 ` Thomas Lamprecht
2025-07-04 12:11 ` Shan Shaji
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Lamprecht @ 2025-07-04 9:34 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak, Shan Shaji
Am 04.07.25 um 09:31 schrieb Dominik Csapak:
>>
>> This is just my doubt. Can i ask why we seperated the
>> proxmox_login_manager as a seperate package?
> I'm not sure why we did this, but my guess would be that it was planned to maybe have
> multiple apps in the future?
>
> @Tim i think you were intially the one doing that, do you remember why we did the split?
>
For having a shared basis for other products, there is a prototype for
PBS on the staff repos IIRC. While we do not plan to develop that further
at the moment, having to split it out again in the future is definitively
quite a bit of extra work, while keeping it split shouldn't be so hard
I think. So, while not a must, I'd I'd keep it split for the time being,
if there are no bigger reasons speaking against that.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen
2025-07-04 9:34 ` Thomas Lamprecht
@ 2025-07-04 12:11 ` Shan Shaji
0 siblings, 0 replies; 9+ messages in thread
From: Shan Shaji @ 2025-07-04 12:11 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion, Dominik Csapak
Got it. Thanks for sharing.
On Fri Jul 4, 2025 at 11:34 AM CEST, Thomas Lamprecht wrote:
> Am 04.07.25 um 09:31 schrieb Dominik Csapak:
> >>
> >> This is just my doubt. Can i ask why we seperated the
> >> proxmox_login_manager as a seperate package?
> > I'm not sure why we did this, but my guess would be that it was planned to maybe have
> > multiple apps in the future?
> >
> > @Tim i think you were intially the one doing that, do you remember why we did the split?
> >
>
> For having a shared basis for other products, there is a prototype for
> PBS on the staff repos IIRC. While we do not plan to develop that further
> at the moment, having to split it out again in the future is definitively
> quite a bit of extra work, while keeping it split shouldn't be so hard
> I think. So, while not a must, I'd I'd keep it split for the time being,
> if there are no bigger reasons speaking against that.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen
2025-07-03 14:00 ` Dominik Csapak
2025-07-04 7:16 ` Shan Shaji
@ 2025-07-04 14:40 ` Shan Shaji
2025-07-07 6:28 ` Dominik Csapak
1 sibling, 1 reply; 9+ messages in thread
From: Shan Shaji @ 2025-07-04 14:40 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
On Thu Jul 3, 2025 at 4:00 PM CEST, Dominik Csapak wrote:
> > + backgroundColor: Theme.of(context).colorScheme.surfaceContainer,
>
> this change seems to be unrelated?
This was intended. The change was made to match the backgroundColor of
`ProxmoxLoginSelector`. Previously it was using the default scaffold
backgroundColor.
>
> > 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,
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen
2025-07-04 14:40 ` Shan Shaji
@ 2025-07-07 6:28 ` Dominik Csapak
2025-07-07 7:09 ` Shan Shaji
0 siblings, 1 reply; 9+ messages in thread
From: Dominik Csapak @ 2025-07-07 6:28 UTC (permalink / raw)
To: Shan Shaji, Proxmox VE development discussion
On 7/4/25 16:40, Shan Shaji wrote:
> On Thu Jul 3, 2025 at 4:00 PM CEST, Dominik Csapak wrote:
>>> + backgroundColor: Theme.of(context).colorScheme.surfaceContainer,
>>
>> this change seems to be unrelated?
> This was intended. The change was made to match the backgroundColor of
> `ProxmoxLoginSelector`. Previously it was using the default scaffold
> backgroundColor.
ok. In this case it was not really clear why it was done in this patch, so a short
sentence that this is changed too wouldn't hurt IMHO.
>>
>>> 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,
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen
2025-07-07 6:28 ` Dominik Csapak
@ 2025-07-07 7:09 ` Shan Shaji
0 siblings, 0 replies; 9+ messages in thread
From: Shan Shaji @ 2025-07-07 7:09 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
Thank you Dominik, Will keep that in mind. I have created a new patch
by including the changes that you mentioned.
superseeded by v2: https://lore.proxmox.com/pve-devel/20250704152654.21221-1-s.shaji@proxmox.com/T/#t
On Mon Jul 7, 2025 at 8:28 AM CEST, Dominik Csapak wrote:
> On 7/4/25 16:40, Shan Shaji wrote:
> > On Thu Jul 3, 2025 at 4:00 PM CEST, Dominik Csapak wrote:
> >>> + backgroundColor: Theme.of(context).colorScheme.surfaceContainer,
> >>
> >> this change seems to be unrelated?
> > This was intended. The change was made to match the backgroundColor of
> > `ProxmoxLoginSelector`. Previously it was using the default scaffold
> > backgroundColor.
>
> ok. In this case it was not really clear why it was done in this patch, so a short
> sentence that this is changed too wouldn't hurt IMHO.
Also this change is not included in the new patch. I have removed it.
> >>
> >>> 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,
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-07-07 7:08 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-07-03 11:19 [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen Shan Shaji
2025-07-03 14:00 ` Dominik Csapak
2025-07-04 7:16 ` Shan Shaji
2025-07-04 7:31 ` Dominik Csapak
2025-07-04 9:34 ` Thomas Lamprecht
2025-07-04 12:11 ` Shan Shaji
2025-07-04 14:40 ` Shan Shaji
2025-07-07 6:28 ` Dominik Csapak
2025-07-07 7:09 ` Shan Shaji
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox