[pbs-devel] [PATCH proxmox-backup 0/5] fix #3067: add notes functionality to webui

[pbs-devel] [PATCH proxmox-backup 0/5] fix #3067: add notes functionality to webui

[Stefan Sterz]

This series adds support for comments/notes to a PBS node similar to
PVE's datacenter or node notes. It's split in two parts, the first two
commits add support for a single line comment, similar to the comments
already available for datastores. The next three commits refactor this
into Markdown-based multi-line comments and add an additional tab to
the UI to provide more space. If prefered, I can squash the commits.

Stefan Sterz (5):
  fix #3067: api: add support for a comment field in node.cfg
  fix #3067: pbs ui: add support for a notes field in the dashboard
  fix #3067: api: add multi-line comments to node.cfg
  fix #3607: ui: make dashboard notes markdown capable
  fix #3607: ui: add a separate notes view for longer markdown notes

 pbs-api-types/src/lib.rs          |   9 ++
 src/api2/node/config.rs           |   4 +
 src/config/node.rs                |  11 ++-
 src/tools/config.rs               |  62 ++++++++++++
 www/Dashboard.js                  | 110 +++++++++++++---------
 www/Makefile                      |   4 +-
 www/NavigationTree.js             |   6 ++
 www/NodeNotes.js                  |  22 +++++
 www/datastore/Summary.js          |   6 +-
 www/panel/MarkdownNotes.js        | 151 ++++++++++++++++++++++++++++++
 www/{datastore => panel}/Notes.js |  19 +++-
 11 files changed, 351 insertions(+), 53 deletions(-)
 create mode 100644 www/NodeNotes.js
 create mode 100644 www/panel/MarkdownNotes.js
 rename www/{datastore => panel}/Notes.js (81%)

-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 1/5] fix #3067: api: add support for a comment field in node.cfg

[Stefan Sterz]

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 src/api2/node/config.rs |  4 ++++
 src/config/node.rs      | 11 ++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/api2/node/config.rs b/src/api2/node/config.rs
index 0a119354..77fe69fe 100644
--- a/src/api2/node/config.rs
+++ b/src/api2/node/config.rs
@@ -64,6 +64,8 @@ pub enum DeletableProperty {
     ciphers_tls_1_2,
     /// Delete the default-lang property.
     default_lang,
+    /// Delete any comment
+    comment,
 }
 
 #[api(
@@ -124,6 +126,7 @@ pub fn update_node_config(
                 DeletableProperty::ciphers_tls_1_3 => { config.ciphers_tls_1_3 = None; },
                 DeletableProperty::ciphers_tls_1_2 => { config.ciphers_tls_1_2 = None; },
                 DeletableProperty::default_lang => { config.default_lang = None; },
+                DeletableProperty::comment => { config.comment = None; },
             }
         }
     }
@@ -139,6 +142,7 @@ pub fn update_node_config(
     if update.ciphers_tls_1_3.is_some() { config.ciphers_tls_1_3 = update.ciphers_tls_1_3; }
     if update.ciphers_tls_1_2.is_some() { config.ciphers_tls_1_2 = update.ciphers_tls_1_2; }
     if update.default_lang.is_some() { config.default_lang = update.default_lang; }
+    if update.comment.is_some() { config.comment = update.comment; }
 
     crate::config::node::save_config(&config)?;
 
diff --git a/src/config/node.rs b/src/config/node.rs
index 0ba87450..9ca44a52 100644
--- a/src/config/node.rs
+++ b/src/config/node.rs
@@ -8,7 +8,8 @@ use proxmox_schema::{api, ApiStringFormat, ApiType, Updater};
 
 use proxmox_http::ProxyConfig;
 
-use pbs_api_types::{EMAIL_SCHEMA, OPENSSL_CIPHERS_TLS_1_2_SCHEMA, OPENSSL_CIPHERS_TLS_1_3_SCHEMA};
+use pbs_api_types::{EMAIL_SCHEMA, OPENSSL_CIPHERS_TLS_1_2_SCHEMA, OPENSSL_CIPHERS_TLS_1_3_SCHEMA,
+    SINGLE_LINE_COMMENT_SCHEMA};
 use pbs_buildcfg::configdir;
 use pbs_config::{open_backup_lockfile, BackupLockGuard};
 
@@ -167,6 +168,10 @@ pub enum Translation {
         "default-lang" : {
             schema: Translation::API_SCHEMA,
             optional: true,
+        },
+        "comment" : {
+            optional: true,
+            schema: SINGLE_LINE_COMMENT_SCHEMA,
         }
     },
 )]
@@ -210,6 +215,10 @@ pub struct NodeConfig {
     /// Default language used in the GUI
     #[serde(skip_serializing_if = "Option::is_none")]
     pub default_lang: Option<String>,
+
+    /// Dashboard comment
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub comment: Option<String>,
 }
 
 impl NodeConfig {
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 2/5] fix #3067: pbs ui: add support for a notes field in the dashboard

[Stefan Sterz]

