From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id CAE701FF15C
for <inbox@lore.proxmox.com>; Fri, 2 May 2025 13:32:13 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id EDE601F531;
Fri, 2 May 2025 13:32:24 +0200 (CEST)
Message-ID: <3615656b-a9c5-430c-b0c2-57c72ac0854b@proxmox.com>
Date: Fri, 2 May 2025 13:32:19 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Alexander Abraham <a.abraham@proxmox.com>
References: <20250429131645.106612-1-a.abraham@proxmox.com>
<20250429131645.106612-3-a.abraham@proxmox.com>
Content-Language: en-US
From: =?UTF-8?Q?Michael_K=C3=B6ppl?= <m.koeppl@proxmox.com>
In-Reply-To: <20250429131645.106612-3-a.abraham@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.002 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.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_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. [colors.green]
Subject: Re: [pve-devel] [PATCH flutter_frontend 1/1] fix #4976: ui: Nodes
offline
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
The commit message is not very descriptive. Also, please run dart format
on these changes. There are some wrong indentations, missing spaces,
trailing spaces, etc. I also added 3 comments inline.
Apart from that, I tested this by manually stopping a node I was
currently looking at in the node overview. The app navigates me back to
the previous screen once it detects that the node is offline and
afterwards stops me from opening that node's overview again while it is
offline. Opening the overview once the node is online again works as
well. Everything seems to work as expected. It's a good change, well done!
On 4/29/25 15:16, Alexander Abraham wrote:
> diff --git a/lib/bloc/pve_node_overview_bloc.dart b/lib/bloc/pve_node_overview_bloc.dart
> index 19d6563..98005fd 100644
> --- a/lib/bloc/pve_node_overview_bloc.dart
> +++ b/lib/bloc/pve_node_overview_bloc.dart
> @@ -1,5 +1,5 @@
> import 'dart:async';
> -
> +import 'package:flutter/material.dart';
> import 'package:proxmox_dart_api_client/proxmox_dart_api_client.dart';
> import 'package:pve_flutter_frontend/bloc/proxmox_base_bloc.dart';
> import 'package:pve_flutter_frontend/states/pve_node_overview_state.dart';
> @@ -8,12 +8,13 @@ class PveNodeOverviewBloc
> extends ProxmoxBaseBloc<PveNodeOverviewEvent, PveNodeOverviewState> {
> final ProxmoxApiClient apiClient;
> final String nodeID;
> + final BuildContext context;
> final PveNodeOverviewState init;
> @override
> PveNodeOverviewState get initialState => init;
>
> PveNodeOverviewBloc(
> - {required this.apiClient, required this.nodeID, required this.init});
> + {required this.apiClient, required this.nodeID, required this.init, required this.context});
>
> Timer? sTimer;
> @override
> @@ -32,36 +33,56 @@ class PveNodeOverviewBloc
> @override
> Stream<PveNodeOverviewState> processEvents(
> PveNodeOverviewEvent event) async* {
> - if (event is UpdateNodeStatus) {
> - final status = await apiClient.getNodeStatus(nodeID);
> - yield latestState.rebuild((b) => b..status.replace(status));
> - final rrdData =
> - await apiClient.getNodeRRDdata(nodeID, PveRRDTimeframeType.hour);
> - yield latestState.rebuild((b) => b..rrdData.replace(rrdData));
> - final services = await apiClient.getNodeServices(nodeID);
> - yield latestState.rebuild((b) => b..services.replace(services));
> - try {
> - final updates = await apiClient.getNodeAptUpdate(nodeID);
> - yield latestState.rebuild((b) => b..updates.replace(updates));
> - yield latestState
> - .rebuild((b) => b..updatesQueryPermissionFailure = false);
> - } on ProxmoxApiException catch (e) {
> - // only throw on non permission related errors
> - if (e.statusCode != 403) {
> - rethrow;
> - } else {
> + PveNodeStatusModel? status;
> + try {
> + status = await apiClient.getNodeStatus(nodeID);
> + if (event is UpdateNodeStatus) {
> + yield latestState.rebuild((b) => b..status.replace(status!));
> + final rrdData =
> + await apiClient.getNodeRRDdata(nodeID, PveRRDTimeframeType.hour);
> + yield latestState.rebuild((b) => b..rrdData.replace(rrdData));
> + final services = await apiClient.getNodeServices(nodeID);
> + yield latestState.rebuild((b) => b..services.replace(services));
> + try {
> + final updates = await apiClient.getNodeAptUpdate(nodeID);
> + yield latestState.rebuild((b) => b..updates.replace(updates));
> yield latestState
> - .rebuild((b) => b..updatesQueryPermissionFailure = true);
> + .rebuild((b) => b..updatesQueryPermissionFailure = false);
> + }
> + on ProxmoxApiException catch (e) {
> + // only throw on non permission related errors
> + if (e.statusCode != 403) {
> + rethrow;
> + }
> + else {
> + yield latestState
> + .rebuild((b) => b..updatesQueryPermissionFailure = true);
> + }
> + }
> + final disks = await apiClient.getNodeDisksList(nodeID);
> + yield latestState.rebuild((b) => b..disks.replace(disks));
> + }
> +
> + if (event is PerformNodeAction) {
> + await apiClient.doResourceAction(
> + nodeID,
> + '',
> + 'node',
> + event.action,
> + parameters: <String, String>{});
> + yield latestState;
> }
> }
> - final disks = await apiClient.getNodeDisksList(nodeID);
> - yield latestState.rebuild((b) => b..disks.replace(disks));
> - }
> - if (event is PerformNodeAction) {
> - await apiClient.doResourceAction(nodeID, '', 'node', event.action,
> - parameters: <String, String>{});
> - yield latestState;
> - }
> + catch(e){
> + yield latestState.rebuild(
> + (b) => b
> + ..updatesQueryPermissionFailure = true
> + );
> + if (context.mounted){
> + Navigator.of(context).pop();
Instead of navigating back immediately, I think it would make sense to
instead display an AlertDialog, showing the error message to the user
and letting them leave the screen through a "Close" button in the
dialog. Otherwise, it feels a bit like the app is taking control away
from the user. We use this approach in other places in the app (e.g. .
What do you think?
> + }
> + else {}
> + }
> }
> }
Wrapping the whole block in a try-catch without a rethrow also means
that users will be navigated back to the home screen without an error
message if e.g. the getNodeDisksList call returns an error. I think it'd
make sense to rethrow here, similar to above.
>
> diff --git a/lib/main.dart b/lib/main.dart
> index a985e3a..bbd3c97 100644
> --- a/lib/main.dart
> +++ b/lib/main.dart
> @@ -330,15 +330,18 @@ class MyApp extends StatelessWidget {
> final rbloc = Provider.of<PveResourceBloc>(context);
> return MultiProvider(
> providers: [
> +
> Provider<PveNodeOverviewBloc>(
> create: (context) => PveNodeOverviewBloc(
> apiClient: state.apiClient,
> nodeID: nodeID,
> + context: context,
> init: PveNodeOverviewState.init(
> rbloc.latestState.isStandalone),
> )..events.add(UpdateNodeStatus()),
> dispose: (context, bloc) => bloc.dispose(),
> ),
> +
> Provider<PveTaskLogBloc>(
> create: (context) => PveTaskLogBloc(
> apiClient: state.apiClient,
> diff --git a/lib/pages/main_layout_slim.dart b/lib/pages/main_layout_slim.dart
> index 322bcac..5fa6199 100644
> --- a/lib/pages/main_layout_slim.dart
> +++ b/lib/pages/main_layout_slim.dart
> @@ -254,7 +254,7 @@ class MobileDashboard extends StatelessWidget {
> fontWeight: FontWeight.bold,
> color: Colors.white),
> ),
> - onPressed: () => showDialog(
> + onPressed: () => (
This does not seem to be related to the issue? The subscription dialog
cannot be opened anymore with this change.
> context: context,
> builder: (c) => const PveSubscriptionAlertDialog(),
> ),
> @@ -556,7 +556,7 @@ class MobileDashboard extends StatelessWidget {
> }
> }
>
> -class PveNodeListTile extends StatelessWidget {
> +class PveNodeListTile extends StatelessWidget{
> final String name;
> final bool online;
> final String type;
> @@ -564,21 +564,58 @@ class PveNodeListTile extends StatelessWidget {
> final String? ip;
> const PveNodeListTile(
> {super.key,
> - required this.name,
> - required this.online,
> - required this.type,
> - this.level,
> - this.ip = ''});
> + required this.name,
> + required this.online,
> + required this.type,
> + this.level,
> + this.ip = ''});
> +
> @override
> Widget build(BuildContext context) {
> + late Color splashColor;
> + if (online){
> + splashColor = ProxmoxColors.greyShade40;
> + }
> + else {
> + splashColor = Colors.transparent;
> + }
> return ListTile(
> leading: Icon(
> Renderers.getDefaultResourceIcon(type),
> ),
> title: Text(name),
> + splashColor: splashColor,
> subtitle: Text(getNodeTileSubtitle(online, level, ip)),
> trailing: Icon(Icons.power, color: online ? Colors.green : Colors.grey),
> - onTap: () => Navigator.pushNamed(context, '/nodes/$name'),
> + onTap: () => {
> + if (online){
> + Navigator.pushNamed(context, '/nodes/$name')
> + }
> + else{
> + showDialog(
> + context: context,
> + builder: (context) {
> + return AlertDialog(
> + contentPadding: const EdgeInsets.fromLTRB(24.0, 12.0, 24.0, 16.0),
> + title: Row(
> + mainAxisAlignment: MainAxisAlignment.spaceBetween,
> + children: <Widget>[
> + Text('Error'),
> + Icon(Icons.warning),
> + ],
> + ),
> + content: Text("Node offline."),
nit: I think "Node is offline" would feel more polished. Or it should
not have a period, since it's not a sentence.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel