Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor How Column Style Components Are Selected #2443

Merged
merged 6 commits into from
Dec 31, 2023
Merged

Refactor How Column Style Components Are Selected #2443

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
178c1b7
fix datagrid expression warning
ada-x64 Nov 22, 2023
8a1af5a
Add fixture to root out any bad logs
ada-x64 Nov 22, 2023
b7d0ab8
add global timeout
ada-x64 Dec 1, 2023
f18a68f
refactor how column style components are selected
ada-x64 Nov 20, 2023
316ec7d
Hide symbol col settings based on view type
ada-x64 Nov 20, 2023
33220af
rm expect_throws and add stub
ada-x64 Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor how column style components are selected
  • Loading branch information
@ada-x64
ada-x64 committed Dec 5, 2023
commit f18a68f5685b0bd9fc023bf47b2f787ca0170dba
33 changes: 21 additions & 12 deletions rust/perspective-viewer/src/rust/components/column_settings_sidebar/sidebar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

use std::fmt::Display;

use wasm_bindgen::UnwrapThrowExt;
use yew::{function_component, html, use_callback, use_state, Callback, Html, Properties};

use crate::components::column_settings_sidebar::attributes_tab::AttributesTab;
Expand Down Expand Up @@ -79,25 +80,30 @@ pub fn ColumnSettingsSidebar(p: &ColumnSettingsProps) -> Html {
attrs
);
}
let maybe_ty = p.session.metadata().get_column_view_type(&column_name);
// view_ty != table_ty when aggregate is applied, i.e. on group-by
let maybe_view_ty = p.session.metadata().get_column_view_type(&column_name);
let maybe_table_ty = p.session.metadata().get_column_table_type(&column_name);
let (view_ty, table_ty) = maybe_view_ty
.zip(maybe_table_ty)
.expect_throw("Unable to get view and table types!");

let mut tabs = vec![];

// TODO: This is a hack and needs to be replaced.
// TODO: This needs to be replaced. Replacing it requires more information
// about the capabilities of the plugin, which requires updating the internal
// plugin API. Leaving it for now.
let plugin = p.renderer.get_active_plugin().unwrap();
let show_styles = maybe_ty
.map(|ty| match &*plugin.name() {
"Datagrid" => ty != Type::Bool,
"X/Y Scatter" => ty == Type::String,
_ => false,
})
.unwrap_or_default();
let show_styles = match &*plugin.name() {
"Datagrid" => view_ty != Type::Bool,
"X/Y Scatter" => table_ty == Type::String,
_ => false,
};

if !matches!(p.selected_column, ColumnLocator::Expr(None))
&& show_styles
&& is_active
&& config.is_some()
&& maybe_ty.is_some()
&& maybe_view_ty.is_some()
{
tabs.push(ColumnSettingsTab::Style);
}
Expand Down Expand Up @@ -132,7 +138,8 @@ pub fn ColumnSettingsSidebar(p: &ColumnSettingsProps) -> Html {
{ renderer }
{ custom_events }
{ selected_column }
{ on_close }/>
{ on_close }
/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to sync up on style guide for html! macro content, because it is immune to rustfmt and changes like these make it difficult for me to use file history to bisect issues.

}
},
ColumnSettingsTab::Style => html! {
Expand All @@ -141,7 +148,9 @@ pub fn ColumnSettingsSidebar(p: &ColumnSettingsProps) -> Html {
{ renderer }
{ custom_events }
{ column_name }
ty={ maybe_ty.unwrap() }/>
{ view_ty }
{ table_ty }
/>
},
}
})
Expand Down
71 changes: 63 additions & 8 deletions rust/perspective-viewer/src/rust/components/column_settings_sidebar/style_tab.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,35 +13,90 @@
mod column_style;
mod symbol;

use wasm_bindgen::UnwrapThrowExt;
use yew::{function_component, html, Html, Properties};

use crate::components::column_settings_sidebar::style_tab::column_style::ColumnStyle;
use crate::components::column_settings_sidebar::style_tab::symbol::SymbolStyle;
use crate::config::Type;
use crate::custom_events::CustomEvents;
use crate::renderer::Renderer;
use crate::session::Session;
use crate::utils::JsValueSerdeExt;

#[derive(Clone, PartialEq, Properties)]
pub struct StyleTabProps {
pub custom_events: CustomEvents,
pub session: Session,
pub renderer: Renderer,

pub ty: Type,
pub table_ty: Type,
pub view_ty: Type,
pub column_name: String,
}

