public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
To: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
Cc: pve-devel@lists.proxmox.com
Subject: Re: [PATCH manager v4 1/2] fix #5475: configurable window title
Date: Wed, 16 Sep 2026 16:51:05 +0200	[thread overview]
Message-ID: <wwvnuxg2padjlnxbzjgq6u7ur2if4yx5zazox45oyrinphblzu@uevfbnmvhdfw> (raw)
In-Reply-To: <20260914062202.27182-2-t.ellmenreich@proxmox.com>

thanks for the patch! I made a few comments inline, 

I also noticed that setting the window title to node-and-cluster fails
silently on standalone nodes, which took me a bit to realize. not sure
if rejecting it for standalone nodes at the API layer makes sense since
the setting becomes meaningful once a cluster is created, but maybe a
hint in the dialog would be enough?

On Mon, Sep 14, 2026 at 08:22:01AM +0200, Thomas Ellmenreich wrote:
> Added a new 'ui-settings' format string to the datacenter.cfg which now
> has a 'title' option to configure the browser tab title for pve tabs.
> 
> Adjusted the index.html templating to construct the correct window title
> on every request according to the cluster configuration.
> 
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  PVE/Service/pveproxy.pm       | 27 ++++++++++++++++
>  www/index.html.tpl            |  2 +-
>  www/manager6/UIOptions.js     |  6 ++++
>  www/manager6/dc/OptionView.js | 61 +++++++++++++++++++++++++++++++++++
>  4 files changed, 95 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Service/pveproxy.pm b/PVE/Service/pveproxy.pm
> index dfdd014c..d0ebdb71 100755
> --- a/PVE/Service/pveproxy.pm
> +++ b/PVE/Service/pveproxy.pm
> @@ -202,6 +202,30 @@ my sub get_path_mtime {
>      return $mtime;
>  }
>  
> +# builds the window title from the datacenter.cfg configuration
> +my $get_window_title = sub {
nit: `my sub foo` seems to be preferred over `my $foo = sub` in this
file.
> +    my ($title_enum, $nodename) = @_;
> +
> +    my $title;
> +
> +    if (defined $title_enum) {
> +        eval {
> +            if ($title_enum eq "node-and-cluster") {
> +                my $clinfo = PVE::Cluster::get_clinfo();
> +                my $clustername = $clinfo->{cluster}->{name};
> +
> +                $title = "$nodename - $clustername" if $clustername;
> +            } elsif ($title_enum eq "fqdn") {
> +                $title = PVE::Tools::get_fqdn($nodename);
haven't looked too deeply into it, but afaict on a standard setup this
resolves the FQDN from /etc/hosts. still, this lookup is now reachable
from an unauthenticated path, could that be problematic in some setups?
not sure how likely a DNS stall is here, just wanted to mention it.
> +            }
> +        };
> +
> +        warn "failed to create '$title_enum' window title: $@" if $@;
> +    }
> +
> +    return ($title // "$nodename") . " - 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 +256,12 @@ sub get_index {
>          }
>      }
>  
> +    my $window_title;
currently if anything dies in the eval block before $get_window_title is
called, $window_title stays undef.

you coudl read only the title setting inside the eval and call 
$get_window_title after the block, since it cannot die anyway.
>      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';
> @@ -277,6 +303,7 @@ sub get_index {
>          console => $args->{console},
>          nodename => $nodename,
>          arch => PVE::Tools::get_host_dpkg_arch(),
> +        window_title => $window_title,
>          debug => $debug,
>          version => "$version",
>          wtversion => $wtversion,
> diff --git a/www/index.html.tpl b/www/index.html.tpl
> index c18e6411..a45bad39 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..49307fa4 100644
> --- a/www/manager6/UIOptions.js
> +++ b/www/manager6/UIOptions.js
> @@ -90,6 +90,12 @@ Ext.define('PVE.UIOptions', {
>          alphabetical: gettext('Alphabetical'),
>      },
>  
> +    titleOptions: {
> +        __default__: 'Node Name (Default)',
> +        'node-and-cluster': 'Node and Cluster Name',
> +        fqdn: 'Fully Qualified Domain Name (FQDN)',
> +    },
these should probably also be wrapped in gettext
> +
>      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);
nit: this shows the raw enum value, which confused me a bit given the 
dropdown in the edit window shows the display text. as you noted
off-list, the display names could make the column quite a lot wider,
however I don't think this would be an issue, since there are other
columns that are pretty wide as well (e.g. the tag style overrides).
> +                }
> +                return txt;
> +            },
> +            header: gettext('Ui Settings'),
as far as I could tell from a quick search through the codebase, we do
not use Ui (with small i) anywhere in user-facing strings, I think UI 
would be better here.
> +            editor: {
> +                xtype: 'proxmoxWindowEdit',
> +                width: 800,
> +                subject: gettext('Ui Settings'),
same here
> +                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',
> -- 
> 2.47.3
> 
> 
> 
> 
> 




  reply	other threads:[~2026-09-16 14:51 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-14  6:22 [PATCH cluster/manager v4 0/2] Configurable window titles for nodes Thomas Ellmenreich
2026-09-14  6:22 ` [PATCH manager v4 1/2] fix #5475: configurable window title Thomas Ellmenreich
2026-09-16 14:51   ` Arthur Bied-Charreton [this message]
2026-09-14  6:22 ` [PATCH cluster v4 2/2] " Thomas Ellmenreich
2026-09-16 14:52   ` Arthur Bied-Charreton
2026-09-16 15:02 ` [PATCH cluster/manager v4 0/2] Configurable window titles for nodes Arthur Bied-Charreton

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=wwvnuxg2padjlnxbzjgq6u7ur2if4yx5zazox45oyrinphblzu@uevfbnmvhdfw \
    --to=a.bied-charreton@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