adds a panel to the dashboard displaying a comment similar to the
comments panel in a datastore summary

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 www/Dashboard.js                  | 110 ++++++++++++++++++------------
 www/Makefile                      |   2 +-
 www/datastore/Summary.js          |   6 +-
 www/{datastore => panel}/Notes.js |  19 +++++-
 4 files changed, 85 insertions(+), 52 deletions(-)
 rename www/{datastore => panel}/Notes.js (81%)

diff --git a/www/Dashboard.js b/www/Dashboard.js
index 70c2305b..a78ad375 100644
--- a/www/Dashboard.js
+++ b/www/Dashboard.js
@@ -122,7 +122,7 @@ Ext.define('PBS.Dashboard', {
 		if (key !== 'summarycolumns') {
 		    return;
 		}
-		Proxmox.Utils.updateColumns(view);
+		Proxmox.Utils.updateColumnWidth(view);
 	    });
 	},
     },
@@ -192,26 +192,14 @@ Ext.define('PBS.Dashboard', {
 
     listeners: {
 	resize: function(panel) {
-	    Proxmox.Utils.updateColumns(panel);
+	    panel.query('>').forEach(c => Proxmox.Utils.updateColumnWidth(c));
 	},
     },
 
     title: gettext('Dashboard'),
 
-    layout: {
-	type: 'column',
-    },
-
     bodyPadding: '20 0 0 20',
 
-    minWidth: 700,
-
-    defaults: {
-	columnWidth: 0.49,
-	xtype: 'panel',
-	margin: '0 20 20 0',
-    },
-
     tools: [
 	{
 	    type: 'gear',
@@ -224,42 +212,74 @@ Ext.define('PBS.Dashboard', {
 
     items: [
 	{
-	    xtype: 'pbsNodeInfoPanel',
-	    reference: 'nodeInfo',
-	    height: 280,
-	},
-	{
-	    xtype: 'pbsDatastoresStatistics',
+	    xtype: 'container',
+	    layout: {
+		type: 'hbox',
+		align: 'stretch',
+	    },
+	    margin: '0 0 20 0',
+	    padding: '0 20 0 0',
 	    height: 280,
-	},
-	{
-	    xtype: 'pbsLongestTasks',
-	    bind: {
-		title: gettext('Longest Tasks') + ' (' +
-		Ext.String.format(gettext('{0} days'), '{days}') + ')',
+	    defaults: {
+		flex: 1,
 	    },
-	    reference: 'longesttasks',
-	    height: 250,
-	},
-	{
-	    xtype: 'pbsRunningTasks',
-	    height: 250,
-	},
-	{
-	    bind: {
-		title: gettext('Task Summary') + ' (' +
-		Ext.String.format(gettext('{0} days'), '{days}') + ')',
+	    minWidth: 700,
+	    items: [{
+		xtype: 'pbsNodeInfoPanel',
+		reference: 'nodeInfo',
+		margin: '0 20 0 0',
 	    },
-	    xtype: 'pbsTaskSummary',
-	    height: 200,
-	    reference: 'tasksummary',
+	    {
+		xtype: 'pbsNotes',
+		reference: 'nodeNotes',
+		node: 'localhost',
+		loadOnInit: true,
+	    }],
 	},
 	{
-	    iconCls: 'fa fa-ticket',
-	    title: 'Subscription',
-	    height: 200,
-	    reference: 'subscription',
-	    xtype: 'pbsSubscriptionInfo',
+	    xtype: 'container',
+	    layout: {
+		type: 'column',
+	    },
+	    minWidth: 700,
+	    defaults: {
+		columnWidth: 0.5,
+		xtype: 'panel',
+		margin: '0 20 20 0',
+	    },
+	    items: [{
+		xtype: 'pbsDatastoresStatistics',
+		height: 250,
+	    },
+	    {
+		xtype: 'pbsLongestTasks',
+		bind: {
+		    title: gettext('Longest Tasks') + ' (' +
+		    Ext.String.format(gettext('{0} days'), '{days}') + ')',
+		},
+		reference: 'longesttasks',
+		height: 250,
+	    },
+	    {
+		xtype: 'pbsRunningTasks',
+		height: 250,
+	    },
+	    {
+		bind: {
+		    title: gettext('Task Summary') + ' (' +
+		    Ext.String.format(gettext('{0} days'), '{days}') + ')',
+		},
+		xtype: 'pbsTaskSummary',
+		height: 250,
+		reference: 'tasksummary',
+	    },
+	    {
+		iconCls: 'fa fa-ticket',
+		title: 'Subscription',
+		height: 200,
+		reference: 'subscription',
+		xtype: 'pbsSubscriptionInfo',
+	    }],
 	},
     ],
 });
diff --git a/www/Makefile b/www/Makefile
index 455fbeec..636d4a57 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -81,6 +81,7 @@ JSSRC=							\
 	panel/StorageAndDisks.js			\
 	panel/UsageChart.js				\
 	panel/NodeInfo.js				\
+	panel/Notes.js				    \
 	ZFSList.js					\
 	DirectoryList.js				\
 	LoginView.js					\
@@ -88,7 +89,6 @@ JSSRC=							\
 	SystemConfiguration.js				\
 	Subscription.js					\
 	datastore/Summary.js				\
-	datastore/Notes.js				\
 	datastore/PruneAndGC.js				\
 	datastore/Prune.js				\
 	datastore/Content.js				\
diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
index c3769257..d11646f0 100644
--- a/www/datastore/Summary.js
+++ b/www/datastore/Summary.js
@@ -206,7 +206,7 @@ Ext.define('PBS.DataStoreSummary', {
 		    },
 		},
 		{
-		    xtype: 'pbsDataStoreNotes',
+		    xtype: 'pbsNotes',
 		    flex: 1,
 		    cbind: {
 			datastore: '{datastore}',
@@ -278,14 +278,14 @@ Ext.define('PBS.DataStoreSummary', {
 	    success: function(response) {
 		let path = Ext.htmlEncode(response.result.data.path);
 		me.down('pbsDataStoreInfo').setTitle(`${me.datastore} (${path})`);
-		me.down('pbsDataStoreNotes').setNotes(response.result.data.comment);
+		me.down('pbsNotes').setNotes(response.result.data.comment);
 	    },
 	    failure: function(response) {
 		// fallback if e.g. we have no permissions to the config
 		let rec = Ext.getStore('pbs-datastore-list')
 		    .findRecord('store', me.datastore, 0, false, true, true);
 		if (rec) {
-		    me.down('pbsDataStoreNotes').setNotes(rec.data.comment || "");
+		    me.down('pbsNotes').setNotes(rec.data.comment || "");
 		}
 	    },
 	});
diff --git a/www/datastore/Notes.js b/www/panel/Notes.js
similarity index 81%
rename from www/datastore/Notes.js
rename to www/panel/Notes.js
index 2928b7ec..4128dd8b 100644
--- a/www/datastore/Notes.js
+++ b/www/panel/Notes.js
@@ -1,6 +1,6 @@
-Ext.define('PBS.DataStoreNotes', {
+Ext.define('PBS.panel.Notes', {
     extend: 'Ext.panel.Panel',
-    xtype: 'pbsDataStoreNotes',
+    xtype: 'pbsNotes',
     mixins: ['Proxmox.Mixin.CBind'],
 
     title: gettext("Comment"),
@@ -11,7 +11,16 @@ Ext.define('PBS.DataStoreNotes', {
 
     cbindData: function(initalConfig) {
 	let me = this;
-	me.url = `/api2/extjs/config/datastore/${me.datastore}`;
+
+	if (('node' in me && 'datastore' in me) ||
+	    (!('node' in me) && !('datastore' in me))) {
+	    throw 'either both a node and a datastore were given or neither. only provide one.';
+	} else if ('node' in me) {
+	    me.url = `/api2/extjs/nodes/${me.node}/config`;
+	} else {
+	    me.url = `/api2/extjs/config/datastore/${me.datastore}`;
+	}
+
 	return { };
     },
 
@@ -97,6 +106,10 @@ Ext.define('PBS.DataStoreNotes', {
 	let sp = Ext.state.Manager.getProvider();
 	me.collapseMode = sp.get('notes-collapse', 'never');
 
+	if (me.loadOnInit === true) {
+	    me.load();
+	}
+
 	if (me.collapseMode === 'auto') {
 	    me.setCollapsed(true);
 	}
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 3/5] fix #3067: api: add multi-line comments to node.cfg

[Stefan Sterz]

add support for multiline comments to node.cfg, similar to how pve
handles multi-line comments

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 pbs-api-types/src/lib.rs |  9 ++++++
 src/config/node.rs       |  4 +--
 src/tools/config.rs      | 62 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index 754e7b22..3892980d 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -137,6 +137,8 @@ const_regex! {
 
     pub SINGLE_LINE_COMMENT_REGEX = r"^[[:^cntrl:]]*$";
 
+    pub MULTI_LINE_COMMENT_REGEX = r"(?m)^([[:^cntrl:]]*)\s*$";
+
     pub BACKUP_REPO_URL_REGEX = concat!(
         r"^^(?:(?:(",
         USER_ID_REGEX_STR!(), "|", APITOKEN_ID_REGEX_STR!(),
@@ -273,6 +275,13 @@ pub const SINGLE_LINE_COMMENT_SCHEMA: Schema = StringSchema::new("Comment (singl
     .format(&SINGLE_LINE_COMMENT_FORMAT)
     .schema();
 
+pub const MULTI_LINE_COMMENT_FORMAT: ApiStringFormat =
+    ApiStringFormat::Pattern(&MULTI_LINE_COMMENT_REGEX);
+
+pub const MULTI_LINE_COMMENT_SCHEMA: Schema = StringSchema::new("Comment (multiple lines).")
+    .format(&MULTI_LINE_COMMENT_FORMAT)
+    .schema();
+
 pub const SUBSCRIPTION_KEY_SCHEMA: Schema = StringSchema::new("Proxmox Backup Server subscription key.")
     .format(&SUBSCRIPTION_KEY_FORMAT)
     .min_length(15)
diff --git a/src/config/node.rs b/src/config/node.rs
index 9ca44a52..bb915f94 100644
--- a/src/config/node.rs
+++ b/src/config/node.rs
@@ -9,7 +9,7 @@ use proxmox_schema::{api, ApiStringFormat, ApiType, Updater};
 use proxmox_http::ProxyConfig;
 
 use pbs_api_types::{EMAIL_SCHEMA, OPENSSL_CIPHERS_TLS_1_2_SCHEMA, OPENSSL_CIPHERS_TLS_1_3_SCHEMA,
-    SINGLE_LINE_COMMENT_SCHEMA};
+    MULTI_LINE_COMMENT_SCHEMA};
 use pbs_buildcfg::configdir;
 use pbs_config::{open_backup_lockfile, BackupLockGuard};
 
@@ -171,7 +171,7 @@ pub enum Translation {
         },
         "comment" : {
             optional: true,
-            schema: SINGLE_LINE_COMMENT_SCHEMA,
+            schema: MULTI_LINE_COMMENT_SCHEMA,
         }
     },
 )]
diff --git a/src/tools/config.rs b/src/tools/config.rs
index f666a8ab..738ab541 100644
--- a/src/tools/config.rs
+++ b/src/tools/config.rs
@@ -32,6 +32,20 @@ pub fn value_from_str(input: &str, schema: &'static Schema) -> Result<Value, Err
 
     let mut config = Object::new();
 
+    // parse first n lines starting with '#' as multi-line comment
+    let comment = input.lines()
+        .take_while(|l| l.starts_with('#'))
+        .map(|l| {
+            let mut ch = l.chars();
+            ch.next();
+            ch.as_str()
+        })
+        .fold(String::new(), |acc, l| acc + l + "\n");
+
+    if !comment.is_empty() {
+        config.insert("comment".to_string(), Value::String(comment));
+    }
+
     for (lineno, line) in input.lines().enumerate() {
         let line = line.trim();
         if line.starts_with('#') || line.is_empty() {
@@ -133,10 +147,23 @@ pub fn value_to_bytes(value: &Value, schema: &'static Schema) -> Result<Vec<u8>,
 
 /// Note: the object must have already been verified at this point.
 fn object_to_writer(output: &mut dyn Write, object: &Object) -> Result<(), Error> {
+    // special key `comment` for multi-line notes
+    if object.contains_key("comment") {
+        let comment = match object.get("comment") {
+            Some(Value::String(v)) => v,
+            _ => bail!("only strings can be comments"),
+        };
+
+        for lines in comment.lines() {
+            writeln!(output, "#{}", lines)?;
+        }
+    }
+
     for (key, value) in object.iter() {
         match value {
             Value::Null => continue, // delete this entry
             Value::Bool(v) => writeln!(output, "{}: {}", key, v)?,
+            Value::String(_) if key == "comment" => continue, // skip comment as we handle it above
             Value::String(v) => {
                 if v.as_bytes().contains(&b'\n') {
                     bail!("value for {} contains newlines", key);
@@ -172,3 +199,38 @@ fn test() {
 
     assert_eq!(config, NODE_CONFIG.as_bytes());
 }
+
+#[test]
+fn test_with_comment() {
+    use proxmox_schema::ApiType;
+
+    // let's just reuse some schema we actually have available:
+    use crate::config::node::NodeConfig;
+
+    const NODE_INPUT: &str = "\
+        #this should\n\
+        #be included\n\
+        acme: account=pebble\n\
+        # this should not\n\
+        acmedomain0: test1.invalid.local,plugin=power\n\
+        acmedomain1: test2.invalid.local\n\
+    ";
+
+    const NODE_OUTPUT: &str = "\
+        #this should\n\
+        #be included\n\
+        acme: account=pebble\n\
+        acmedomain0: test1.invalid.local,plugin=power\n\
+        acmedomain1: test2.invalid.local\n\
+    ";
+
+    let data: NodeConfig = from_str(NODE_INPUT, &NodeConfig::API_SCHEMA)
+        .expect("failed to parse multi-line notes node config");
+
+    println!("{}", data.comment.as_ref().expect("no comment was parsed"));
+
+    let config = to_bytes(&data, &NodeConfig::API_SCHEMA)
+        .expect("failed to serialize multi-line notes node config");
+
+    assert_eq!(config, NODE_OUTPUT.as_bytes());
+}
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 4/5] fix #3607: ui: make dashboard notes markdown capable

[Stefan Sterz]

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 www/Dashboard.js           |   2 +-
 www/Makefile               |   3 +-
 www/panel/MarkdownNotes.js | 134 +++++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+), 2 deletions(-)
 create mode 100644 www/panel/MarkdownNotes.js

diff --git a/www/Dashboard.js b/www/Dashboard.js
index a78ad375..a76660ff 100644
--- a/www/Dashboard.js
+++ b/www/Dashboard.js
@@ -230,7 +230,7 @@ Ext.define('PBS.Dashboard', {
 		margin: '0 20 0 0',
 	    },
 	    {
-		xtype: 'pbsNotes',
+		xtype: 'pbsMarkdownNotes',
 		reference: 'nodeNotes',
 		node: 'localhost',
 		loadOnInit: true,
diff --git a/www/Makefile b/www/Makefile
index 636d4a57..2d55d39d 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -81,7 +81,8 @@ JSSRC=							\
 	panel/StorageAndDisks.js			\
 	panel/UsageChart.js				\
 	panel/NodeInfo.js				\
-	panel/Notes.js				    \
+	panel/Notes.js				        \
+	panel/MarkdownNotes.js			        \
 	ZFSList.js					\
 	DirectoryList.js				\
 	LoginView.js					\
diff --git a/www/panel/MarkdownNotes.js b/www/panel/MarkdownNotes.js
new file mode 100644
index 00000000..83119d36
--- /dev/null
+++ b/www/panel/MarkdownNotes.js
@@ -0,0 +1,134 @@
+Ext.define('PBS.panel.MarkdownNotes', {
+    extend: 'Ext.panel.Panel',
+    xtype: 'pbsMarkdownNotes',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    title: gettext("Notes"),
+    bodyPadding: 10,
+    scrollable: true,
+    animCollapse: false,
+    maxLength: 64*1022,
+
+    cbindData: function(initalConfig) {
+	let me = this;
+
+	if (('node' in me && 'datastore' in me) ||
+	    (!('node' in me) && !('datastore' in me))) {
+	    throw 'either both a node and a datastore were given or neither. please provide one.';
+	} else if ('node' in me) {
+	    me.url = `/api2/extjs/nodes/${me.node}/config`;
+	} else {
+	    me.url = `/api2/extjs/config/datastore/${me.datastore}`;
+	}
+
+	return {};
+    },
+
+    run_editor: function() {
+	let me = this;
+	let win = Ext.create('Proxmox.window.Edit', {
+	    title: gettext('Notes'),
+	    width: 800,
+	    height: 600,
+	    resizable: true,
+	    layout: 'fit',
+	    defaultButton: undefined,
+	    setMaxLength: function(maxLength) {
+		let area = win.down('textarea[name="comment"]');
+		area.maxLength = maxLength;
+		area.validate();
+
+		return me;
+	    },
+	    items: {
+		xtype: 'textarea',
+		name: 'comment',
+		height: '100%',
+		value: '',
+		hideLabel: true,
+		emptyText: gettext('You can use Markdown for rich text formatting.'),
+		fieldStyle: {
+		    'white-space': 'pre-wrap',
+		    'font-family': 'monospace',
+		},
+	    },
+	    url: me.url,
+	    listeners: {
+		destroy: function() {
+		    me.load();
+		},
+	    },
+	}).show();
+	win.setMaxLength(me.maxLength);
+	win.load();
+    },
+
+    setNotes: function(value) {
+	let me = this;
+	var data = value || '';
+
+	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) {
+		Ext.Msg.alert(gettext('Error'), response.htmlStatus);
+		me.setCollapsed(false);
+	    },
+	    success: function(response, opts) {
+		me.setNotes(response.result.data.comment);
+	    },
+	});
+    },
+
+    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();
+	},
+    }],
+
+    collapsible: true,
+    collapseDirection: 'right',
+
+    initComponent: function() {
+	var me = this;
+
+	me.callParent();
+
+	let sp = Ext.state.Manager.getProvider();
+	me.collapseMode = sp.get('notes-collapse', 'never');
+
+	if (me.loadOnInit === true) {
+	    me.load();
+	}
+
+	if (me.collapseMode === 'auto') {
+	    me.setCollapsed(true);
+	}
+    },
+});
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 5/5] fix #3607: ui: add a separate notes view for longer markdown notes

[Stefan Sterz]

since markdown notes might be rather long having only the small panel
in the dashboard might not be sufficient. this commit adds a tab
similar to pve's datacenter or node notes.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 www/Makefile               |  1 +
 www/NavigationTree.js      |  6 ++++++
 www/NodeNotes.js           | 22 ++++++++++++++++++++++
 www/panel/MarkdownNotes.js | 33 +++++++++++++++++++++++++--------
 4 files changed, 54 insertions(+), 8 deletions(-)
 create mode 100644 www/NodeNotes.js

diff --git a/www/Makefile b/www/Makefile
index 2d55d39d..f1c0f8bb 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -99,6 +99,7 @@ JSSRC=							\
 	datastore/DataStoreList.js			\
 	ServerStatus.js					\
 	ServerAdministration.js				\
+	NodeNotes.js				        \
 	Dashboard.js					\
 	${TAPE_UI_FILES}				\
 	NavigationTree.js				\
diff --git a/www/NavigationTree.js b/www/NavigationTree.js
index 576d05ab..916582ef 100644
--- a/www/NavigationTree.js
+++ b/www/NavigationTree.js
@@ -32,6 +32,12 @@ Ext.define('PBS.store.NavigationStore', {
 		path: 'pbsDashboard',
 		leaf: true,
 	    },
+	    {
+		text: gettext('Notes'),
+		iconCls: 'fa fa-sticky-note-o',
+		path: 'pbsNodeNotes',
+		leaf: true,
+	    },
 	    {
 		text: gettext('Configuration'),
 		iconCls: 'fa fa-gears',
diff --git a/www/NodeNotes.js b/www/NodeNotes.js
new file mode 100644
index 00000000..9a0fa00c
--- /dev/null
+++ b/www/NodeNotes.js
@@ -0,0 +1,22 @@
+Ext.define('PBS.NodeNotes', {
+    extend: 'Ext.panel.Panel',
+    xtype: 'pbsNodeNotes',
+
+    scrollable: true,
+    layout: 'fit',
+
+    items: [
+	{
+	    xtype: 'container',
+	    layout: 'fit',
+	    items: [{
+		xtype: 'pbsMarkdownNotes',
+		tools: false,
+		border: false,
+		node: 'localhost',
+		loadOnInit: true,
+		enableTbar: true,
+	    }],
+	},
+    ],
+});
diff --git a/www/panel/MarkdownNotes.js b/www/panel/MarkdownNotes.js
index 83119d36..f522cdfd 100644
--- a/www/panel/MarkdownNotes.js
+++ b/www/panel/MarkdownNotes.js
@@ -112,23 +112,40 @@ Ext.define('PBS.panel.MarkdownNotes', {
 	},
     }],
 
-    collapsible: true,
-    collapseDirection: 'right',
+    tbar: {
+	itemId: 'tbar',
+	hidden: true,
+	items: [
+	    {
+		text: gettext('Edit'),
+		handler: function() {
+		    this.up('panel').run_editor();
+		},
+	    },
+	],
+    },
 
     initComponent: function() {
 	var me = this;
 
 	me.callParent();
 
-	let sp = Ext.state.Manager.getProvider();
-	me.collapseMode = sp.get('notes-collapse', 'never');
+	if (me.enableTbar === true) {
+	    me.down('#tbar').setVisible(true);
+	} else {
+	    me.setCollapsible(true);
+	    me.collapseDirection = 'right';
 
-	if (me.loadOnInit === true) {
-	    me.load();
+	    let sp = Ext.state.Manager.getProvider();
+	    me.collapseMode = sp.get('notes-collapse', 'never');
+
+	    if (me.collapseMode === 'auto') {
+		me.setCollapsed(true);
+	    }
 	}
 
-	if (me.collapseMode === 'auto') {
-	    me.setCollapsed(true);
+	if (me.loadOnInit === true) {
+	    me.load();
 	}
     },
 });
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup 3/5] fix #3067: api: add multi-line comments to node.cfg

[Wolfgang Bumiller]

some comments inline

On Tue, Feb 22, 2022 at 12:25:54PM +0100, Stefan Sterz wrote:
> add support for multiline comments to node.cfg, similar to how pve
> handles multi-line comments
> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
>  pbs-api-types/src/lib.rs |  9 ++++++
>  src/config/node.rs       |  4 +--
>  src/tools/config.rs      | 62 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
> index 754e7b22..3892980d 100644
> --- a/pbs-api-types/src/lib.rs
> +++ b/pbs-api-types/src/lib.rs
> @@ -137,6 +137,8 @@ const_regex! {
>  
>      pub SINGLE_LINE_COMMENT_REGEX = r"^[[:^cntrl:]]*$";
>  
> +    pub MULTI_LINE_COMMENT_REGEX = r"(?m)^([[:^cntrl:]]*)\s*$";

I don't think the trailing `\s*` is necessary?

> +
>      pub BACKUP_REPO_URL_REGEX = concat!(
>          r"^^(?:(?:(",
>          USER_ID_REGEX_STR!(), "|", APITOKEN_ID_REGEX_STR!(),
> (...)
> diff --git a/src/config/node.rs b/src/config/node.rs
> index 9ca44a52..bb915f94 100644
> --- a/src/config/node.rs
> +++ b/src/config/node.rs
> @@ -9,7 +9,7 @@ use proxmox_schema::{api, ApiStringFormat, ApiType, Updater};
>  use proxmox_http::ProxyConfig;
>  
>  use pbs_api_types::{EMAIL_SCHEMA, OPENSSL_CIPHERS_TLS_1_2_SCHEMA, OPENSSL_CIPHERS_TLS_1_3_SCHEMA,
> -    SINGLE_LINE_COMMENT_SCHEMA};
> +    MULTI_LINE_COMMENT_SCHEMA};

Please check how rustfmt would deal with the above `use` statement ;-)

>  use pbs_buildcfg::configdir;
>  use pbs_config::{open_backup_lockfile, BackupLockGuard};
>  
> (...)
> diff --git a/src/tools/config.rs b/src/tools/config.rs
> index f666a8ab..738ab541 100644
> --- a/src/tools/config.rs
> +++ b/src/tools/config.rs
> @@ -32,6 +32,20 @@ pub fn value_from_str(input: &str, schema: &'static Schema) -> Result<Value, Err
>  
>      let mut config = Object::new();
>  
> +    // parse first n lines starting with '#' as multi-line comment
> +    let comment = input.lines()
> +        .take_while(|l| l.starts_with('#'))
> +        .map(|l| {
> +            let mut ch = l.chars();
> +            ch.next();
> +            ch.as_str()

^ The `take_while` ensures `l` starts with '#', so you could just use 

    .map(|l| &l[1..])

Alternatively, since 1.57 (and we're at 1.58 now), you could also
combine the `.take_while` and `.map` into:

    .map_while(|l| l.strip_prefix("#"))

However...

> +        })
> +        .fold(String::new(), |acc, l| acc + l + "\n");
> +
> +    if !comment.is_empty() {
> +        config.insert("comment".to_string(), Value::String(comment));
> +    }
> +
>      for (lineno, line) in input.lines().enumerate() {

... here we're starting over, so maybe we should refactor this a little.
Eg. we could use a `.lines().enumerate().peekable()` iterator:

    let mut lines = input.lines().enumerate().peekable();
    let mut comments = String::new();
    while let Some((_, line)) = iter.next_if(|(_, line)| line.starts_with('#') {
        comments.push_str(&line[1..]);
        comments.push('\n');
    }

    for (lineno, line) in lines {
    <...>

>          let line = line.trim();
>          if line.starts_with('#') || line.is_empty() {
> @@ -133,10 +147,23 @@ pub fn value_to_bytes(value: &Value, schema: &'static Schema) -> Result<Vec<u8>,
>  
>  /// Note: the object must have already been verified at this point.
>  fn object_to_writer(output: &mut dyn Write, object: &Object) -> Result<(), Error> {
> +    // special key `comment` for multi-line notes
> +    if object.contains_key("comment") {
> +        let comment = match object.get("comment") {

`contains_key` + `get` is somewhat wasteful and should be combined.

    if let Some(comment) = object.get("comment") {

For the type check you can then use `.as_str().ok_or_else(...)`

Or alternatively use a single match for both checks:

    match object.get("comment") {
        Some(Value::String(comment)) => {
            <the loop>
        }
        Some(_) => bail!(...),
        None => (),
    }

> +            Some(Value::String(v)) => v,
> +            _ => bail!("only strings can be comments"),
> +        };
> +
> +        for lines in comment.lines() {
> +            writeln!(output, "#{}", lines)?;
> +        }
> +    }
> +
>      for (key, value) in object.iter() {

Given that we type-check the comment above _and_ the data matching the
schema is a precondition for calling this function, I'd just put an

    if key == "comment" { continue }

here rather than the conditional check limited to the `Value::String` case below.

>          match value {
>              Value::Null => continue, // delete this entry
>              Value::Bool(v) => writeln!(output, "{}: {}", key, v)?,
> +            Value::String(_) if key == "comment" => continue, // skip comment as we handle it above
>              Value::String(v) => {
>                  if v.as_bytes().contains(&b'\n') {
>                      bail!("value for {} contains newlines", key);
> @@ -172,3 +199,38 @@ fn test() {
>  
>      assert_eq!(config, NODE_CONFIG.as_bytes());
>  }
> +
> +#[test]
> +fn test_with_comment() {
> +    use proxmox_schema::ApiType;
> +
> +    // let's just reuse some schema we actually have available:
> +    use crate::config::node::NodeConfig;
> +
> +    const NODE_INPUT: &str = "\
> +        #this should\n\
> +        #be included\n\
> +        acme: account=pebble\n\
> +        # this should not\n\

^ I find it curous that your 'comment' section doesn't have leading
spaces, while the "real comment" does ;-)

Should we think about trimming off up to 1 space when parsing the lines
and prefix them with `"# "` when printing them out?
Though I suppose it doesn't matter much as since we drop "real" comments
on a RMW-cycle these files don't really need to bother much with being
all that eye-pleasing I guess...

Re: [pbs-devel] [PATCH proxmox-backup 3/5] fix #3067: api: add multi-line comments to node.cfg

[Stefan Sterz]

responses inline

On 23.02.22 11:28, Wolfgang Bumiller wrote:
> some comments inline
> 
> On Tue, Feb 22, 2022 at 12:25:54PM +0100, Stefan Sterz wrote:
>> -- snip --
>>   
>> +    pub MULTI_LINE_COMMENT_REGEX = r"(?m)^([[:^cntrl:]]*)\s*$";
> 
> I don't think the trailing `\s*` is necessary?
> 

Yes, you are right, stole that from the Perl implementation, which uses 
"m/^\#(.*)\s*$/" (to parse the configuration file, hence the extra 
"\#"). I am not too familiar with Perl so that's probably my mistake, 
but just to be sure, is it necessary over there [1]? Tried playing 
around with it a bit, but couldn't figure out the purpose of "\s" since 
"." already matches any character. Unless Perl does "lazy" matching 
here, then it trims the trailing whitespace. Note that, AFAICT, both "." 
and "[:^cntrl:]" do not match the line feed character (\n) (and 
[:^cntrl:] does not match the carriage return either), but that those 
occur after the "end of line" ("$") and, thus, would not be matched by 
"\s" either.

[1]: 
https://git.proxmox.com/?p=qemu-server.git;a=blob;f=PVE/QemuServer.pm;h=a99f1a56f64a4789a9dc62184856bab2927d68c8;hb=HEAD#l2369

>> -- snip -- 
>>  
>>   use pbs_api_types::{EMAIL_SCHEMA, OPENSSL_CIPHERS_TLS_1_2_SCHEMA, OPENSSL_CIPHERS_TLS_1_3_SCHEMA,
>> -    SINGLE_LINE_COMMENT_SCHEMA};
>> +    MULTI_LINE_COMMENT_SCHEMA};
> 
> Please check how rustfmt would deal with the above `use` statement ;-)
> 

Done, although rustfmt complains about quite a few more things in those 
three files. Should I leave them or let rustfmt fix them while I am at it?

>> -- snip -- >>>> +    let comment = input.lines()
>> +        .take_while(|l| l.starts_with('#'))
>> +        .map(|l| {
>> +            let mut ch = l.chars();
>> +            ch.next();
>> +            ch.as_str()
> 
> ^ The `take_while` ensures `l` starts with '#', so you could just use
> 
>      .map(|l| &l[1..])
> 
> Alternatively, since 1.57 (and we're at 1.58 now), you could also
> combine the `.take_while` and `.map` into:
> 
>      .map_while(|l| l.strip_prefix("#"))
> 
> However...
> 
>> +        })
>> +        .fold(String::new(), |acc, l| acc + l + "\n");
>> +
>> +    if !comment.is_empty() {
>> +        config.insert("comment".to_string(), Value::String(comment));
>> +    }
>> +
>>       for (lineno, line) in input.lines().enumerate() {
> 
> ... here we're starting over, so maybe we should refactor this a little.
> Eg. we could use a `.lines().enumerate().peekable()` iterator:
> 
>      let mut lines = input.lines().enumerate().peekable();
>      let mut comments = String::new();
>      while let Some((_, line)) = iter.next_if(|(_, line)| line.starts_with('#') {
>          comments.push_str(&line[1..]);
>          comments.push('\n');
>      }
> 
>      for (lineno, line) in lines {
>      <...>
> 

Done. Thanks :-)

>> -- snip --
>>
>> +    if object.contains_key("comment") {
>> +        let comment = match object.get("comment") {
> 
> `contains_key` + `get` is somewhat wasteful and should be combined.
> 
>      if let Some(comment) = object.get("comment") {
> 
> For the type check you can then use `.as_str().ok_or_else(...)`
> 
> Or alternatively use a single match for both checks:
> 
>      match object.get("comment") {
>          Some(Value::String(comment)) => {
>              <the loop>
>          }
>          Some(_) => bail!(...),
>          None => (),
>      }
> 

Done. Combined it to:

      if let Some(Value::String(comment)) = object.get("comment") {
          /* loop here */
      }

Hope that's alright, personally I find that more legible than the match 
or `as_str().ok_or_else()`, although it is quite compact.

>> +            Some(Value::String(v)) => v,
>> +            _ => bail!("only strings can be comments"),
>> +        };
>> +
>> +        for lines in comment.lines() {
>> +            writeln!(output, "#{}", lines)?;
>> +        }
>> +    }
>> +
>>       for (key, value) in object.iter() {
> 
> Given that we type-check the comment above _and_ the data matching the
> schema is a precondition for calling this function, I'd just put an
> 
>      if key == "comment" { continue }
> 
> here rather than the conditional check limited to the `Value::String` case below.
> 

Coming back to rustfmt, it seems to want to format this as a multi-line 
if. So either, we ignore that and make it a single line if anyway, let 
rustfmt have its way, or we could do:

      match value {
              _ if key == "comment" => continue,
              Value::Null => continue,           // delete this entry
              /*..*/
      }

I'll switch it to whatever option is deemed nicer.

>> -- snip --
>> >> +
>> +#[test]
>> +fn test_with_comment() {
>> +    use proxmox_schema::ApiType;
>> +
>> +    // let's just reuse some schema we actually have available:
>> +    use crate::config::node::NodeConfig;
>> +
>> +    const NODE_INPUT: &str = "\
>> +        #this should\n\
>> +        #be included\n\
>> +        acme: account=pebble\n\
>> +        # this should not\n\
> 
> ^ I find it curous that your 'comment' section doesn't have leading
> spaces, while the "real comment" does ;-)
> 
> Should we think about trimming off up to 1 space when parsing the lines
> and prefix them with `"# "` when printing them out?
> Though I suppose it doesn't matter much as since we drop "real" comments
> on a RMW-cycle these files don't really need to bother much with being
> all that eye-pleasing I guess...

Note that in cases where indentation matters for Markdown rendering, 
such as lists, it is not trivial to distinguish between a space that is 
necessary for proper indentation and a space that a user wants for 
"cosmetic" reasons. It is currently valid Markdown to indent nested 
lists with only one space, i.e. the following note:

#- one
# - two

will render two as a nested list.

Further, AFAICT, that is how PVE handles these comments (just a 
preceding '#' no extra space etc.) [1,2]. I think there is some value in 
keeping this behavior consistent between products. My "real comment" has 
an extra space, because I find the lack of a space to be somewhat 
"claustrophobic" :-)

If someone where to add an extra space manually, that should still be 
parse-able. Extra spaces would be present, but not visible in the 
rendered HTML (due to how Markdown/HTML deal with extra spaces, with the 
exception of the cases mentioned above). From my testing that is 
consistent with PVE's behavior.

[1]: 
https://git.proxmox.com/?p=qemu-server.git;a=blob;f=PVE/QemuServer.pm;h=a99f1a56f64a4789a9dc62184856bab2927d68c8;hb=HEAD#l2369
[2]: 
https://git.proxmox.com/?p=qemu-server.git;a=blob;f=PVE/QemuServer.pm;h=a99f1a56f64a4789a9dc62184856bab2927d68c8;hb=HEAD#l2497