From: "Elias Huhsovitz" <e.huhsovitz@proxmox.com>
To: "Thomas Ellmenreich" <t.ellmenreich@proxmox.com>,
<pve-devel@lists.proxmox.com>
Subject: Re: [PATCH manager v2 1/2] fix #5475: configurable window title
Date: Wed, 22 Jul 2026 16:49:04 +0200 [thread overview]
Message-ID: <DK56LOIP3U5Z.2P4BD9P0ZEDQQ@proxmox.com> (raw)
In-Reply-To: <DK53XT3KQT5N.2A0407A0IPVXX@proxmox.com>
A few comments adressing 1. & 3.
On Wed Jul 22, 2026 at 2:43 PM CEST, Thomas Ellmenreich wrote:
> Thanks again for your review. I realised I had a question here as well.
>
> [snip]
>
>> 1. The setting names render including the $ symbol, e.g. ($nodename, $fqdn) in the WebUI.
>> This makes it seem broken from an end user point of view.
>> You generally do not expect placeholder values in settings menu.
>
> Rendering the '$' was on purpose, as they are placeholders. Would you
> recommend removing just the '$' or would you rather render the actual
> values?
>
> I technically copied it from somewhere, as I had a different placeholder
> marker, although I can't seem to find that reference anymore.
While using $ is a standard programming convention for variables,
displaying it literally in a user-facing dropdown
(e.g., $nodename - Proxmox Virtual Environment)
can look like a broken string interpolation or a rendering bug to an end user.
Instead of showing the template string, my suggestions would be
descriptive labels for the items, e.g.:
* __default__ -> "Node Name (Default)"
* node-and-cluster -> "Node and Cluster Name"
* fqdn -> Fully Qualified Domain Name (FQDN)
>> 3. I recommend adding some general robustness in case of undef values:
>> Initialize $window_title to a safe value not relying on the cluster, in
>> case of an error. Expect that the underlying API/calls might return
>> undef.
>
> It was an active choice to show something broken in the case of undef, as I
> thought hiding the error could also be confusing. But then again, I don't mind
> using the fallback value.
IMO the problem is that the undef might get dereference and cause a 'die'
potentially aborting the entire entire eval block in pveproy.pm:255.
I would advocate for "graceful" degradation:
- add a defined check to prevent the crash
- initialize the $window_title with a safe default.
You can also add a warn statement inside the subroutine so it does not
fail silently.
next prev parent reply other threads:[~2026-07-22 14:49 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-20 10:03 [PATCH cluster/manager v2 0/2] Configurable window titles for nodes Thomas Ellmenreich
2026-07-20 10:03 ` [PATCH manager v2 1/2] fix #5475: configurable window title Thomas Ellmenreich
2026-07-22 10:27 ` Elias Huhsovitz
2026-07-22 12:43 ` Thomas Ellmenreich
2026-07-22 14:49 ` Elias Huhsovitz [this message]
2026-07-20 10:03 ` [PATCH cluster v2 2/2] " Thomas Ellmenreich
2026-07-22 10:38 ` Elias Huhsovitz
2026-07-22 11:23 ` Thomas Ellmenreich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DK56LOIP3U5Z.2P4BD9P0ZEDQQ@proxmox.com \
--to=e.huhsovitz@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=t.ellmenreich@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.