Re: [pve-devel] [PATCH widget-toolkit v2] fix #5836: ui: translate systemd states in ServiceView

[Dominik Csapak <d.csapak@proxmox.com>]

comments inline:

On 11/11/24 11:37, Timothy Nicholson wrote:
> Signed-off-by: Timothy Nicholson <t.nicholson@proxmox.com>
> ---
> changes since v1 [0]:
> - pass literal strings to gettext
> - changed switch-case to include common systemd states
> 
> [0]: https://lore.proxmox.com/pve-devel/20241108125254.147646-1-t.nicholson@proxmox.com/
> 
>   src/Utils.js            | 25 +++++++++++++++++++++++++
>   src/node/ServiceView.js | 14 ++++++--------
>   2 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/src/Utils.js b/src/Utils.js
> index 7dd034a..c189f98 100644
> --- a/src/Utils.js
> +++ b/src/Utils.js
> @@ -305,6 +305,31 @@ utilities: {
>   	return Ext.htmlEncode(username);
>       },
>   
> +    renderSystemdState: function(state) {
> +	switch (state) {
> +	    case 'enabled':
> +		return gettext('enabled');
> +	    case 'disabled':
> +		return gettext('disabled');
> +	    case 'running':
> +		return gettext('running');
> +	    case 'dead':
> +		return gettext('dead');
> +	    case 'not-found':
> +		return gettext('not installed');
> +	    case 'static':
> +		return gettext('static');
> +	    case 'reload':
> +		return gettext('reload');
> +	    case 'start':
> +		return gettext('start');
> +	    case 'stop':
> +		return gettext('stop');
> +	    default:
> +		return state;
> +	}
> +    },
> +

instead of making this a switch statement, i'd rather do what we do elsewhere
and simply use an object like this:

---8<---
systemdStates: {
     'enabled': gettext('enabled'),
     'disabled': getttext('disabled'),
     ...
}
--->8---

when we could simply use something like
   let text = Proxmox.Utils.systemdStates[state] ?? state;


also where do the states 'start'/'stop'/etc. come from?
when is a service in state 'start' instead of e.g. 'running' ?

'start'/'stop'/etc. as state does not make much sense to me?

*if* these states actually exist (e.g. when it's being started)
i'd rather use better terms in the gettext like 'started'/'starting' or 'stopped'/'stopping', etc.
does that make sense  to you?


>       getStoredAuth: function() {
>   	let storedAuth = JSON.parse(window.localStorage.getItem('ProxmoxUser'));
>   	return storedAuth || {};
> diff --git a/src/node/ServiceView.js b/src/node/ServiceView.js
> index 19cfc18..c519da1 100644
> --- a/src/node/ServiceView.js
> +++ b/src/node/ServiceView.js
> @@ -180,14 +180,8 @@ Ext.define('Proxmox.node.ServiceView', {
>   		    sortable: true,
>   		    dataIndex: 'state',
>   		    renderer: (value, meta, rec) => {
> -			const unitState = rec.get('unit-state');
> -			if (unitState === 'masked') {
> -			    return gettext('disabled');
> -			} else if (unitState === 'not-found') {
> -			    return gettext('not installed');
> -			} else {
> -			    return value;
> -			}
> +			const state = rec.get('state');
> +			return Proxmox.Utils.renderSystemdState(state);
>   		    },
>   		},
>   		{
> @@ -203,6 +197,10 @@ Ext.define('Proxmox.node.ServiceView', {
>   		    sortable: true,
>   		    hidden: !Ext.Array.contains(['PVEAuthCookie', 'PBSAuthCookie'], Proxmox?.Setup?.auth_cookie_name),
>   		    dataIndex: 'unit-state',
> +		    renderer: (value, meta, rec) => {
> +			const unitState = rec.get('unit-state');
> +			return Proxmox.Utils.renderSystemdState(unitState);
> +		    }
>   		},
>   		{
>   		    header: gettext('Description'),



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel