Re: [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen

From: Dominik Csapak <d.csapak@proxmox.com>
To: Shan Shaji <s.shaji@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen
Date: Fri, 4 Jul 2025 09:31:43 +0200	[thread overview]
Message-ID: <f375c071-c7e1-4b19-afa2-3e89d53e0643@proxmox.com> (raw)
In-Reply-To: <DB334N2HL4HG.1IRRO4KZAY9JN@noor>

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