public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
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 12:27:26 +0200	[thread overview]
Message-ID: <DK511D00TB8U.1DRR6IBH6XE92@proxmox.com> (raw)
In-Reply-To: <20260720100329.118508-2-t.ellmenreich@proxmox.com>

On Mon Jul 20, 2026 at 12:03 PM CEST, Thomas Ellmenreich wrote:
> Allow setting of the new 'ui-settings' format string and the nested
> 'title' property. Setting this property defines the window titles
> for all cluster nodes.
>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
TL;DR: 
- missing argument for PVE::Tools::get_fqdn($nodename)
- missing defined checks pveproxy.pm
- confusing option names

I tested the patch on a pve cluster consisting of 2 nodes.
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.
2. I encountered a bug regarding the fdqn setting (see comment below)
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.

see commnets below:

> ---
>  PVE/Service/pveproxy.pm       | 21 ++++++++++++
>  www/index.html.tpl            |  2 +-
>  www/manager6/UIOptions.js     |  6 ++++
>  www/manager6/dc/OptionView.js | 61 +++++++++++++++++++++++++++++++++++
>  4 files changed, 89 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/Service/pveproxy.pm b/PVE/Service/pveproxy.pm
> index c6011a00..811295f4 100755
> --- a/PVE/Service/pveproxy.pm
> +++ b/PVE/Service/pveproxy.pm
> @@ -202,6 +202,24 @@ my sub get_path_mtime {
>      return $mtime;
>  }
>  
> +# builds the window title from the datacenter.cfg configuration
> +my $get_window_title = sub {
> +    my ($title_enum, $nodename) = @_;
> +
> +    my $title = "$nodename";
> +
> +    if ($title_enum eq "node-and-cluster") {
> +        my $clinfo = PVE::Cluster::get_clinfo();
> +        my $clustername = $clinfo->{cluster}->{name};
			 	^^^^
This returned undef once when i was messing with the cluster settings, when i reloaded the web page using CTL +
F5 on firefox. This resulted in my window title to
default to the link address of the web page. I suspect this is because
the error is surpressed somewhere, probably by the eval block below.

The correct formatting appeared when i accessed the WebUI in a new
brower tab.

Initializing the window title using a safe default in case of
an error, might be an option.
> +
> +        $title = "$nodename - $clustername";
> +    } elsif ($title_enum eq "fqdn") {
> +        $title = PVE::Tools::get_fqdn();
			^^^^^
PVE:Tools::get_fqdn requires a parameter. The first line of the function
(on my test machine is):
my ($nodename) = @_;

This resulted in an error (captured by journalctl -u pveproxy -f):
Use of uninitialized value $lang in concatenation (.) or string at /usr/share/perl5/PVE/Service/pveproxy.pm line 273.
pve-3 pveproxy[27666]: getaddrinfo: Name or service not known at /usr/share/perl5/PVE/Tools.pm line 915.

I belive the fix is:
$title = PVE::Tools::get_fqdn($nodename);

Also: a misconfigured /etc/hosts file might result in the function
returning undef --> some type of error handling is needed.
> +    }
> +
> +    return $title . " - Proxmox Virtual Environment";
> +};
> +
>  # NOTE: Requests to those pages are not authenticated so we must be very careful here
>  sub get_index {
>      my ($nodename, $server, $r, $args) = @_;
> @@ -232,10 +250,12 @@ sub get_index {
>          }
>      }
>  
> +    my $window_title;
>      my $consent_text;
>      eval {
>          my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>          $consent_text = $dc_conf->{'consent-text'};
> +        $window_title = $get_window_title->($dc_conf->{'ui-settings'}->{'title'}, $nodename);
>  
>          if (!$lang) {
>              $lang = $dc_conf->{language} // 'en';
> @@ -276,6 +296,7 @@ sub get_index {
>          token => $token,
>          console => $args->{console},
>          nodename => $nodename,
> +        window_title => $window_title,
>          debug => $debug,
>          version => "$version",
>          wtversion => $wtversion,
> diff --git a/www/index.html.tpl b/www/index.html.tpl
> index 74ee02d9..5138b7cc 100644
> --- a/www/index.html.tpl
> +++ b/www/index.html.tpl
> @@ -4,7 +4,7 @@
>      <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
>      <meta http-equiv="X-UA-Compatible" content="IE=edge">
>      <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, user-scalable=no">
> -    <title>[% nodename %] - Proxmox Virtual Environment</title>
> +    <title>[% window_title %]</title>
>      <link rel="icon" sizes="128x128" href="/pve2/images/logo-128.png" />
>      <link rel="apple-touch-icon" sizes="128x128" href="/pve2/images/logo-128.png" />
>      <link rel="stylesheet" type="text/css" href="/pve2/ext6/theme-crisp/resources/theme-crisp-all.css?ver=7.0.0" />
> diff --git a/www/manager6/UIOptions.js b/www/manager6/UIOptions.js
> index 8c4674af..bb0af8d5 100644
> --- a/www/manager6/UIOptions.js
> +++ b/www/manager6/UIOptions.js
> @@ -90,6 +90,12 @@ Ext.define('PVE.UIOptions', {
>          alphabetical: gettext('Alphabetical'),
>      },
>  
> +    titleOptions: {
> +        __default__: '$nodename - Proxmox Virtual Environment',
> +        'node-and-cluster': '$nodename - $clustername - Proxmox Virtual Environment',
> +        fqdn: '$fqdn - Proxmox Virtual Environment',
> +    },
> +
>      shouldSortTags: function () {
>          return !(PVE.UIOptions.options['tag-style']?.ordering === 'config');
>      },
> diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
> index dc12aa7e..208cc147 100644
> --- a/www/manager6/dc/OptionView.js
> +++ b/www/manager6/dc/OptionView.js
> @@ -91,6 +91,67 @@ Ext.define('PVE.dc.OptionView', {
>              defaultValue: '__default__',
>              deleteEmpty: true,
>          });
> +        me.rows['ui-settings'] = {
> +            required: true,
> +            renderer: (value) => {
> +                if (value === undefined) {
> +                    return gettext('No Overrides');
> +                }
> +                let txt = '';
> +                if (value.title) {
> +                    txt += Ext.String.format(gettext('Title: {0}'), value.title);
> +                }
> +                return txt;
> +            },
> +            header: gettext('Ui Settings'),
> +            editor: {
> +                xtype: 'proxmoxWindowEdit',
> +                width: 800,
> +                subject: gettext('Ui Settings'),
> +                fieldDefaults: {
> +                    labelWidth: 100,
> +                },
> +                url: '/api2/extjs/cluster/options',
> +                items: [
> +                    {
> +                        xtype: 'inputpanel',
> +                        setValues: function (values) {
> +                            if (values === undefined) {
> +                                return undefined;
> +                            }
> +                            values = values?.['ui-settings'] ?? {};
> +                            values.title = values.title || '__default__';
> +                            return Proxmox.panel.InputPanel.prototype.setValues.call(this, values);
> +                        },
> +                        onGetValues: function (values) {
> +                            let style = {};
> +                            if (values.title) {
> +                                style.title = values.title;
> +                            }
> +                            let value = PVE.Parser.printPropertyString(style);
> +                            if (value === '') {
> +                                return {
> +                                    delete: 'ui-settings',
> +                                };
> +                            }
> +                            return {
> +                                'ui-settings': value,
> +                            };
> +                        },
> +                        items: [
> +                            {
> +                                xtype: 'proxmoxKVComboBox',
> +                                name: 'title',
> +                                fieldLabel: gettext('Title'),
> +                                comboItems: Object.entries(PVE.UIOptions.titleOptions),
> +                                deleteEmpty: true,
> +                                defaultValue: '__default__',
> +                            },
> +                        ],
> +                    },
> +                ],
> +            },
> +        };
>          me.add_text_row('email_from', gettext('Email from address'), {
>              deleteEmpty: true,
>              vtype: 'proxmoxMail',





  reply	other threads:[~2026-07-22 10:27 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 [this message]
2026-07-22 12:43     ` Thomas Ellmenreich
2026-07-22 14:49       ` Elias Huhsovitz
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=DK511D00TB8U.1DRR6IBH6XE92@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal