* [pbs-devel] [PATCH proxmox-backup 1/5] fix #3067: api: add support for a comment field in node.cfg
2022-02-22 11:25 [pbs-devel] [PATCH proxmox-backup 0/5] fix #3067: add notes functionality to webui Stefan Sterz
@ 2022-02-22 11:25 ` Stefan Sterz
2022-02-22 11:25 ` [pbs-devel] [PATCH proxmox-backup 2/5] fix #3067: pbs ui: add support for a notes field in the dashboard Stefan Sterz
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Stefan Sterz @ 2022-02-22 11:25 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [pbs-devel] [PATCH proxmox-backup 2/5] fix #3067: pbs ui: add support for a notes field in the dashboard
2022-02-22 11:25 [pbs-devel] [PATCH proxmox-backup 0/5] fix #3067: add notes functionality to webui Stefan Sterz
2022-02-22 11:25 ` [pbs-devel] [PATCH proxmox-backup 1/5] fix #3067: api: add support for a comment field in node.cfg Stefan Sterz
@ 2022-02-22 11:25 ` Stefan Sterz
2022-02-22 11:25 ` [pbs-devel] [PATCH proxmox-backup 3/5] fix #3067: api: add multi-line comments to node.cfg Stefan Sterz
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Stefan Sterz @ 2022-02-22 11:25 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [pbs-devel] [PATCH proxmox-backup 3/5] fix #3067: api: add multi-line comments to node.cfg
2022-02-22 11:25 [pbs-devel] [PATCH proxmox-backup 0/5] fix #3067: add notes functionality to webui Stefan Sterz
2022-02-22 11:25 ` [pbs-devel] [PATCH proxmox-backup 1/5] fix #3067: api: add support for a comment field in node.cfg Stefan Sterz
2022-02-22 11:25 ` [pbs-devel] [PATCH proxmox-backup 2/5] fix #3067: pbs ui: add support for a notes field in the dashboard Stefan Sterz
@ 2022-02-22 11:25 ` Stefan Sterz
2022-02-23 10:28 ` Wolfgang Bumiller
2022-02-22 11:25 ` [pbs-devel] [PATCH proxmox-backup 4/5] fix #3607: ui: make dashboard notes markdown capable Stefan Sterz
2022-02-22 11:25 ` [pbs-devel] [PATCH proxmox-backup 5/5] fix #3607: ui: add a separate notes view for longer markdown notes Stefan Sterz
4 siblings, 1 reply; 8+ messages in thread
From: Stefan Sterz @ 2022-02-22 11:25 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 3/5] fix #3067: api: add multi-line comments to node.cfg
2022-02-22 11:25 ` [pbs-devel] [PATCH proxmox-backup 3/5] fix #3067: api: add multi-line comments to node.cfg Stefan Sterz
@ 2022-02-23 10:28 ` Wolfgang Bumiller
2022-02-23 14:41 ` Stefan Sterz
0 siblings, 1 reply; 8+ messages in thread
From: Wolfgang Bumiller @ 2022-02-23 10:28 UTC (permalink / raw)
To: Stefan Sterz; +Cc: pbs-devel
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...
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 3/5] fix #3067: api: add multi-line comments to node.cfg
2022-02-23 10:28 ` Wolfgang Bumiller
@ 2022-02-23 14:41 ` Stefan Sterz
0 siblings, 0 replies; 8+ messages in thread
From: Stefan Sterz @ 2022-02-23 14:41 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: pbs-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 4/5] fix #3607: ui: make dashboard notes markdown capable
2022-02-22 11:25 [pbs-devel] [PATCH proxmox-backup 0/5] fix #3067: add notes functionality to webui Stefan Sterz
` (2 preceding siblings ...)
2022-02-22 11:25 ` [pbs-devel] [PATCH proxmox-backup 3/5] fix #3067: api: add multi-line comments to node.cfg Stefan Sterz
@ 2022-02-22 11:25 ` Stefan Sterz
2022-02-22 11:25 ` [pbs-devel] [PATCH proxmox-backup 5/5] fix #3607: ui: add a separate notes view for longer markdown notes Stefan Sterz
4 siblings, 0 replies; 8+ messages in thread
From: Stefan Sterz @ 2022-02-22 11:25 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [pbs-devel] [PATCH proxmox-backup 5/5] fix #3607: ui: add a separate notes view for longer markdown notes
2022-02-22 11:25 [pbs-devel] [PATCH proxmox-backup 0/5] fix #3067: add notes functionality to webui Stefan Sterz
` (3 preceding siblings ...)
2022-02-22 11:25 ` [pbs-devel] [PATCH proxmox-backup 4/5] fix #3607: ui: make dashboard notes markdown capable Stefan Sterz
@ 2022-02-22 11:25 ` Stefan Sterz
4 siblings, 0 replies; 8+ messages in thread
From: Stefan Sterz @ 2022-02-22 11:25 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread