[PATCH cluster/manager/yew-mobile-gui v6 0/4] Configurable window titles for nodes

[PATCH cluster/manager/yew-mobile-gui v6 0/4] Configurable window titles for nodes

[Thomas Ellmenreich]

Thank you @Dominik, @Thomas, @Elias, @Arthur for your comments.

Configurable window titles for nodes
====================================

Currently the window title is fixed to the following string:

$nodename - Proxmox Virtual Environment

This has become problematic for users with multiple clusters, each containing
the same node name. To help distinguish between tabs, it has been requested [1]
that the window title be made configurable with the fqdn as an option.

Implementation
--------------

After considering the different options, I have deemed a server side
implementation, through the datacenter.cfg, to be the better option.
Specifically because of its durability across sessions and because it
automatically configures the title for all users and all nodes across the
cluster.

The config now contains a new ui-settings format string which then in turn
contains a 'title' enum field for the window title. This enum string will then
be converted to a window title and inserted in the index.html.tpl template on
every request.

The current enum options are:

- default - Displays the default '$nodename - Proxmox Virtual Environment' string
  if the config enum is not present

- 'node-and-cluster' - Displays the nodename and then the clustername in the
  following style: '$nodename - $clustername - Proxmox Virtual Environment'

- 'fqdn' - Displays the current fqdn as returned by `PVE::Tools::get_fqdn()`
  like so: '$fqdn - Proxmox Virtual Environment'


Possible future extensions
--------------------------

- If proper cross-node user settings are implemented in the future, they could
  be used to allow users to define their own window title settings and have
  these settings be persisted across sessions

- One of the selection options for window titles could be a user defined
  template string which would allow users of PVE to define their own
  window titles.

Changelog
---------

* v5 -> v6
    - In v5 I added a cache that contains the fqdn and a timestamp at which
      it was fetched, after Arthur's comments [2]. The cache was then refreshed
      after 24h passed. In the new version, I now set the exact time for refresh
      instead of the time of insertion. This allows for a different refresh
      time, in the case that fetching the fqdn fails.

    - The `get_window_title_prefix` helper never actually died if the contained
      eval block returned an error. I delayed the returning of the prefix so that
      the subroutine may die with the context.

    - Since `get_window_title_prefix` can fail, I moved it after the language
      setting, since setting the correct language is of greater importance.

    - Made the same changes to the index file of the mobile ui, since it will
      access the same pve-manager backend.

    - Split the pve-manager patch into two separate patches, so that the
      window title part may be reverted without reverting the ui-settings
      setup.

    - If an error is thrown inside the `get_window_title_prefix` utility, it now
      has the trailing newline removed, if there is one.

* v4 -> v5
    - When the window title is set to fqdn, cache it instead of fetching it on
      every request. The fetch happens on restart and every 24 hours.

    - Reordering of the window title construction so that it will always fall
      back to the original default.

    - In the settings overview, the window title settings are now properly shown
      instead of showing the enum value.

    - Wrapped some text with 'gettext'.

    - Reordering of the patches since the (now) second one depends on the
      first one.

* v3 -> v4
    - rebased on master

* v2 -> v3
    - The creation of the final title is now wrapped in an 'eval' block to catch
      any exceptions. If an error occurs or the return value is undefined, the
      default title is shown.

    - `get_fqdn` is now correctly called with the $nodename. Thanks @Elias ;)

    - The combo box showing the title options now displays descriptive names for
      the different options instead of an example of what the real title would
      look like.

* v1 -> v2
    - Instead of storing the 'window-title' setting directly in the datacenter.cfg
      its now nested in a more general 'ui-settings' format string. This allows
      easier expansion in the future

    - All adjustments that were needed to create the 'ui-settings' format string

* RFC -> v1
    - change of the `datacenter.cfg` setting title from "windowtitle" to
      "window-title"

    - renamed `nodeandcluster` to `node-and-cluster`

    - added the " - Proxmox Virtual Environment" trailer to all titles

    - removed the questions from this text and added the "Possible future
      extensions" section

[1]: https://bugzilla.proxmox.com/show_bug.cgi?id=5475
[2]: https://lore.proxmox.com/pve-devel/wwvnuxg2padjlnxbzjgq6u7ur2if4yx5zazox45oyrinphblzu@uevfbnmvhdfw/


pve-cluster:

Thomas Ellmenreich (1):
  datacenter config: add ui settings

 src/PVE/DataCenterConfig.pm | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)


pve-manager:

Thomas Ellmenreich (2):
  ui: add cluster level ui settings
  fix #5475: ui: enable configurable window title

 PVE/Service/pveproxy.pm       | 45 ++++++++++++++++++++++++
 www/index.html.tpl            |  2 +-
 www/manager6/UIOptions.js     |  6 ++++
 www/manager6/dc/OptionView.js | 64 +++++++++++++++++++++++++++++++++++
 4 files changed, 116 insertions(+), 1 deletion(-)


pve-yew-mobile-gui:

Thomas Ellmenreich (1):
  fix #5475: template: use new window-title in index template

 debian/control | 1 +
 index.html.tpl | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)


