Skip to content

Commit

Permalink
Merge pull request #2851 from finos/viewer-restore-lock
Browse files Browse the repository at this point in the history
Fix `perspective-workspace` restore call on errored table
  • Loading branch information
texodus authored Nov 15, 2024
2 parents d630f0f + c4fd31e commit 126a6a1
Show file tree
Hide file tree
Showing 9 changed files with 84 additions and 23 deletions.
23 changes: 13 additions & 10 deletions packages/perspective-viewer-datagrid/src/js/model/toolbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,21 @@ export function toggle_edit_mode(mode = undefined) {
];
}

this.model._edit_mode = mode;
this.parentElement?.setSelection();
this.model._selection_state = {
selected_areas: [],
dirty: true,
};
if (this.model) {
this.model._edit_mode = mode;
this.parentElement?.setSelection();
this.model._selection_state = {
selected_areas: [],
dirty: true,
};

this._edit_mode = mode;
this.dataset.editMode = mode;
if (this._edit_button !== undefined) {
this._edit_button.dataset.editMode = mode;
this._edit_mode = mode;
if (this._edit_button !== undefined) {
this._edit_button.dataset.editMode = mode;
}
}

this.dataset.editMode = mode;
}

export function toggle_scroll_lock(force = undefined) {
Expand Down
51 changes: 51 additions & 0 deletions packages/perspective-workspace/test/js/table.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,57 @@ function tests(context, compare) {

return compare(page, `${context}-replace-table-frees-table.txt`);
});

test("replaceTable() works when previous table errored", async ({
page,
}) => {
const config = {
viewers: {
One: { table: "errored", name: "One" },
},
detail: {
main: {
currentIndex: 0,
type: "tab-area",
widgets: ["One"],
},
},
};

const result = await page.evaluate(async (config) => {
const workspace = document.getElementById("workspace");
await workspace.addTable(
"errored",
new Promise((_, reject) => setTimeout(reject, 50))
);

try {
await workspace.restore(config);
} catch (e) {
console.error(e);
return e.toString();
}
}, config);

// NOTE This is the error message we expect when `restore()` is called
// without a `Table`, subject to change.
expect(result).toEqual("Error: `restore()` called before `load()`");
await page.evaluate(async () => {
await workspace.replaceTable(
"errored",
window.__WORKER__.table("x\n1")
);
});

await page.evaluate(async () => {
await workspace.flush();
});

return compare(
page,
`${context}-replace-table-works-with-errored-table.txt`
);
});
}

test.describe("Workspace table functions", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,12 @@ impl StyleTabProps {
.presentation
.update_columns_config_value(props.column_name.clone(), update);
let columns_configs = props.presentation.all_columns_configs();
let plugin_config = props.renderer.get_active_plugin()?.save();
let plugin_config = props.renderer.get_active_plugin()?.save()?;
props
.renderer
.get_active_plugin()?
.restore(&plugin_config, Some(&columns_configs));
.restore(&plugin_config, Some(&columns_configs))?;

props.renderer.update(&props.session).await?;
let detail = serde_wasm_bindgen::to_value(&columns_configs).unwrap();
props.custom_events.dispatch_column_style_changed(&detail);
Expand Down
2 changes: 1 addition & 1 deletion rust/perspective-viewer/src/rust/components/viewer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ impl Component for PerspectiveViewer {
Some(presentation.all_columns_configs())
};

renderer.reset(columns_config.as_ref()).await;
renderer.reset(columns_config.as_ref()).await?;
presentation.reset_available_themes(None).await;
let result = renderer.draw(session.validate().await?.create_view()).await;
if let Some(sender) = sender {
Expand Down
14 changes: 9 additions & 5 deletions rust/perspective-viewer/src/rust/js/plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,11 @@ extern "C" {
#[wasm_bindgen(method, catch)]
pub fn column_style_controls(this: &JsPerspectiveViewerPlugin, view_type: &str, group: Option<&str>) -> ApiResult<JsValue>;

#[wasm_bindgen(method)]
pub fn save(this: &JsPerspectiveViewerPlugin) -> JsValue;
#[wasm_bindgen(method, catch)]
pub fn save(this: &JsPerspectiveViewerPlugin) -> ApiResult<JsValue>;

#[wasm_bindgen(method, js_name=restore)]
pub fn _restore(this: &JsPerspectiveViewerPlugin, token: &JsValue, columns_config: &JsValue);
#[wasm_bindgen(method, js_name=restore, catch)]
pub fn _restore(this: &JsPerspectiveViewerPlugin, token: &JsValue, columns_config: &JsValue) -> ApiResult<()>;

#[wasm_bindgen(method)]
pub fn delete(this: &JsPerspectiveViewerPlugin);
Expand Down Expand Up @@ -103,7 +103,11 @@ extern "C" {
}

impl JsPerspectiveViewerPlugin {
pub fn restore(&self, token: &JsValue, columns_config: Option<&ColumnConfigMap>) {
pub fn restore(
&self,
token: &JsValue,
columns_config: Option<&ColumnConfigMap>,
) -> ApiResult<()> {
let columns_config = JsValue::from_serde_ext(&columns_config).unwrap();
self._restore(token, &columns_config)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ pub trait GetViewerConfigModel: HasSession + HasRenderer + HasPresentation {
let js_plugin = renderer.get_active_plugin()?;
let settings = presentation.is_settings_open();
let plugin = js_plugin.name();
let plugin_config: serde_json::Value = js_plugin.save().into_serde_ext()?;
let plugin_config: serde_json::Value = js_plugin.save()?.into_serde_ext()?;
let theme = presentation.get_selected_theme_name().await;
let title = presentation.get_title();
let columns_config = presentation.all_columns_configs();
Expand Down
4 changes: 2 additions & 2 deletions rust/perspective-viewer/src/rust/model/restore_and_render.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,12 @@ pub trait RestoreAndRender: HasRenderer + HasSession + HasPresentation {
let plugin_update = if let Some(x) = plugin_config {
wasm_bindgen::JsValue::from_serde_ext(&*x).unwrap()
} else {
plugin.save()
plugin.save()?
};

presentation.update_columns_configs(columns_config);
let columns_config = presentation.all_columns_configs();
plugin.restore(&plugin_update, Some(&columns_config));
plugin.restore(&plugin_update, Some(&columns_config))?;
session.validate().await?.create_view().await
});

Expand Down
6 changes: 4 additions & 2 deletions rust/perspective-viewer/src/rust/renderer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,13 @@ impl Renderer {
}))
}

pub async fn reset(&self, columns_config: Option<&ColumnConfigMap>) {
pub async fn reset(&self, columns_config: Option<&ColumnConfigMap>) -> ApiResult<()> {
self.0.borrow_mut().plugins_idx = None;
if let Ok(plugin) = self.get_active_plugin() {
plugin.restore(&json!({}), columns_config);
plugin.restore(&json!({}), columns_config)?;
}

Ok(())
}

pub fn delete(&self) -> ApiResult<()> {
Expand Down
Binary file modified tools/perspective-test/results.tar.gz
Binary file not shown.

0 comments on commit 126a6a1

Please sign in to comment.