[pve-devel] [PATCH font-logos/manager v2 0/5] fix #2435: lxc: show distro and privileged status in summary

[pve-devel] [PATCH font-logos/manager v2 0/5] fix #2435: lxc: show distro and privileged status in summary

[Christoph Heiss]

This implements #2435 [0]. Show the unprivileged status in the summary
panel, the distro logo and name in the title of the summary panel.

Patch 1 & 2 fix two small typos in the `fonts-font-logos` package.
Patch 3 then prepares the pveproxy to serve the required CSS and font
files for the icon font we use [1] (packaged here [2]), and finally
patch 4 & 5 finally wire everything up into the UI.

N.B.: The `fonts-font-logos` package is not yet available through the
repos yet (as far as I could see), so it must be built & installed
locally for testing.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=2435
[1] https://github.com/Lukas-W/font-logos
[2] https://git.proxmox.com/?p=fonts-font-logos.git;a=summary

fonts-font-logos:

Christoph Heiss (2):
  d/install: fix typo in css install path
  css: fix missing `@` for font-face rule

 debian/install     | 2 +-
 src/font-logos.css | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

pve-manager:

Christoph Heiss (3):
  pveproxy, ui, d/control: add font-logos
  ui: GuestStatusView: show privileged status as new row
  ui: GuestStatusView: show distro logo and name in summary header

 PVE/Service/pveproxy.pm               |  2 ++
 debian/control                        |  1 +
 www/index.html.tpl                    |  1 +
 www/manager6/panel/GuestStatusView.js | 51 ++++++++++++++++++++++++++-
 4 files changed, 54 insertions(+), 1 deletion(-)

--
2.39.2

