From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 71CFB1FF164 for ; Fri, 4 Jul 2025 09:31:09 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7ECC331631; Fri, 4 Jul 2025 09:31:48 +0200 (CEST) Message-ID: Date: Fri, 4 Jul 2025 09:31:43 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Shan Shaji , Proxmox VE development discussion References: <20250703111939.52692-1-s.shaji@proxmox.com> <7de107d7-c2fe-4f22-976d-295cd9263a0c@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL 0.022 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.218 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Subject: Re: [pve-devel] [PATCH proxmox_login_manager] ui: settings: add privacy policy url link in settings screen X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 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