all lists on 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 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.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal