Re: [pbs-devel] [pve-devel] [PATCH widget-toolkit v3 4/6] toolkit: add markdown based NotesView and NotesEdit

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 04.03.22 12:32, Stefan Sterz wrote:
> move these from pve to the widget toolkit to be ablte to use them in
> pbs
> 

this is no 1:1 move but the commit messages fails to describe the changes
made nor the rationale.

> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
>  src/Makefile            |   2 +
>  src/panel/NotesView.js  | 155 ++++++++++++++++++++++++++++++++++++++++
>  src/window/NotesEdit.js |  38 ++++++++++
>  3 files changed, 195 insertions(+)
>  create mode 100644 src/panel/NotesView.js
>  create mode 100644 src/window/NotesEdit.js
> 
> diff --git a/src/Makefile b/src/Makefile
> index de34531..ae20947 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -65,6 +65,7 @@ JSSRC=					\
>  	panel/ACMEDomains.js		\
>  	panel/StatusView.js		\
>  	panel/TfaView.js		\
> +	panel/NotesView.js		\
>  	window/Edit.js			\
>  	window/PasswordEdit.js		\
>  	window/SafeDestroy.js		\
> @@ -86,6 +87,7 @@ JSSRC=					\
>  	window/AddWebauthn.js		\
>  	window/AddYubico.js		\
>  	window/TfaEdit.js		\
> +	window/NotesEdit.js		\
>  	node/APT.js			\
>  	node/APTRepositories.js		\
>  	node/NetworkEdit.js		\
> diff --git a/src/panel/NotesView.js b/src/panel/NotesView.js
> new file mode 100644
> index 0000000..df5e688
> --- /dev/null
> +++ b/src/panel/NotesView.js
> @@ -0,0 +1,155 @@
> +Ext.define('Proxmox.panel.NotesView', {
> +    extend: 'Ext.panel.Panel',
> +    xtype: 'pmxNotesView',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    title: gettext("Notes"),
> +    bodyPadding: 10,
> +    scrollable: true,
> +    animCollapse: false,
> +    maxLength: 64 * 1024,
> +
> +    cbindData: function(initalConfig) {
> +	let me = this;
> +
> +	if (me.node) {
> +	    me.url = `/api2/extjs/nodes/${me.node}/config`;
> +	    me.maxLength = 64 * 1022;

why factor 1022 ?! And why override it uncoditionally, leaving no control for the
caller?

also half of initialization logic now gets handled by cbind and half by initComponent,
why the split-approach? This could just be an `else` for the initComponent's if (!node)
clause.

> +	}
> +
> +	return {};
> +    },
> +
> +

^- extra whitespace
> +    run_editor: function() {
> +	let me = this;
> +	Ext.create('Proxmox.window.NotesEdit', {
> +	    url: me.url,
> +	    listeners: {
> +		destroy: () => me.load(),
> +	    },
> +	    autoShow: true,
> +	}).setMaxLength(me.maxLength);
> +    },
> +
> +    setNotes: function(value) {
> +	let me = this;
> +	var data = value || '';

use let for new code, could also just drop intermediate variable and make either the
caller fallback, or  use `value || ''` for the parse and !data for the setCollapsed.

> +
> +	let mdHtml = Proxmox.Markdown.parse(data);
> +	me.update(mdHtml);
> +
> +	if (me.collapsible && me.collapseMode === 'auto') {
> +	    me.setCollapsed(data === '');
> +	}
> +    },
> +
> +    load: function() {
> +	var me = this;
> +
> +	Proxmox.Utils.API2Request({
> +	    url: me.url,
> +	    waitMsgTarget: me,
> +	    failure: function(response, opts) {
> +		me.update(gettext('Error') + " " + response.htmlStatus);
> +		me.setCollapsed(false);
> +	    },
> +	    success: function(response, opts) {
> +		let text = response.result.data.description;

useless intermediate variable

> +		me.setNotes(text);

me.setNotes(response.result.data.description || '');

or alternatively:

success: ({ result }) => me.setNotes(result.data.description || ''),

> +	    },
> +	});
> +    },
> +
> +    listeners: {
> +	render: function(c) {
> +	    var me = this;
> +	    me.getEl().on('dblclick', me.run_editor, me);
> +	},
> +	afterlayout: function() {
> +	    let me = this;
> +	    if (me.collapsible && !me.getCollapsed() && me.collapseMode === 'always') {
> +		me.setCollapsed(true);
> +		me.collapseMode = ''; // only once, on initial load!
> +	    }
> +	},
> +    },
> +
> +    tools: [{
> +	type: 'gear',
> +	handler: function() {
> +	    this.up('panel').run_editor();
> +	},
> +    }],
> +
> +    tbar: {

no hard feelings on moving tbar from top of file here, but it makes slightly diff'ing harder.

> +	itemId: 'tbar',
> +	hidden: true,
> +	items: [
> +	    {
> +		text: gettext('Edit'),
> +		handler: function() {
> +		    this.up('panel').run_editor();

I'd keep the `let view = `

> +		},
> +	    },
> +	],
> +    },
> +
> +    initComponent: function() {
> +	const me = this;
> +	let type = '';
> +
> +	if (!me.node) {
> +	    if (me.pveSelNode.data.id === 'root') {
> +		me.url = '/api2/extjs/cluster/options';
> +		type = me.pveSelNode.data.type;
> +	    } else {
> +		const nodename = me.pveSelNode.data.node;
> +		type = me.pveSelNode.data.type;
> +
> +		if (!nodename) {
> +		    throw "no node name specified";
> +		}
> +
> +		if (!Ext.Array.contains(['node', 'qemu', 'lxc'], type)) {
> +		    throw 'invalid type specified';
> +		}
> +
> +		const vmid = me.pveSelNode.data.vmid;
> +
> +		if (!vmid && type !== 'node') {
> +		    throw "no VM ID specified";
> +		}
> +
> +		me.url = `/api2/extjs/nodes/${nodename}/`;
> +
> +		// add the type specific path if qemu/lxc and set the backend's maxLen
> +		if (type === 'qemu' || type === 'lxc') {
> +		    me.url += `${type}/${vmid}/`;
> +		    me.maxLength = 8 * 1024;
> +		}
> +
> +		me.url += 'config';
> +	    }
> +	}
> +
> +	me.callParent();
> +
> +	if (me.enableTbar === true || type === 'node' || type === '') { // '' is for datacenter


s/enableTbar/enableTBar/ and please also set the default option value on top, as
it's needlessly hard to make devs pull any possible config out from the code.

> +	    me.down('#tbar').setVisible(true);
> +	} else if (me.pveSelNode.data.template !== 1) {

use ?. for accessing properties that may not be defined, this can easily break if used in
a context where the if evaluates false.

> +	    me.setCollapsible(true);
> +	    me.collapseDirection = 'right';
> +
> +	    let sp = Ext.state.Manager.getProvider();
> +	    me.collapseMode = sp.get('guest-notes-collapse', 'never');
> +
> +	    if (me.collapseMode === 'auto') {
> +		me.setCollapsed(true);
> +	    }
> +	}
> +
> +
> +	me.load();
> +    },
> +});
> diff --git a/src/window/NotesEdit.js b/src/window/NotesEdit.js
> new file mode 100644
> index 0000000..ab5254d
> --- /dev/null
> +++ b/src/window/NotesEdit.js
> @@ -0,0 +1,38 @@
> +Ext.define('Proxmox.window.NotesEdit', {
> +    extend: 'Proxmox.window.Edit',
> +
> +    title: gettext('Notes'),
> +    onlineHelp: 'markdown_basics',

it could be better to let the caller set the onlineHelp, so that the reference
existence is correctly checked in pve-manage/proxmox-backup.

> +
> +    width: 800,
> +    height: '600px',
> +
> +    resizable: true,
> +    layout: 'fit',
> +
> +    autoLoad: true,
> +    defaultButton: undefined,
> +
> +    setMaxLength: function(maxLength) {
> +	let me = this;
> +
> +	let area = me.down('textarea[name="description"]');
> +	area.maxLength = maxLength;
> +	area.validate();
> +
> +	return me;
> +    },
> +
> +    items: {
> +	xtype: 'textarea',
> +	name: 'description',
> +	height: '100%',
> +	value: '',
> +	hideLabel: true,
> +	emptyText: gettext('You can use Markdown for rich text formatting.'),
> +	fieldStyle: {
> +	    'white-space': 'pre-wrap',
> +	    'font-family': 'monospace',
> +	},
> +    },
> +});