Summary over all repositories:
  7 files changed, 143 insertions(+), 2 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH cluster v6 1/4] datacenter config: add ui settings

[Thomas Ellmenreich]

Add a new 'ui-settings' format string to the datacenter.cfg. It currently
only contains a 'title' option so that the window title of nodes browser
tabs may be configured.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
Reviewed-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
Reviewed-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
Tested-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
Tested-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/DataCenterConfig.pm | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/src/PVE/DataCenterConfig.pm b/src/PVE/DataCenterConfig.pm
index 004122e..751d8ed 100644
--- a/src/PVE/DataCenterConfig.pm
+++ b/src/PVE/DataCenterConfig.pm
@@ -344,6 +344,16 @@ my $user_tag_privs_format = {
     },
 };
 
+my $ui_settings_format = {
+    'title' => {
+        optional => 1,
+        type => 'string',
+        enum => ['node-and-cluster', 'fqdn'],
+        description =>
+            "Define the text displayed in the browser tab title for all nodes in this cluster.",
+    },
+};
+
 my $datacenter_schema = {
     type => "object",
     additionalProperties => 0,
@@ -436,6 +446,12 @@ my $datacenter_schema = {
             # FIXME: remove 'applet' with 9.0 (add pve8to9 check!)
             enum => ['applet', 'vv', 'html5', 'xtermjs'],
         },
+        'ui-settings' => {
+            optional => 1,
+            type => "string",
+            format => $ui_settings_format,
+            description => "For cluster wide ui settings.",
+        },
         email_from => {
             optional => 1,
             type => 'string',
@@ -618,6 +634,10 @@ sub parse_datacenter_config {
         $res->{'tag-style'} = parse_property_string($tag_style_format, $tag_style);
     }
 
+    if (my $ui_settings = $res->{'ui-settings'}) {
+        $res->{'ui-settings'} = parse_property_string($ui_settings_format, $ui_settings);
+    }
+
     if (my $user_tag_privs = $res->{'user-tag-access'}) {
         $res->{'user-tag-access'} = parse_property_string($user_tag_privs_format, $user_tag_privs);
 
@@ -711,6 +731,11 @@ sub write_datacenter_config {
         $cfg->{'tag-style'} = PVE::JSONSchema::print_property_string($tag_style, $tag_style_format);
     }
 
+    if (ref(my $ui_settings = $cfg->{'ui-settings'})) {
+        $cfg->{'ui-settings'} =
+            PVE::JSONSchema::print_property_string($ui_settings, $ui_settings_format);
+    }
+
     if (ref(my $user_tag_privs = $cfg->{'user-tag-access'})) {
         if (my $user_tags = $user_tag_privs->{'user-allow-list'}) {
             $user_tag_privs->{'user-allow-list'} = join(';', sort $user_tags->@*);
-- 
2.47.3

[PATCH manager v6 2/4] ui: add cluster level ui settings

[Thomas Ellmenreich]

Extend the cluster settings editor to include editing for the
ui-settings format string. This commit does not contain any actual
ui-setting's yet, the window-title will be added in the next commit.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 www/manager6/dc/OptionView.js | 47 +++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index dc12aa7e..4fc18ae7 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -91,6 +91,53 @@ Ext.define('PVE.dc.OptionView', {
             defaultValue: '__default__',
             deleteEmpty: true,
         });
+        me.rows['ui-settings'] = {
+            required: true,
+            renderer: (value) => {
+                if (value === undefined) {
+                    return gettext('No Overrides');
+                }
+                return '';
+            },
+            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'] ?? {};
+                            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: [],
+                    },
+                ],
+            },
+        };
         me.add_text_row('email_from', gettext('Email from address'), {
             deleteEmpty: true,
             vtype: 'proxmoxMail',
-- 
2.47.3

[PATCH manager v6 3/4] fix #5475: ui: enable configurable window title

[Thomas Ellmenreich]

By setting the title property in the cluster ui-settings, the window
title displayed for PVE instances can be changed. The setting is
evaluated on every fetch of the index template, although not every
evaluation causes a refresh of the title as the fqdn is only renewed
every 24h.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5475
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 PVE/Service/pveproxy.pm       | 45 +++++++++++++++++++++++++++++++++++
 www/index.html.tpl            |  2 +-
 www/manager6/UIOptions.js     |  6 +++++
 www/manager6/dc/OptionView.js | 21 ++++++++++++++--
 4 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/PVE/Service/pveproxy.pm b/PVE/Service/pveproxy.pm
index dfdd014c..397e1490 100755
--- a/PVE/Service/pveproxy.pm
+++ b/PVE/Service/pveproxy.pm
@@ -202,6 +202,46 @@ my sub get_path_mtime {
     return $mtime;
 }
 
+my $FQDN_CACHE = {
+    next_refresh => undef,
+    fqdn => undef,
+};
+
+my sub get_window_title_prefix {
+    my ($title_enum, $nodename) = @_;
+
+    if (!defined($title_enum)) {
+        return;
+    }
+
+    my $prefix = eval {
+        if ($title_enum eq "node-and-cluster") {
+            my $clinfo = PVE::Cluster::get_clinfo();
+            my $clustername = $clinfo->{cluster}->{name};
+
+            return "$nodename - $clustername" if $clustername;
+        } elsif ($title_enum eq "fqdn") {
+            my $now = time();
+            if (!$FQDN_CACHE->{next_refresh} || $now >= $FQDN_CACHE->{next_refresh}) {
+                $FQDN_CACHE->{fqdn} = eval { PVE::Tools::get_fqdn($nodename); };
+                if (my $err = $@) {
+                    $FQDN_CACHE->{next_refresh} = $now + 60;
+                    die $err;
+                }
+                $FQDN_CACHE->{next_refresh} = $now + 24 * 60 * 60;
+            }
+
+            return $FQDN_CACHE->{fqdn};
+        }
+    };
+
+    if (my $err = $@) {
+        chomp $err;
+        die "failed to create '$title_enum' window title: $err";
+    }
+    return $prefix;
+}
+
 # NOTE: Requests to those pages are not authenticated so we must be very careful here
 sub get_index {
     my ($nodename, $server, $r, $args) = @_;
@@ -232,6 +272,7 @@ sub get_index {
         }
     }
 
+    my $window_title_prefix;
     my $consent_text;
     eval {
         my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
@@ -240,6 +281,8 @@ sub get_index {
         if (!$lang) {
             $lang = $dc_conf->{language} // 'en';
         }
+        $window_title_prefix =
+            get_window_title_prefix($dc_conf->{'ui-settings'}->{'title'}, $nodename);
     };
     warn "$@\n" if $@;
 
@@ -277,6 +320,8 @@ sub get_index {
         console => $args->{console},
         nodename => $nodename,
         arch => PVE::Tools::get_host_dpkg_arch(),
+        window_title => ($window_title_prefix // "$nodename")
+            . " - Proxmox Virtual Environment",
         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..e94a75df 100644
--- a/www/manager6/UIOptions.js
+++ b/www/manager6/UIOptions.js
@@ -90,6 +90,12 @@ Ext.define('PVE.UIOptions', {
         alphabetical: gettext('Alphabetical'),
     },
 
+    titleOptions: {
+        __default__: gettext('Node Name (Default)'),
+        'node-and-cluster': gettext('Node and Cluster Name'),
+        fqdn: gettext('Fully Qualified Domain Name (FQDN)'),
+    },
+
     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 4fc18ae7..ebf61270 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -97,7 +97,14 @@ Ext.define('PVE.dc.OptionView', {
                 if (value === undefined) {
                     return gettext('No Overrides');
                 }
-                return '';
+                let txt = '';
+                if (value.title) {
+                    txt += Ext.String.format(
+                        gettext('Title: {0}'),
+                        PVE.UIOptions.titleOptions[value.title],
+                    );
+                }
+                return txt;
             },
             header: gettext('UI Settings'),
             editor: {
@@ -116,6 +123,7 @@ Ext.define('PVE.dc.OptionView', {
                                 return undefined;
                             }
                             values = values?.['ui-settings'] ?? {};
+                            values.title = values.title || '__default__';
                             return Proxmox.panel.InputPanel.prototype.setValues.call(this, values);
                         },
                         onGetValues: function (values) {
@@ -133,7 +141,16 @@ Ext.define('PVE.dc.OptionView', {
                                 'ui-settings': value,
                             };
                         },
-                        items: [],
+                        items: [
+                            {
+                                xtype: 'proxmoxKVComboBox',
+                                name: 'title',
+                                fieldLabel: gettext('Title'),
+                                comboItems: Object.entries(PVE.UIOptions.titleOptions),
+                                deleteEmpty: true,
+                                defaultValue: '__default__',
+                            },
+                        ],
                     },
                 ],
             },
-- 
2.47.3

[PATCH pve-yew-mobile-gui v6 4/4] fix #5475: template: use new window-title in index template

[Thomas Ellmenreich]

When constructing the index file, pve-manager now provides a
window-title, so use this window title. Since a specific pve-manager
version is required add a 'Breaks'.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5475
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 debian/control | 1 +
 index.html.tpl | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/debian/control b/debian/control
index 834b67c..3e04c60 100644
--- a/debian/control
+++ b/debian/control
@@ -56,6 +56,7 @@ Depends: fonts-font-awesome,
          pve-yew-mobile-i18n,
          ${misc:Depends},
          ${shlibs:Depends},
+Breaks: pve-manager (<= 9.2.23)
 Recommends: proxmox-ve,
 Description: Mobile Web UI to for the Proxmox Virtual Environment
  This package provides the mobile web UI of the Proxmox Virtual Environment.
diff --git a/index.html.tpl b/index.html.tpl
index 67fb53a..350b061 100644
--- a/index.html.tpl
+++ b/index.html.tpl
@@ -11,7 +11,7 @@
   <meta http-equiv="Content-type" content="text/html; charset=utf-8" />
   <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="manifest" href="[% base_path %]/manifest.json" />
 
-- 
2.47.3