#[function_component]
pub fn StyleTab(p: &StyleTabProps) -> Html {
pub fn StyleTab(props: &StyleTabProps) -> Html {
let plugin = props
.renderer
.get_active_plugin()
.expect("No active plugins!");

let plugin_column_names = plugin
.config_column_names()
.and_then(|arr| arr.into_serde_ext::<Vec<Option<String>>>().ok())
.expect_throw("Could not deserialize config_column_names into Vec<Option<String>>");

let view_config = props.session.get_view_config();
let (col_idx, _) = view_config
.columns
.iter()
.enumerate()
.find(|(_, opt)| {
opt.as_ref()
.map(|s| s == &props.column_name)
.unwrap_or_default()
})
.expect_throw(&format!(
"Could not find column with name {:?}",
props.column_name
));

let col_grp = plugin_column_names
.iter()
// This mimics the behavior where plugins can take a single value
// per column grouping, and any overflow falls into the final heading
.chain(std::iter::repeat(plugin_column_names.last().unwrap()))
.nth(col_idx)
.unwrap()
.to_owned()
.expect_throw(&format!("Could not get column group for index {col_idx:?}"));

// TODO: We need a better way to determine which styling components to show.
// This will come with future changes to the plugin API.
let components = match col_grp.as_str() {
"Symbol" => Some(html! {
<SymbolStyle
column_name={ props.column_name.clone() }
session={ &props.session }
renderer={ &props.renderer }
custom_events={ &props.custom_events }/>
}),
"Columns" => Some(html! {
<ColumnStyle
custom_events={ props.custom_events.clone() }
session={ props.session.clone() }
renderer={ props.renderer.clone() }
view_ty={ props.view_ty }
column_name={ props.column_name.clone() }/>
}),
_ => None,
};

html! {
<div id="style-tab">
<div id="column-style-container" class="tab-section">
<ColumnStyle
custom_events={ p.custom_events.clone() }
session={ p.session.clone() }
renderer={ p.renderer.clone() }
ty={ p.ty }
column_name={ p.column_name.clone() }/>
{components}
</div>
</div>
}
Expand Down
75 changes: 9 additions & 66 deletions .../perspective-viewer/src/rust/components/column_settings_sidebar/style_tab/column_style.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@
// ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛

use serde::de::DeserializeOwned;
use wasm_bindgen::UnwrapThrowExt;
use yew::{function_component, html, Callback, Html, Properties};

use crate::components::column_settings_sidebar::style_tab::symbol::SymbolAttr;
use crate::components::datetime_column_style::DatetimeColumnStyle;
use crate::components::number_column_style::NumberColumnStyle;
use crate::components::string_column_style::StringColumnStyle;
Expand All @@ -24,7 +24,6 @@ use crate::custom_events::CustomEvents;
use crate::model::*;
use crate::renderer::Renderer;
use crate::session::Session;
use crate::utils::*;
use crate::{clone, css, derive_model, html_template};

/// This function retrieves the plugin's config using its `save` method.
Expand Down Expand Up @@ -69,7 +68,7 @@ pub struct ColumnStyleProps {
pub session: Session,
pub renderer: Renderer,

pub ty: Type,
pub view_ty: Type,
pub column_name: String,
}
derive_model!(CustomEvents, Session, Renderer for ColumnStyleProps);
Expand All @@ -78,13 +77,13 @@ pub fn ColumnStyle(p: &ColumnStyleProps) -> Html {
let props = p.clone();
let (config, attrs) = (props.get_plugin_config(), props.get_plugin_attrs());
let (config, attrs) = (config.unwrap(), attrs.unwrap());
let title = format!("{} Styling", props.ty.to_capitalized());
let opt_html = match props.ty {
let title = format!("{} Styling", props.view_ty.to_capitalized());
let opt_html = match props.view_ty {
Type::String => get_column_style::<_, StringColumnStyleDefaultConfig>(
config.clone(),
attrs.clone(),
&props.column_name,
props.ty,
props.view_ty,
)
.map(|(config, default_config)| {
let on_change = Callback::from(move |config| {
Expand All @@ -109,10 +108,10 @@ pub fn ColumnStyle(p: &ColumnStyleProps) -> Html {
config.clone(),
attrs.clone(),
&props.column_name,
props.ty,
props.view_ty,
)
.map(|(config, default_config)| {
let enable_time_config = matches!(props.ty, Type::Datetime);
let enable_time_config = matches!(props.view_ty, Type::Datetime);
let on_change = Callback::from(move |config| {
props.send_plugin_config(
props.column_name.clone(),
Expand All @@ -135,7 +134,7 @@ pub fn ColumnStyle(p: &ColumnStyleProps) -> Html {
config.clone(),
attrs.clone(),
&props.column_name,
props.ty,
props.view_ty,
)
.map(|(config, default_config)| {
let on_change = {
Expand Down Expand Up @@ -163,64 +162,8 @@ pub fn ColumnStyle(p: &ColumnStyleProps) -> Html {
_ => Err("Booleans aren't styled yet.".into()),
};

let props = p;
let plugin = props
.renderer
.get_active_plugin()
.expect("No active plugins!");

let plugin_column_names = plugin
.config_column_names()
.map(|arr| {
arr.into_serde_ext::<Vec<Option<String>>>()
.expect("Could not deserialize config_column_names into Vec<Option<String>>")
})
.unwrap_or_default();

let view_config = props.session.get_view_config();
let mut is_symbol = false;
let components = plugin_column_names
.iter()
.zip(&view_config.columns)
.filter_map(|(ty, name)| {
let attr_type = props
.session
.metadata()
.get_column_view_type(&props.column_name)
.expect("Couldn't get column type for {column_name}");

let zipped = ty.as_deref().zip(name.as_deref());
match zipped {
Some(("Symbol", name)) if name == props.column_name && p.ty == Type::String => {
is_symbol = true;
Some(html! {
<SymbolAttr
{ attr_type }
column_name={ props.column_name.clone() }
session={ &props.session }
renderer={ &props.renderer }
custom_events={ &props.custom_events }/>
})
},
_ => None,
}
})
.collect::<Html>();

let is_ok = opt_html.is_ok();
html_template! {
<LocalStyle href={ css!("column-style") } />
if is_ok {
{ opt_html.unwrap() }
}

{ components }
if !is_ok && !is_symbol {
<div class="style_contents">
<div id="column-style-container" class="no-style">
<div class="style-contents">{ "No styles available" }</div>
</div>
</div>
}
{opt_html.expect_throw("Could not generate HTML to style this column!")}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errors are not the right way to handle this (or really in general), not even JavaScript errors. It is really hard to predict when something that is the result of a long complex process will adhere to restriction like this that cannot (or at least, isn't) be represented in the type system. More importantly, throwing an error here can corrupt/crash the rust app.

There is a similar circumstance in active_column.rs, where there is a small corner case where the View must exists before the UI can be rendered. This render path shouldn't be hit, so we render a stub.

In this case, we probably want to keep the stub, we just don't want it ever to show. If for some reason it does, we don't want to segfault, so it needs to render something because we can't (AFAIK) prevent this logic from being called with a missing view type (?). Probably also worth warning the console as this is programmer error.

}
}
6 changes: 2 additions & 4 deletions rust/perspective-viewer/src/rust/components/column_settings_sidebar/style_tab/symbol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ use self::types::{SymbolConfig, SymbolKVPair};
use crate::components::column_settings_sidebar::style_tab::symbol::symbol_pairs::PairsList;
use crate::components::style::LocalStyle;
use crate::config::plugin::{PluginConfig, Symbol};
use crate::config::Type;
use crate::custom_elements::FilterDropDownElement;
use crate::custom_events::CustomEvents;
use crate::model::{GetPluginConfig, UpdatePluginConfig};
Expand All @@ -40,7 +39,6 @@ pub fn next_default_symbol(values: &Vec<Symbol>, pairs_len: usize) -> String {
}
#[derive(Properties, PartialEq, Clone)]
pub struct SymbolAttrProps {
pub attr_type: Type,
pub column_name: String,
pub session: Session,
pub renderer: Renderer,
Expand All @@ -61,12 +59,12 @@ pub enum SymbolAttrMsg {
UpdatePairs(Vec<SymbolKVPair>),
}

pub struct SymbolAttr {
pub struct SymbolStyle {
pairs: Vec<SymbolKVPair>,
row_dropdown: Rc<FilterDropDownElement>,
}

impl yew::Component for SymbolAttr {
impl yew::Component for SymbolStyle {
type Message = SymbolAttrMsg;
type Properties = SymbolAttrProps;

Expand Down