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