[pve-devel] [PATCH font-logos v2 1/5] d/install: fix typo in css install path

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 debian/install | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/debian/install b/debian/install
index 0ef0655..69b9d81 100644
--- a/debian/install
+++ b/debian/install
@@ -1,2 +1,2 @@
 font-logos/assets/* /usr/share/fonts-font-logos/fonts
-font-logos.css  /usr/share/fonts-font-fonts/css
+font-logos.css  /usr/share/fonts-font-logos/css
--
2.41.0

[pve-devel] [PATCH font-logos v2 2/5] css: fix missing `@` for font-face rule

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 src/font-logos.css | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/font-logos.css b/src/font-logos.css
index 0de3d12..ab6c57d 100644
--- a/src/font-logos.css
+++ b/src/font-logos.css
@@ -1,4 +1,4 @@
-font-face {
+@font-face {
   font-display: auto;
   font-family: "font-logos";
   font-style: normal;
--
2.41.0

[pve-devel] [PATCH manager v2 3/5] pveproxy, ui, d/control: add font-logos

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 PVE/Service/pveproxy.pm | 2 ++
 debian/control          | 1 +
 www/index.html.tpl      | 1 +
 3 files changed, 4 insertions(+)

diff --git a/PVE/Service/pveproxy.pm b/PVE/Service/pveproxy.pm
index 9e8a2fecd..ac1085457 100755
--- a/PVE/Service/pveproxy.pm
+++ b/PVE/Service/pveproxy.pm
@@ -50,6 +50,7 @@ my $basedirs = {
     docs => '/usr/share/pve-docs',
     extjs => '/usr/share/javascript/extjs',
     fontawesome => '/usr/share/fonts-font-awesome',
+    fontlogos => '/usr/share/fonts-font-logos',
     i18n => '/usr/share/pve-i18n',
     manager => '/usr/share/pve-manager',
     novnc => '/usr/share/novnc-pve',
@@ -81,6 +82,7 @@ sub init {
     add_dirs($dirs, '/pve2/ext6/', "$basedirs->{extjs}/");
     add_dirs($dirs, '/pve2/fa/css/' =>  "$basedirs->{fontawesome}/css/");
     add_dirs($dirs, '/pve2/fa/fonts/' =>  "$basedirs->{fontawesome}/fonts/");
+    add_dirs($dirs, '/pve2/font-logos/' =>  "$basedirs->{fontlogos}/");
     add_dirs($dirs, '/pve2/images/' =>  "$basedirs->{manager}/images/");
     add_dirs($dirs, '/pve2/js/' =>  "$basedirs->{manager}/js/");
     add_dirs($dirs, '/pve2/locale/', "$basedirs->{i18n}/");
diff --git a/debian/control b/debian/control
index 3206b514f..036ded617 100644
--- a/debian/control
+++ b/debian/control
@@ -39,6 +39,7 @@ Depends: apt (>= 1.5~),
          cstream,
          dtach,
          fonts-font-awesome,
+         fonts-font-logos,
          gdisk,
          hdparm,
          ifupdown2 (>= 3.0) | ifenslave (>= 2.6),
diff --git a/www/index.html.tpl b/www/index.html.tpl
index b07ce5f17..46dc877bc 100644
--- a/www/index.html.tpl
+++ b/www/index.html.tpl
@@ -10,6 +10,7 @@
     <link rel="stylesheet" type="text/css" href="/pve2/ext6/theme-crisp/resources/theme-crisp-all.css?ver=7.0.0" />
     <link rel="stylesheet" type="text/css" href="/pve2/ext6/crisp/resources/charts-all.css?ver=7.0.0" />
     <link rel="stylesheet" type="text/css" href="/pve2/fa/css/font-awesome.css" />
+    <link rel="stylesheet" type="text/css" href="/pve2/font-logos/css/font-logos.css" />
     <link rel="stylesheet" type="text/css" href="/pve2/css/ext6-pve.css?ver=[% version %]" />
     <link rel="stylesheet" type="text/css" href="/pwt/css/ext6-pmx.css?ver=[% wtversion %]" />
     [%- IF theme != 'crisp' %]
-- 
2.41.0

[pve-devel] [PATCH manager v2 4/5] ui: GuestStatusView: show privileged status as new row

[Christoph Heiss]

As that info is not available through the store (which stores the
status), it must be fetched separately.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 www/manager6/panel/GuestStatusView.js | 32 +++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/www/manager6/panel/GuestStatusView.js b/www/manager6/panel/GuestStatusView.js
index 8db1f492c..ca2f03453 100644
--- a/www/manager6/panel/GuestStatusView.js
+++ b/www/manager6/panel/GuestStatusView.js
@@ -11,6 +11,29 @@ Ext.define('PVE.panel.GuestStatusView', {
 	};
     },
 
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	init: view => {
+	    if (view.pveSelNode.data.type !== 'lxc') {
+		return;
+	    }
+
+	    const nodename = view.pveSelNode.data.node;
+	    const vmid = view.pveSelNode.data.vmid;
+
+	    Proxmox.Utils.API2Request({
+		url: `/api2/extjs/nodes/${nodename}/lxc/${vmid}/config`,
+		waitMsgTargetView: view,
+		method: 'GET',
+		success: ({ result }) => {
+		    view.down('#unprivileged').updateValue(
+			Proxmox.Utils.format_boolean(result.data.unprivileged));
+		},
+	    });
+	},
+    },
+
     layout: {
 	type: 'vbox',
 	align: 'stretch',
@@ -58,6 +81,15 @@ Ext.define('PVE.panel.GuestStatusView', {
 	    },
 	    printBar: false,
 	},
+	{
+	    itemId: 'unprivileged',
+	    iconCls: 'fa fa-lock fa-fw',
+	    title: gettext('Unprivileged'),
+	    printBar: false,
+	    cbind: {
+		hidden: '{isQemu}',
+	    },
+	},
 	{
 	    xtype: 'box',
 	    height: 15,
-- 
2.41.0

[pve-devel] [PATCH manager v2 5/5] ui: GuestStatusView: show distro logo and name in summary header

[Christoph Heiss]

It fits neatly there, is rather unintrusive and yet still visible at
first sight. It also solves the problem of having to create a bigger
row, so that the icon is still easily recognisable. At the default
font-size of 13pt, this really wasn't the case.

Verified that each supported distro is present in the font and the name
matches up and tested through all supported distros (including
'unmanaged').

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 www/manager6/panel/GuestStatusView.js | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/www/manager6/panel/GuestStatusView.js b/www/manager6/panel/GuestStatusView.js
index ca2f03453..c773f252d 100644
--- a/www/manager6/panel/GuestStatusView.js
+++ b/www/manager6/panel/GuestStatusView.js
@@ -29,6 +29,7 @@ Ext.define('PVE.panel.GuestStatusView', {
 		success: ({ result }) => {
 		    view.down('#unprivileged').updateValue(
 			Proxmox.Utils.format_boolean(result.data.unprivileged));
+		    view.ostype = result.data.ostype;
 		},
 	    });
 	},
@@ -166,6 +167,22 @@ Ext.define('PVE.panel.GuestStatusView', {
 		+ ')';
 	}
 
-	me.setTitle(me.getRecordValue('name') + text);
+	let title = `<div class="left-aligned">${me.getRecordValue('name') + text}</div>`;
+
+	if (me.pveSelNode.data.type === 'lxc' && me.ostype && me.ostype !== 'unmanaged') {
+	    // Manual mappings for distros with special casing
+	    const namemap = {
+		'archlinux': 'Arch Linux',
+		'nixos': 'NixOS',
+		'opensuse': 'openSUSE',
+		'centos': 'CentOS',
+	    };
+
+	    const distro = namemap[me.ostype] ?? Ext.String.capitalize(me.ostype);
+	    title += `<div class="right-aligned">
+		<i class="fl-${me.ostype} fl-fw"></i>&nbsp;${distro}</div>`;
+	}
+
+	me.setTitle(title);
     },
 });
-- 
2.41.0

Re: [pve-devel] [PATCH font-logos/manager v2 0/5] fix #2435: lxc: show distro and privileged status in summary

[Christoph Heiss]

Ping - still applies cleanly on current master of both repositories.

On Wed, Jul 05, 2023 at 01:12:44PM +0200, Christoph Heiss wrote:
>
> This implements #2435 [0]. Show the unprivileged status in the summary
> panel, the distro logo and name in the title of the summary panel.
>
> Patch 1 & 2 fix two small typos in the `fonts-font-logos` package.
> Patch 3 then prepares the pveproxy to serve the required CSS and font
> files for the icon font we use [1] (packaged here [2]), and finally
> patch 4 & 5 finally wire everything up into the UI.
>
> N.B.: The `fonts-font-logos` package is not yet available through the
> repos yet (as far as I could see), so it must be built & installed
> locally for testing.
>
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=2435
> [1] https://github.com/Lukas-W/font-logos
> [2] https://git.proxmox.com/?p=fonts-font-logos.git;a=summary
>
> fonts-font-logos:
>
> Christoph Heiss (2):
>   d/install: fix typo in css install path
>   css: fix missing `@` for font-face rule
>
>  debian/install     | 2 +-
>  src/font-logos.css | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> pve-manager:
>
> Christoph Heiss (3):
>   pveproxy, ui, d/control: add font-logos
>   ui: GuestStatusView: show privileged status as new row
>   ui: GuestStatusView: show distro logo and name in summary header
>
>  PVE/Service/pveproxy.pm               |  2 ++
>  debian/control                        |  1 +
>  www/index.html.tpl                    |  1 +
>  www/manager6/panel/GuestStatusView.js | 51 ++++++++++++++++++++++++++-
>  4 files changed, 54 insertions(+), 1 deletion(-)
>
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>

Re: [pve-devel] [PATCH manager v2 4/5] ui: GuestStatusView: show privileged status as new row

[Dominik Csapak]

one small comment inline

On 7/5/23 13:12, Christoph Heiss wrote:
> As that info is not available through the store (which stores the
> status), it must be fetched separately.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>   www/manager6/panel/GuestStatusView.js | 32 +++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/www/manager6/panel/GuestStatusView.js b/www/manager6/panel/GuestStatusView.js
> index 8db1f492c..ca2f03453 100644
> --- a/www/manager6/panel/GuestStatusView.js
> +++ b/www/manager6/panel/GuestStatusView.js
> @@ -11,6 +11,29 @@ Ext.define('PVE.panel.GuestStatusView', {
>   	};
>       },
>   
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	init: view => {

while this works, it's a rather unusual use for us

normally we use the standard 'function(view) {}' syntax
the advantage of that is that the 'this' object then
is the controller

if someone wants to use the controller in the future and starts
using 'this' it won't work with the arrow syntax (it will be undefined)

> +	    if (view.pveSelNode.data.type !== 'lxc') {
> +		return;
> +	    }
> +
> +	    const nodename = view.pveSelNode.data.node;
> +	    const vmid = view.pveSelNode.data.vmid;
> +
> +	    Proxmox.Utils.API2Request({
> +		url: `/api2/extjs/nodes/${nodename}/lxc/${vmid}/config`,
> +		waitMsgTargetView: view,
> +		method: 'GET',
> +		success: ({ result }) => {
> +		    view.down('#unprivileged').updateValue(
> +			Proxmox.Utils.format_boolean(result.data.unprivileged));
> +		},
> +	    });
> +	},
> +    },
> +
>       layout: {
>   	type: 'vbox',
>   	align: 'stretch',
> @@ -58,6 +81,15 @@ Ext.define('PVE.panel.GuestStatusView', {
>   	    },
>   	    printBar: false,
>   	},
> +	{
> +	    itemId: 'unprivileged',
> +	    iconCls: 'fa fa-lock fa-fw',
> +	    title: gettext('Unprivileged'),
> +	    printBar: false,
> +	    cbind: {
> +		hidden: '{isQemu}',
> +	    },
> +	},
>   	{
>   	    xtype: 'box',
>   	    height: 15,

Re: [pve-devel] [PATCH manager v2 5/5] ui: GuestStatusView: show distro logo and name in summary header

[Dominik Csapak]

one small comment inline

On 7/5/23 13:12, Christoph Heiss wrote:
> It fits neatly there, is rather unintrusive and yet still visible at
> first sight. It also solves the problem of having to create a bigger
> row, so that the icon is still easily recognisable. At the default
> font-size of 13pt, this really wasn't the case.
> 
> Verified that each supported distro is present in the font and the name
> matches up and tested through all supported distros (including
> 'unmanaged').
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>   www/manager6/panel/GuestStatusView.js | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/panel/GuestStatusView.js b/www/manager6/panel/GuestStatusView.js
> index ca2f03453..c773f252d 100644
> --- a/www/manager6/panel/GuestStatusView.js
> +++ b/www/manager6/panel/GuestStatusView.js
> @@ -29,6 +29,7 @@ Ext.define('PVE.panel.GuestStatusView', {
>   		success: ({ result }) => {
>   		    view.down('#unprivileged').updateValue(
>   			Proxmox.Utils.format_boolean(result.data.unprivileged));
> +		    view.ostype = result.data.ostype;
>   		},
>   	    });
>   	},
> @@ -166,6 +167,22 @@ Ext.define('PVE.panel.GuestStatusView', {
>   		+ ')';
>   	}
>   
> -	me.setTitle(me.getRecordValue('name') + text);
> +	let title = `<div class="left-aligned">${me.getRecordValue('name') + text}</div>`;
> +
> +	if (me.pveSelNode.data.type === 'lxc' && me.ostype && me.ostype !== 'unmanaged') {
> +	    // Manual mappings for distros with special casing
> +	    const namemap = {
> +		'archlinux': 'Arch Linux',
> +		'nixos': 'NixOS',
> +		'opensuse': 'openSUSE',
> +		'centos': 'CentOS',
> +	    };
> +
> +	    const distro = namemap[me.ostype] ?? Ext.String.capitalize(me.ostype);
> +	    title += `<div class="right-aligned">
> +		<i class="fl-${me.ostype} fl-fw"></i>&nbsp;${distro}</div>`;
> +	}

while in practice it won't make a difference,
we probably want to wrap the distro in an Ext.htmlEncode

we don't allow arbitrary values via the api, but if i edit
the config it simply returns the value, e.g. having

   ostype: <b>test</b>

in the container config leads to bold text here

though i don't think this is a blocker for this series

> +
> +	me.setTitle(title);
>       },
>   });

Re: [pve-devel] [PATCH font-logos/manager v2 0/5] fix #2435: lxc: show distro and privileged status in summary

[Dominik Csapak]

the series looks mostly fine to me, i wrote two small
comments in the relevant patches, but both
are not super important to fix IMO

aside from that consider this series

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>

[pve-devel] applied-series: [PATCH font-logos/manager v2 0/5] fix #2435: lxc: show distro and privileged status in summary

[Thomas Lamprecht]

Am 20/10/2023 um 15:33 schrieb Dominik Csapak:
> the series looks mostly fine to me, i wrote two small
> comments in the relevant patches, but both
> are not super important to fix IMO
> 
> aside from that consider this series
> 
> Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
> Tested-by: Dominik Csapak <d.csapak@proxmox.com>

With your tags, and the review comments addressed and squashed into:

applied, thanks!