diff --git a/packages/perspective/index.d.ts b/packages/perspective/index.d.ts index b94f7c7560..dfce052264 100644 --- a/packages/perspective/index.d.ts +++ b/packages/perspective/index.d.ts @@ -138,7 +138,7 @@ declare module "@finos/perspective" { }; export type ColumnName = string | null; - export type Expression = string; + export type Expression = { name: string; expr: string }; export type Filter = [ ColumnName, FilterOp, @@ -169,7 +169,7 @@ declare module "@finos/perspective" { delete(): Promise; on_delete(callback: () => void): void; validate_expressions( - expressions: Array + expressions: Array ): Promise; schema(): Promise; size(): Promise; diff --git a/packages/perspective/src/js/perspective.js b/packages/perspective/src/js/perspective.js index cf775e36ac..6e1b14cdeb 100644 --- a/packages/perspective/src/js/perspective.js +++ b/packages/perspective/src/js/perspective.js @@ -1415,13 +1415,13 @@ export default function (Module) { * which will be used in the engine to look up column values. * * @private - * @param {Array} expressions + * @param {Array<{name: string, expr: string}> | Array} expressions */ function parse_expression_strings(expressions) { let validated_expressions = []; const expression_idx_map = {}; - for (let expression_string of expressions) { + for (let expression of expressions) { // Map of column names to column IDs, so that we generate // column IDs correctly without collision. let column_name_map = {}; @@ -1434,19 +1434,24 @@ export default function (Module) { // First, look for a column alias, which is a // style comment // on the first line of the expression. let expression_alias; - let alias_match = expression_string.match(/^\/\/(?.+?)\n/); + if (expression.name && expression.name !== "") { + expression_alias = expression.name; + } else { + let alias_match = expression.match(/^\/\/(?.+?)\n/); - if (alias_match?.groups?.alias) { - expression_alias = alias_match.groups.alias.trim(); - } + if (alias_match?.groups?.alias) { + expression_alias = alias_match.groups.alias.trim(); + } - // If an alias does not exist, the alias is the expression itself. - if (!expression_alias || expression_alias.length == 0) { - expression_alias = expression_string; + // If an alias does not exist, the alias is the expression itself. + if (!expression_alias || expression_alias.length == 0) { + expression_alias = expression; + } } // Replace `true` and `false` reserved words with symbols - let parsed_expression_string = expression_string.replace( + let expr_val = expression.expr ?? expression; + let parsed_expression_string = expr_val.replace( /([a-zA-Z_]+[a-zA-Z0-9_]*)/g, (match) => { if (match == "true") { @@ -1516,7 +1521,7 @@ export default function (Module) { const validated = [ expression_alias, - expression_string, + expr_val, parsed_expression_string, column_id_map, ]; @@ -1544,7 +1549,7 @@ export default function (Module) { * passed in is not in `expressions`, it is guaranteed to be in `errors`. * * @async - * @param {Array} expressions An array of string expressions to + * @param {Array<{name: string, expr: string}>} expressions An array of expressions to * be validated. * * @returns {Promise} diff --git a/packages/perspective/test/js/expressions/functionality.spec.js b/packages/perspective/test/js/expressions/functionality.spec.js index 722b9a6bfa..2acbdbff14 100644 --- a/packages/perspective/test/js/expressions/functionality.spec.js +++ b/packages/perspective/test/js/expressions/functionality.spec.js @@ -3253,6 +3253,46 @@ const perspective = require("@finos/perspective"); view.delete(); table.delete(); }); + + test("Validation works with both input formats", async function () { + const table = await perspective.table({ + a: [1, 2, 3, 4], + b: [1, 2, 3, 4], + }); + const str_exprs = [ + '// x\n"a"', + '// y\n"b" * 0.5', + "// c\n'abcdefg'", + "// d\ntrue and false", + '// e\nfloat("a") > 2 ? null : 1', + "// f\ntoday()", + "// g\nnow()", + "// h\nlength(123)", + ]; + const dict_exprs = [ + { name: "x", expr: '"a"' }, + { name: "y", expr: '"b" * 0.5' }, + { name: "c", expr: "'abcdefg'" }, + { name: "d", expr: "true and false" }, + { name: "e", expr: 'float("a") > 2 ? null : 1' }, + { name: "f", expr: "today()" }, + { name: "g", expr: "now()" }, + { name: "h", expr: "length(123)" }, + ]; + let vstrs = await table.validate_expressions(str_exprs); + let vdicts = await table.validate_expressions(dict_exprs); + + expect(vstrs.errors).toEqual(vdicts.errors); + expect(vstrs.expression_schema).toEqual( + vdicts.expression_schema + ); + + for (let [key, val] of Object.entries(vstrs.expression_alias)) { + let dict_val = vdicts.expression_alias[key]; + let parsed_val = `// ${key}\n${dict_val}`; + expect(val).toEqual(parsed_val); + } + }); }); }); })(perspective); diff --git a/python/perspective/perspective/table/_utils.py b/python/perspective/perspective/table/_utils.py index f43ea3205b..4304ab5b3e 100644 --- a/python/perspective/perspective/table/_utils.py +++ b/python/perspective/perspective/table/_utils.py @@ -10,6 +10,7 @@ # ┃ of the [Apache License 2.0](https://www.apache.org/licenses/LICENSE-2.0). ┃ # ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ +from operator import itemgetter import re from datetime import date, datetime from functools import partial @@ -142,7 +143,7 @@ def _replace_interned_param(match_obj): return "{}'{}'{}".format(full[0:intern_idx], value, full[intern_idx + len(intern_fn) :]) -def _parse_expression_strings(expressions): +def _parse_expression_inputs(expressions): """Given a list of string expressions, parse out column names and string literals using regex and return a list of lists that contain three items in each inner list: @@ -156,24 +157,24 @@ def _parse_expression_strings(expressions): alias_map = {} for expression in expressions: - if '""' in expression: + if type(expression) is dict: + expr_raw, alias = itemgetter("expr", "name")(expression) + parsed = expr_raw + else: + expr_raw = expression + parsed = expr_raw + alias_match = re.match(ALIAS_REGEX, expr_raw) + if alias_match: + alias = alias_match.group(1).strip() + else: + alias = expr_raw + + if '""' in expr_raw: raise ValueError("Cannot reference empty column in expression!") column_id_map = {} column_name_map = {} - alias_match = re.match(ALIAS_REGEX, expression) - - # initialize `parsed` here so we keep `expression` unedited as the - # user typed it into Perspective - parsed = expression - - if alias_match: - alias = alias_match.group(1).strip() - else: - # Expression itself is the alias - alias = expression - # we need to be able to modify the running_cidx inside of every call to # replacer_fn - must pass by reference unfortunately running_cidx = [0] @@ -204,7 +205,7 @@ def _parse_expression_strings(expressions): parsed = re.sub(FUNCTION_LITERAL_REGEX, _replace_interned_param, parsed) parsed = re.sub(REPLACE_FN_REGEX, _replace_interned_param, parsed) - validated = [alias, expression, parsed, column_id_map] + validated = [alias, expr_raw, parsed, column_id_map] if alias_map.get(alias) is not None: idx = alias_map[alias] diff --git a/python/perspective/perspective/table/table.py b/python/perspective/perspective/table/table.py index 6a85216d6b..cb5e22738c 100644 --- a/python/perspective/perspective/table/table.py +++ b/python/perspective/perspective/table/table.py @@ -20,7 +20,7 @@ from ._utils import ( _dtype_to_pythontype, _dtype_to_str, - _parse_expression_strings, + _parse_expression_inputs, _str_to_pythontype, ) from .libpsppy import ( @@ -196,7 +196,7 @@ def validate_expressions(self, expressions, as_string=False): `error_message`, `line`, and `column`. Args: - expressions (:obj:`list`): A list of string expressions to validate + expressions (:obj:`list`): A list of string expressions or {name: string, expr: string} objects to validate and create a schema from. Keyword Args: @@ -208,7 +208,7 @@ def validate_expressions(self, expressions, as_string=False): if len(expressions) == 0: return validated - expressions = _parse_expression_strings(expressions) + expressions = _parse_expression_inputs(expressions) validation_results = validate_expressions(self._table, expressions) expression_schema = validation_results.get_expression_schema() expression_errors = validation_results.get_expression_errors() @@ -444,7 +444,7 @@ def view( config = {} if expressions is not None: - config["expressions"] = _parse_expression_strings(expressions) + config["expressions"] = _parse_expression_inputs(expressions) if columns is None: config["columns"] = self.columns() diff --git a/python/perspective/perspective/tests/table/test_view_expression.py b/python/perspective/perspective/tests/table/test_view_expression.py index ab6a881493..8aa67cfc46 100644 --- a/python/perspective/perspective/tests/table/test_view_expression.py +++ b/python/perspective/perspective/tests/table/test_view_expression.py @@ -25,6 +25,38 @@ def randstr(length, input=ascii_letters): class TestViewExpression(object): + def test_expression_conversion(self): + table = Table({"a": [1, 2, 3, 4], "b": [5, 6, 7, 8]}) + test_expressions_str = [ + '// x\n"a"', + '// y\n"b" * 0.5', + "// c\n'abcdefg'", + "// d\ntrue and false", + '// e\nfloat("a") > 2 ? null : 1', + "// f\ntoday()", + "// g\nnow()", + "// h\nlength(123)", + ] + test_expressions_dict = [ + {"name": "x", "expr": '"a"'}, + {"name": "y", "expr": '"b" * 0.5'}, + {"name": "c", "expr": "'abcdefg'"}, + {"name": "d", "expr": "true and false"}, + {"name": "e", "expr": 'float("a") > 2 ? null : 1'}, + {"name": "f", "expr": "today()"}, + {"name": "g", "expr": "now()"}, + {"name": "h", "expr": "length(123)"}, + ] + str_validated = table.validate_expressions(test_expressions_str) + dict_validated = table.validate_expressions(test_expressions_dict) + assert str_validated["errors"] == dict_validated["errors"] + assert str_validated["expression_schema"] == dict_validated["expression_schema"] + assert str_validated["expression_alias"].keys() == dict_validated["expression_alias"].keys() + for dict_key, dict_val in dict_validated["expression_alias"].items(): + str_alias = str_validated["expression_alias"] + matched_val = "// {}\n{}".format(dict_key, dict_val) + assert str_alias[dict_key] == matched_val + def test_table_validate_expressions_empty(self): table = Table({"a": [1, 2, 3, 4], "b": [5, 6, 7, 8]}) validate = table.validate_expressions([]) diff --git a/rust/perspective-viewer/src/less/column-settings-panel.less b/rust/perspective-viewer/src/less/column-settings-panel.less index e762c62404..9cec1d9ce2 100644 --- a/rust/perspective-viewer/src/less/column-settings-panel.less +++ b/rust/perspective-viewer/src/less/column-settings-panel.less @@ -23,4 +23,28 @@ -webkit-mask-image: var(--column-settings-icon--mask-image); mask-image: var(--column-settings-icon--mask-image); } + + // NOTE: These should probably make their way to global form styling eventually. + .errored { + outline-color: var(--error--color); + } + .item_title { + font-size: 9px; + } + input { + &[type="text"] { + outline: 1px solid; + outline-color: var(--inactive--color); + background-color: var(--plugin--background); + border: none; + border-radius: 2px; + margin-bottom: 0.5em; + font-family: "Roboto Mono", monospace; + font-size: 12px; + min-height: 24px; + &:disabled { + background-color: var(--inactive--color); + } + } + } } diff --git a/rust/perspective-viewer/src/less/containers/tabs.less b/rust/perspective-viewer/src/less/containers/tabs.less index b4e8d79b4e..fa36bd1b37 100644 --- a/rust/perspective-viewer/src/less/containers/tabs.less +++ b/rust/perspective-viewer/src/less/containers/tabs.less @@ -61,9 +61,6 @@ } } .tab-content { - * { - font-size: 12px; - } .tab-section { padding: 8px; // border-bottom: 1px solid var(--inactive--border-color); @@ -72,8 +69,5 @@ font-size: 14px; margin-left: 1em; } - .item_title { - margin-bottom: 1em; - } } } diff --git a/rust/perspective-viewer/src/rust/components/column_dropdown.rs b/rust/perspective-viewer/src/rust/components/column_dropdown.rs index cb3ce5d27d..50f9c19edc 100644 --- a/rust/perspective-viewer/src/rust/components/column_dropdown.rs +++ b/rust/perspective-viewer/src/rust/components/column_dropdown.rs @@ -136,7 +136,7 @@ impl Component for ColumnDropDown { { col } }, InPlaceColumn::Expression(col) => html! { - { col } + { col.name.clone() } }, }; diff --git a/rust/perspective-viewer/src/rust/components/column_selector/active_column.rs b/rust/perspective-viewer/src/rust/components/column_selector/active_column.rs index b8d9d82623..607bad9a51 100644 --- a/rust/perspective-viewer/src/rust/components/column_selector/active_column.rs +++ b/rust/perspective-viewer/src/rust/components/column_selector/active_column.rs @@ -207,12 +207,12 @@ impl Component for ActiveColumn { New(InPlaceColumn::Expression(col)) => { let mut view_config = ctx.props().session.get_view_config().clone(); if ctx.props().idx >= view_config.columns.len() { - view_config.columns.push(Some(col.clone())); + view_config.columns.push(Some(col.name.clone())); } else { - view_config.columns[ctx.props().idx] = Some(col.clone()); + view_config.columns[ctx.props().idx] = Some(col.name.clone()); } - view_config.expressions.push(col); + view_config.expressions.push(col.into()); let update = ViewConfigUpdate { columns: Some(view_config.columns), expressions: Some(view_config.expressions), diff --git a/rust/perspective-viewer/src/rust/components/column_selector/config_selector.rs b/rust/perspective-viewer/src/rust/components/column_selector/config_selector.rs index a4811f90b0..b25c47ea96 100644 --- a/rust/perspective-viewer/src/rust/components/column_selector/config_selector.rs +++ b/rust/perspective-viewer/src/rust/components/column_selector/config_selector.rs @@ -385,8 +385,8 @@ impl Component for ConfigSelector { } ConfigSelectorMsg::New(DragTarget::GroupBy, InPlaceColumn::Expression(col)) => { let mut view_config = ctx.props().session.get_view_config().clone(); - view_config.group_by.push(col.clone()); - view_config.expressions.push(col); + view_config.group_by.push(col.name.clone()); + view_config.expressions.push(col.into()); let update = ViewConfigUpdate { group_by: Some(view_config.group_by), expressions: Some(view_config.expressions), @@ -399,8 +399,8 @@ impl Component for ConfigSelector { } ConfigSelectorMsg::New(DragTarget::SplitBy, InPlaceColumn::Expression(col)) => { let mut view_config = ctx.props().session.get_view_config().clone(); - view_config.split_by.push(col.clone()); - view_config.expressions.push(col); + view_config.split_by.push(col.name.clone()); + view_config.expressions.push(col.into()); let update = ViewConfigUpdate { split_by: Some(view_config.split_by), expressions: Some(view_config.expressions), @@ -414,11 +414,11 @@ impl Component for ConfigSelector { ConfigSelectorMsg::New(DragTarget::Filter, InPlaceColumn::Expression(col)) => { let mut view_config = ctx.props().session.get_view_config().clone(); view_config.filter.push(Filter( - col.clone(), + col.name.clone(), FilterOp::EQ, FilterTerm::Scalar(Scalar::Null), )); - view_config.expressions.push(col); + view_config.expressions.push(col.into()); let update = ViewConfigUpdate { filter: Some(view_config.filter), expressions: Some(view_config.expressions), @@ -431,8 +431,8 @@ impl Component for ConfigSelector { } ConfigSelectorMsg::New(DragTarget::Sort, InPlaceColumn::Expression(col)) => { let mut view_config = ctx.props().session.get_view_config().clone(); - view_config.sort.push(Sort(col.clone(), SortDir::Asc)); - view_config.expressions.push(col); + view_config.sort.push(Sort(col.name.clone(), SortDir::Asc)); + view_config.expressions.push(col.into()); let update = ViewConfigUpdate { sort: Some(view_config.sort), expressions: Some(view_config.expressions), diff --git a/rust/perspective-viewer/src/rust/components/column_selector/empty_column.rs b/rust/perspective-viewer/src/rust/components/column_selector/empty_column.rs index d0e6482e45..8ebf3f4b20 100644 --- a/rust/perspective-viewer/src/rust/components/column_selector/empty_column.rs +++ b/rust/perspective-viewer/src/rust/components/column_selector/empty_column.rs @@ -16,6 +16,7 @@ use web_sys::*; use yew::prelude::*; use crate::components::style::LocalStyle; +use crate::config::Expression; use crate::css; use crate::custom_elements::ColumnDropDownElement; @@ -28,7 +29,7 @@ pub struct EmptyColumn { pub enum InPlaceColumn { Column(String), // ExpressionAlias(String), - Expression(String), + Expression(Expression), } #[derive(Properties)] diff --git a/rust/perspective-viewer/src/rust/components/column_settings_sidebar/attributes_tab.rs b/rust/perspective-viewer/src/rust/components/column_settings_sidebar/attributes_tab.rs index 4b53fb3629..bfe2e15323 100644 --- a/rust/perspective-viewer/src/rust/components/column_settings_sidebar/attributes_tab.rs +++ b/rust/perspective-viewer/src/rust/components/column_settings_sidebar/attributes_tab.rs @@ -16,7 +16,6 @@ use expression_editor::ExprEditorAttr; use yew::{function_component, html, Callback, Html, Properties}; use crate::components::viewer::ColumnLocator; -use crate::config::Type; use crate::custom_events::CustomEvents; use crate::renderer::Renderer; use crate::session::Session; @@ -29,7 +28,6 @@ pub struct AttributesTabProps { pub session: Session, pub renderer: Renderer, pub custom_events: CustomEvents, - pub maybe_ty: Option, } #[function_component] @@ -37,12 +35,6 @@ pub fn AttributesTab(p: &AttributesTabProps) -> Html { clone!(p.on_close, p.selected_column, p.session, p.renderer); html_template! {
- if let Some(ty) = p.maybe_ty { -
-
{ "Type" }
-
{ ty }
-
- }
Html { let is_validating = yew::use_state_eq(|| false); - let on_save = yew::use_callback(p.clone(), |v, p| { + let on_save = yew::use_callback(p.clone(), |(v, noderef), p| { match &p.selected_column { - ColumnLocator::Expr(Some(alias)) => update_expr(alias, &v, p), - ColumnLocator::Expr(None) => save_expr(v, p), + ColumnLocator::Expr(Some(alias)) => update_expr(alias, noderef, &v, p), + ColumnLocator::Expr(None) => save_expr(v, noderef, p), // TODO: We should be able to create a new expression from the currently selected // column if it is not already an expression column. @@ -59,39 +60,55 @@ pub fn expression_editor_attr(p: &ExprEditorAttrProps) -> Html { html! {
-
{"Expression Editor"}
} } -fn update_expr(name: &str, new_expression: &JsValue, props: &ExprEditorAttrProps) { - let n = name.to_string(); - let exp = new_expression.clone(); +fn update_expr( + old_name: &str, + alias_ref: NodeRef, + new_expr_val: &JsValue, + props: &ExprEditorAttrProps, +) { + let old_name = old_name.to_string(); let sesh = props.session.clone(); let props = props.clone(); + let new_name = alias_ref.cast::().unwrap().value(); + let new_expr_val = new_expr_val.as_string().unwrap(); + let new_expr = Expression { + name: new_name, + expr: new_expr_val, + }; ApiFuture::spawn(async move { - let update = sesh.create_replace_expression_update(&n, &exp).await; + let update = sesh + .create_replace_expression_update(&old_name, &new_expr) + .await; props.update_and_render(update).await?; Ok(()) }); } -fn save_expr(expression: JsValue, props: &ExprEditorAttrProps) { +fn save_expr(expression: JsValue, alias_ref: NodeRef, props: &ExprEditorAttrProps) { let task = { - let expression = expression.as_string().unwrap(); - let mut expressions = props.session.get_view_config().expressions.clone(); - expressions.retain(|x| x != &expression); - expressions.push(expression); + let expression_val = expression.as_string().unwrap(); + let name = alias_ref.cast::().unwrap().value(); + let expr = Expression { + name, + expr: expression_val, + }; + let mut serde_exprs = props.session.get_view_config().expressions.clone(); + serde_exprs.retain(|serde_expr| serde_expr.name() != expr.name); + serde_exprs.push(expr.into()); props.update_and_render(ViewConfigUpdate { - expressions: Some(expressions), + expressions: Some(serde_exprs), ..Default::default() }) }; @@ -101,15 +118,15 @@ fn save_expr(expression: JsValue, props: &ExprEditorAttrProps) { fn delete_expr(expr_name: &str, props: &ExprEditorAttrProps) { let session = &props.session; - let expression = session - .metadata() - .get_expression_by_alias(expr_name) - .unwrap(); + // let expression = session + // .metadata() + // .get_expression_by_alias(expr_name) + // .unwrap(); - let mut expressions = session.get_view_config().expressions.clone(); - expressions.retain(|x| x != &expression); + let mut serde_exprs = session.get_view_config().expressions.clone(); + serde_exprs.retain(|serde_expr| serde_expr.name() != expr_name); let config = ViewConfigUpdate { - expressions: Some(expressions), + expressions: Some(serde_exprs), ..ViewConfigUpdate::default() }; diff --git a/rust/perspective-viewer/src/rust/components/column_settings_sidebar/sidebar.rs b/rust/perspective-viewer/src/rust/components/column_settings_sidebar/sidebar.rs index 7b16ffc175..ef0d5f75bf 100644 --- a/rust/perspective-viewer/src/rust/components/column_settings_sidebar/sidebar.rs +++ b/rust/perspective-viewer/src/rust/components/column_settings_sidebar/sidebar.rs @@ -81,7 +81,7 @@ pub fn ColumnSettingsSidebar(p: &ColumnSettingsProps) -> Html { let title = format!("Editing ‘{column_name}’..."); - let mut tabs = vec![ColumnSettingsTab::Attributes]; + let mut tabs = vec![]; if !matches!(p.selected_column, ColumnLocator::Expr(None)) && is_active @@ -89,9 +89,10 @@ pub fn ColumnSettingsSidebar(p: &ColumnSettingsProps) -> Html { && attrs.is_some() && maybe_ty.is_some() { - tabs.reverse(); tabs.push(ColumnSettingsTab::Style); - tabs.reverse(); + } + if matches!(p.selected_column, ColumnLocator::Expr(_)) { + tabs.push(ColumnSettingsTab::Attributes); } clone!( @@ -121,7 +122,6 @@ pub fn ColumnSettingsSidebar(p: &ColumnSettingsProps) -> Html { { selected_column } { on_close } - { maybe_ty } /> } } diff --git a/rust/perspective-viewer/src/rust/components/expression_editor.rs b/rust/perspective-viewer/src/rust/components/expression_editor.rs index 28782624a8..e7f0e7bd73 100644 --- a/rust/perspective-viewer/src/rust/components/expression_editor.rs +++ b/rust/perspective-viewer/src/rust/components/expression_editor.rs @@ -13,6 +13,7 @@ use std::rc::Rc; use wasm_bindgen::prelude::*; +use web_sys::HtmlInputElement; use yew::prelude::*; use super::containers::split_panel::*; @@ -29,15 +30,16 @@ pub enum ExpressionEditorMsg { SetExpr(Rc), ValidateComplete(Option), SaveExpr, + AliasChanged(InputEvent), } #[derive(Properties, PartialEq)] pub struct ExpressionEditorProps { pub session: Session, - pub on_save: Callback, + pub on_save: Callback<(JsValue, NodeRef)>, pub on_validate: Callback, pub on_delete: Option>, - pub alias: Option, + pub old_alias: Option, pub disabled: bool, } @@ -54,16 +56,11 @@ pub fn get_new_column_name(session: &Session) -> String { impl ExpressionEditorProps { fn initial_expr(&self) -> Rc { - let txt = if let Some(ref alias) = self.alias { - self.session - .metadata() - .get_expression_by_alias(alias) - .unwrap_or_default() - } else { - format!("// {}\n", get_new_column_name(&self.session)) - }; - - txt.into() + self.old_alias + .as_ref() + .and_then(|alias| self.session.metadata().get_expression_by_alias(alias)) + .unwrap_or_default() + .into() } } @@ -73,6 +70,8 @@ pub struct ExpressionEditor { edit_enabled: bool, expr: Rc, error: Option, + alias_ref: NodeRef, + alias: Option, } impl Component for ExpressionEditor { @@ -80,11 +79,18 @@ impl Component for ExpressionEditor { type Properties = ExpressionEditorProps; fn create(ctx: &Context) -> Self { + let alias = ctx + .props() + .old_alias + .clone() + .or_else(|| Some(get_new_column_name(&ctx.props().session))); Self { save_enabled: false, edit_enabled: false, error: None, expr: ctx.props().initial_expr(), + alias_ref: NodeRef::default(), + alias, } } @@ -95,9 +101,12 @@ impl Component for ExpressionEditor { clone!(ctx.props().session); ctx.props().on_validate.emit(true); ctx.link().send_future(async move { - match session.validate_expr(JsValue::from(&*val)).await { + match session.validate_expr(&val).await { Ok(x) => ExpressionEditorMsg::ValidateComplete(x), - Err(_err) => ExpressionEditorMsg::ValidateComplete(None), + Err(err) => { + web_sys::console::error_1(&err); + ExpressionEditorMsg::ValidateComplete(None) + } } }); @@ -107,7 +116,7 @@ impl Component for ExpressionEditor { self.error = err; if self.error.is_none() { let is_edited = maybe!({ - let alias = ctx.props().alias.as_ref()?; + let alias = ctx.props().old_alias.as_ref()?; let session = &ctx.props().session; let old = session.metadata().get_expression_by_alias(alias)?; let is_edited = *self.expr != old; @@ -118,7 +127,7 @@ impl Component for ExpressionEditor { }); self.edit_enabled = is_edited.unwrap_or_default(); - self.save_enabled = is_edited.unwrap_or(true); + self.save_enabled = is_edited.unwrap_or(true) && self.alias.is_some(); } else { self.save_enabled = false; } @@ -130,7 +139,7 @@ impl Component for ExpressionEditor { self.edit_enabled = false; self.save_enabled = false; maybe!({ - let alias = ctx.props().alias.as_ref()?; + let alias = ctx.props().old_alias.as_ref()?; let session = &ctx.props().session; let old = session.metadata().get_expression_by_alias(alias)?; self.expr = old.into(); @@ -143,7 +152,9 @@ impl Component for ExpressionEditor { ExpressionEditorMsg::SaveExpr => { if self.save_enabled { let expr = self.expr.to_owned(); - ctx.props().on_save.emit(JsValue::from(&*expr)); + ctx.props() + .on_save + .emit((JsValue::from(&*expr), self.alias_ref.clone())); } false @@ -155,6 +166,12 @@ impl Component for ExpressionEditor { false } + ExpressionEditorMsg::AliasChanged(event) => { + let value = event.target_unchecked_into::().value(); + self.alias = (!value.is_empty()).then_some(value); + self.save_enabled = self.error.is_none() && self.alias.is_some(); + true + } } } @@ -165,8 +182,12 @@ impl Component for ExpressionEditor { let oninput = ctx.link().callback(ExpressionEditorMsg::SetExpr); let onsave = ctx.link().callback(|()| ExpressionEditorMsg::SaveExpr); + + let on_alias_change = ctx.link().callback(ExpressionEditorMsg::AliasChanged); + let alias_class = self.alias.is_none().then_some("errored"); + let is_closable = maybe! { - let alias = ctx.props().alias.as_ref()?; + let alias = ctx.props().old_alias.as_ref()?; Some(!ctx.props().session.is_column_expression_in_use(alias)) } .unwrap_or_default(); @@ -176,47 +197,63 @@ impl Component for ExpressionEditor { html_template! { -
- - -
- if let Some(err) = &self.error { -
- { &err.error_message } -
- } +
+
+ + +
+
+
{ "Expression" }
+
+ - if is_closable { - - } +
+ if let Some(err) = &self.error { +
+ { &err.error_message } +
+ } - if ctx.props().alias.is_some() { - - } + if is_closable { + + } + + if ctx.props().old_alias.is_some() { + + } - + +
+
<> @@ -225,9 +262,14 @@ impl Component for ExpressionEditor { } fn changed(&mut self, ctx: &Context, old_props: &Self::Properties) -> bool { - if ctx.props().alias != old_props.alias { + if ctx.props().old_alias != old_props.old_alias { self.expr = ctx.props().initial_expr(); } + self.alias = ctx + .props() + .old_alias + .clone() + .or_else(|| Some(get_new_column_name(&ctx.props().session))); true } } diff --git a/rust/perspective-viewer/src/rust/config/expression.rs b/rust/perspective-viewer/src/rust/config/expression.rs new file mode 100644 index 0000000000..614f01ef9f --- /dev/null +++ b/rust/perspective-viewer/src/rust/config/expression.rs @@ -0,0 +1,103 @@ +// ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ +// ┃ ██████ ██████ ██████ █ █ █ █ █ █▄ ▀███ █ ┃ +// ┃ ▄▄▄▄▄█ █▄▄▄▄▄ ▄▄▄▄▄█ ▀▀▀▀▀█▀▀▀▀▀ █ ▀▀▀▀▀█ ████████▌▐███ ███▄ ▀█ █ ▀▀▀▀▀ ┃ +// ┃ █▀▀▀▀▀ █▀▀▀▀▀ █▀██▀▀ ▄▄▄▄▄ █ ▄▄▄▄▄█ ▄▄▄▄▄█ ████████▌▐███ █████▄ █ ▄▄▄▄▄ ┃ +// ┃ █ ██████ █ ▀█▄ █ ██████ █ ███▌▐███ ███████▄ █ ┃ +// ┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┫ +// ┃ Copyright (c) 2017, the Perspective Authors. ┃ +// ┃ ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ ┃ +// ┃ This file is part of the Perspective library, distributed under the terms ┃ +// ┃ of the [Apache License 2.0](https://www.apache.org/licenses/LICENSE-2.0). ┃ +// ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + +use serde::{Deserialize, Serialize}; + +#[derive(Serialize, Deserialize, Clone, PartialEq, Debug)] +pub struct Expression { + pub name: String, + pub expr: String, +} +impl Expression { + pub fn new(name: S1, expr: S2) -> Self { + Self { + name: name.to_string(), + expr: expr.to_string(), + } + } +} +impl PartialEq for Expression { + fn eq(&self, other: &ExprSerde) -> bool { + other.name() == self.name && other.expr() == self.expr + } +} + +#[derive(Serialize, Deserialize, Clone, PartialEq, Debug)] +#[serde(untagged)] +pub enum ExprSerde { + Str(String), + Struct(Expression), +} +impl ExprSerde { + pub fn name(&self) -> &str { + match self { + Self::Struct(s) => &s.name, + Self::Str(s) => + if let Some( (name, expr) ) = s.split_once('\n') && !expr.is_empty() && name.starts_with("//") { + name.split_at(2).1 + } else { + s + } + } + } + pub fn expr(&self) -> &str { + match self { + Self::Struct(s) => &s.expr, + Self::Str(s) => + if let Some( (name, expr) ) = s.split_once('\n') && !expr.is_empty() && name.starts_with("//") { + expr + } else { + s + } + } + } +} +impl From for ExprSerde { + fn from(value: Expression) -> Self { + ExprSerde::Struct(value) + } +} +impl From for ExprSerde { + fn from(value: S) -> Self { + ExprSerde::Str(value.to_string()) + } +} +impl From for Expression { + fn from(value: ExprSerde) -> Self { + match value { + ExprSerde::Struct(s) => s, + ExprSerde::Str(s) => { + if let Some( (name, expr) ) = s.split_once('\n') && !expr.is_empty() && name.starts_with("//") { + Self { + name: name.split_at(2).1.to_owned(), + expr: expr.to_owned(), + } + } else { + Self { + name: s.clone(), + expr: s.clone(), + } + } + } + } + } +} + +#[test] +fn test_expression_conversion() { + let expr_strs = ["1", "//expr\n2"]; + let expr_structs = [Expression::new("1", "1"), Expression::new("expr", "2")]; + for i in [0, 1] { + let str_serde: ExprSerde = expr_strs[i].into(); + assert_eq!(expr_structs[i], str_serde) + } +} diff --git a/rust/perspective-viewer/src/rust/config/mod.rs b/rust/perspective-viewer/src/rust/config/mod.rs index 5f172f62b8..fd26102af8 100644 --- a/rust/perspective-viewer/src/rust/config/mod.rs +++ b/rust/perspective-viewer/src/rust/config/mod.rs @@ -16,6 +16,7 @@ mod aggregates; mod column_type; mod datetime_column_style; +mod expression; mod filters; mod number_column_style; pub mod plugin; @@ -27,6 +28,7 @@ mod viewer_config; pub use aggregates::*; pub use column_type::*; pub use datetime_column_style::*; +pub use expression::*; pub use filters::*; pub use number_column_style::*; pub use sort::*; diff --git a/rust/perspective-viewer/src/rust/config/view_config.rs b/rust/perspective-viewer/src/rust/config/view_config.rs index 8aa93b0bd6..567203be8d 100644 --- a/rust/perspective-viewer/src/rust/config/view_config.rs +++ b/rust/perspective-viewer/src/rust/config/view_config.rs @@ -21,6 +21,7 @@ use wasm_bindgen_test::*; use {crate::*, js_sys::Array}; use super::aggregates::*; +use super::expression::*; use super::filters::*; use super::sort::*; use crate::js::perspective::JsPerspectiveViewConfig; @@ -45,7 +46,7 @@ pub struct ViewConfig { pub sort: Vec, #[serde(default)] - pub expressions: Vec, + pub expressions: Vec, #[serde(default)] pub aggregates: HashMap, @@ -123,7 +124,7 @@ pub struct ViewConfigUpdate { pub sort: Option>, #[serde(default)] - pub expressions: Option>, + pub expressions: Option>, #[serde(default)] pub aggregates: Option>, @@ -176,14 +177,11 @@ mod tests { let x = json!({ "filter": [filter] }); let rec: ViewConfig = x.into_serde_ext().unwrap(); - assert_eq!( - rec.filter, - vec![Filter( - "Test".to_owned(), - FilterOp::Contains, - FilterTerm::Scalar(Scalar::String("aaa".to_owned())) - )] - ); + assert_eq!(rec.filter, vec![Filter( + "Test".to_owned(), + FilterOp::Contains, + FilterTerm::Scalar(Scalar::String("aaa".to_owned())) + )]); } #[wasm_bindgen_test] @@ -194,14 +192,11 @@ mod tests { let x = json!({ "filter": [filter] }); let rec: ViewConfig = x.into_serde_ext().unwrap(); - assert_eq!( - rec.filter, - vec![Filter( - "Test".to_owned(), - FilterOp::LT, - FilterTerm::Scalar(Scalar::Float(4_f64)) - )] - ); + assert_eq!(rec.filter, vec![Filter( + "Test".to_owned(), + FilterOp::LT, + FilterTerm::Scalar(Scalar::Float(4_f64)) + )]); } #[wasm_bindgen_test] @@ -230,13 +225,10 @@ mod tests { let x = json!({"sort": [sort1, sort2]}); let rec: ViewConfig = x.into_serde_ext().unwrap(); - assert_eq!( - rec.sort, - vec![ - Sort("Test1".to_owned(), SortDir::Asc), - Sort("Test2".to_owned(), SortDir::Desc) - ] - ); + assert_eq!(rec.sort, vec![ + Sort("Test1".to_owned(), SortDir::Asc), + Sort("Test2".to_owned(), SortDir::Desc) + ]); } #[wasm_bindgen_test] diff --git a/rust/perspective-viewer/src/rust/custom_elements/column_dropdown.rs b/rust/perspective-viewer/src/rust/custom_elements/column_dropdown.rs index 3dd6c60964..48f6e0d58b 100644 --- a/rust/perspective-viewer/src/rust/custom_elements/column_dropdown.rs +++ b/rust/perspective-viewer/src/rust/custom_elements/column_dropdown.rs @@ -20,6 +20,7 @@ use yew::{props, Callback}; use crate::components::column_dropdown::*; use crate::components::column_selector::InPlaceColumn; +use crate::config::Expression; use crate::custom_elements::modal::*; use crate::session::Session; use crate::utils::ApiFuture; @@ -72,13 +73,13 @@ impl ColumnDropDownElement { clone!(self.modal, self.session); ApiFuture::spawn(async move { if !exclude.contains(&input) { - let is_expr = session - .validate_expr(JsValue::from_str(&input)) - .await? - .is_none(); + let is_expr = session.validate_expr(&input).await?.is_none(); if is_expr { - values.push(InPlaceColumn::Expression(input)); + values.push(InPlaceColumn::Expression(Expression { + name: input.clone(), + expr: input, + })); } } diff --git a/rust/perspective-viewer/src/rust/session.rs b/rust/perspective-viewer/src/rust/session.rs index 907cfb86b3..b16fc89d5c 100644 --- a/rust/perspective-viewer/src/rust/session.rs +++ b/rust/perspective-viewer/src/rust/session.rs @@ -37,7 +37,7 @@ use crate::dragdrop::*; use crate::js::perspective::*; use crate::js::plugin::*; use crate::utils::*; -use crate::*; +use crate::{JsValueSerdeExt, *}; /// The `Session` struct is the principal interface to the Perspective engine, /// the `Table` and `View` objects for this viewer, and all associated state @@ -195,30 +195,33 @@ impl Session { /// An async task which replaces a `column` aliased expression with another. pub async fn create_replace_expression_update( &self, - column: &str, - expression: &JsValue, + old_expr_name: &str, + new_expr: &Expression, ) -> ViewConfigUpdate { - let old_expression = self.metadata().get_expression_by_alias(column).unwrap(); - let new_name = self - .get_validated_expression_name(expression) - .await + let old_expr_val = self + .metadata() + .get_expression_by_alias(old_expr_name) .unwrap(); - let expression = expression.as_string().unwrap(); - self.get_view_config().create_replace_expression_update( - column, - &old_expression, - &new_name, - &expression, - ) + let old_expr = Expression { + name: old_expr_name.into(), + expr: old_expr_val, + }; + + self.get_view_config() + .create_replace_expression_update(&old_expr, new_expr) } /// Validate an expression strin and marshall the results. pub async fn validate_expr( &self, - expr: JsValue, + expr: &str, ) -> Result, JsValue> { - let arr = std::iter::once(expr).collect::(); + let arr = std::iter::once(JsValue::from_serde_ext(&Expression { + name: "_".into(), + expr: expr.to_owned(), + })?) + .collect::(); let table = self.borrow().table.as_ref().unwrap().clone(); let errors = table.validate_expressions(arr).await?.errors(); let error_keys = js_sys::Object::keys(&errors); @@ -361,13 +364,14 @@ impl Session { Ok(ValidSession(self)) } - async fn get_validated_expression_name(&self, expr: &JsValue) -> ApiResult { - let arr = std::iter::once(expr).collect::(); - let table = self.borrow().table.as_ref().unwrap().clone(); - let schema = table.validate_expressions(arr).await?.expression_schema(); - let schema_keys = js_sys::Object::keys(&schema); - schema_keys.get(0).as_string().into_apierror() - } + // async fn get_validated_expression_name(&self, expr: &JsValue) -> + // ApiResult { let arr = + // std::iter::once(expr).collect::(); let table = + // self.borrow().table.as_ref().unwrap().clone(); let schema = + // table.validate_expressions(arr).await?.expression_schema(); + // let schema_keys = js_sys::Object::keys(&schema); + // schema_keys.get(0).as_string().into_apierror() + // } async fn flat_as_jsvalue(&self, flat: bool) -> ApiResult { if flat { @@ -414,7 +418,7 @@ impl Session { let arr = config .expressions .iter() - .map(JsValue::from) + .filter_map(|expr| serde_wasm_bindgen::to_value(expr).ok()) .collect::(); let valid_recs = table.validate_expressions(arr).await?; diff --git a/rust/perspective-viewer/src/rust/session/metadata.rs b/rust/perspective-viewer/src/rust/session/metadata.rs index d62601bb05..994590d02e 100644 --- a/rust/perspective-viewer/src/rust/session/metadata.rs +++ b/rust/perspective-viewer/src/rust/session/metadata.rs @@ -139,7 +139,7 @@ impl SessionMetadata { /// # Arguments /// - `alias` An alias name for an expression column in this `Session`. pub fn get_expression_by_alias(&self, alias: &str) -> Option { - maybe!(self.as_ref()?.expr_meta.as_ref()?.alias.get(alias)).cloned() + self.as_ref()?.expr_meta.as_ref()?.alias.get(alias).cloned() } /// Returns the edited expression `String` (e.g. the not-yet-saved, edited diff --git a/rust/perspective-viewer/src/rust/session/replace_expression_update.rs b/rust/perspective-viewer/src/rust/session/replace_expression_update.rs index df5f9d8461..a511271f80 100644 --- a/rust/perspective-viewer/src/rust/session/replace_expression_update.rs +++ b/rust/perspective-viewer/src/rust/session/replace_expression_update.rs @@ -15,6 +15,7 @@ use std::collections::HashMap; use crate::config::*; impl ViewConfig { + // TODO: Split this up into expr and alias fns /// Create an update for this `ViewConfig` that replaces an expression /// column with a new one, e.g. when a user edits an expression. This may /// changed either the expression alias, the expression itself, or both; as @@ -24,12 +25,10 @@ impl ViewConfig { /// fill in `old_expression` and `new_alias`. pub(super) fn create_replace_expression_update( &self, - old_alias: &str, - old_expression: &str, - new_alias: &str, - new_expression: &str, + old_expr: &Expression, + new_expr: &Expression, ) -> ViewConfigUpdate { - let expression = new_expression; + let expression = new_expr; let Self { columns, expressions, @@ -43,11 +42,11 @@ impl ViewConfig { let expressions = expressions .into_iter() - .map(|x| { - if x == old_expression { - expression.to_owned() + .map(|serde_expr| { + if old_expr == &serde_expr { + expression.to_owned().into() } else { - x + serde_expr } }) .collect::>(); @@ -55,8 +54,8 @@ impl ViewConfig { let aggregates = aggregates .into_iter() .map(|x| { - if x.0 == old_expression { - (expression.to_owned(), x.1) + if x.0 == old_expr.name { + (expression.name.to_owned(), x.1) } else { x } @@ -66,7 +65,7 @@ impl ViewConfig { let columns = columns .into_iter() .map(|x| match x { - Some(x) if x == old_alias => Some(new_alias.to_owned()), + Some(x) if x == old_expr.name => Some(new_expr.name.to_owned()), x => x, }) .collect::>(); @@ -74,8 +73,8 @@ impl ViewConfig { let group_by = group_by .into_iter() .map(|x| { - if x == old_alias { - new_alias.to_owned() + if x == old_expr.name { + new_expr.name.to_owned() } else { x } @@ -85,8 +84,8 @@ impl ViewConfig { let split_by = split_by .into_iter() .map(|x| { - if x == old_alias { - new_alias.to_owned() + if x == old_expr.name { + new_expr.name.to_owned() } else { x } @@ -96,8 +95,8 @@ impl ViewConfig { let sort = sort .into_iter() .map(|x| { - if x.0 == old_alias { - Sort(new_alias.to_owned(), x.1) + if x.0 == old_expr.name { + Sort(new_expr.name.to_owned(), x.1) } else { x } @@ -108,8 +107,8 @@ impl ViewConfig { let filter = filter .into_iter() .map(|x| { - if x.0 == old_alias { - Filter(new_alias.to_owned(), x.1, x.2) + if x.0 == old_expr.name { + Filter(new_expr.name.to_owned(), x.1, x.2) } else { x } diff --git a/rust/perspective-viewer/test/js/column_settings.spec.ts b/rust/perspective-viewer/test/js/column_settings.spec.ts index 6d57ca26a9..820231fb73 100644 --- a/rust/perspective-viewer/test/js/column_settings.spec.ts +++ b/rust/perspective-viewer/test/js/column_settings.spec.ts @@ -32,9 +32,12 @@ export async function checkTab( }); let titles = await columnSettingsSidebar.tabTitle.all(); if (active) { - expect(titles.length).toBe(2); - expect(await titles[0].innerText()).toBe("Style"); - expect(await titles[1].innerText()).toBe("Attributes"); + if (expression) { + expect(await titles[0].innerText()).toBe("Style"); + expect(await titles[1].innerText()).toBe("Attributes"); + } else { + expect(await titles[0].innerText()).toBe("Style"); + } } else { if (expression) { expect(titles.length).toBe(1); diff --git a/tools/perspective-test/results.tar.gz b/tools/perspective-test/results.tar.gz index c634ee9e46..76517ef73b 100644 Binary files a/tools/perspective-test/results.tar.gz and b/tools/perspective-test/results.tar.gz differ diff --git a/tools/perspective-test/src/js/models/column_settings.ts b/tools/perspective-test/src/js/models/column_settings.ts index 76f14b1f58..0f466c989a 100644 --- a/tools/perspective-test/src/js/models/column_settings.ts +++ b/tools/perspective-test/src/js/models/column_settings.ts @@ -54,14 +54,18 @@ export class AttributesTab { export class ExpressionEditor { container: Locator; + nameInput: Locator; content: Locator; + textarea: Locator; saveBtn: Locator; resetBtn: Locator; deleteBtn: Locator; constructor(parent: Locator) { this.container = parent.locator("#editor-container"); + this.nameInput = parent.locator("#editor-alias-container input"); this.content = this.container.locator("#content"); + this.textarea = this.container.locator("textarea"); this.saveBtn = this.container.locator( "#psp-expression-editor-button-save" ); diff --git a/tools/perspective-test/src/js/models/settings_panel.ts b/tools/perspective-test/src/js/models/settings_panel.ts index 9902548e7d..8c427b58c2 100644 --- a/tools/perspective-test/src/js/models/settings_panel.ts +++ b/tools/perspective-test/src/js/models/settings_panel.ts @@ -54,13 +54,22 @@ export class SettingsPanel { await exprEditor.container.waitFor({ state: "visible", }); - expect(await exprEditor.content.isVisible()).toBe(true); - // brute force clear the expression editor - for (let i = 0; i < 30; i++) { - await exprEditor.content.press("Backspace", { delay: 10 }); - } - await exprEditor.content.type(`//${name}\n${expr}`, { delay: 100 }); - await exprEditor.content.blur(); + await exprEditor.nameInput.waitFor({ + state: "visible", + }); + expect(await exprEditor.textarea.isVisible()).toBe(true); + expect(await exprEditor.nameInput.isVisible()).toBe(true); + + await exprEditor.nameInput.focus(); + await exprEditor.nameInput.clear(); + await exprEditor.nameInput.type(name, { delay: 100 }); + await exprEditor.nameInput.blur(); + + await exprEditor.textarea.focus(); + await exprEditor.textarea.clear(); + await exprEditor.textarea.type(expr, { delay: 100 }); + await exprEditor.textarea.blur(); + let saveBtn = this.pageView.page.locator( "#psp-expression-editor-button-save" );