forked from 1b-agent/opencadstudio-upstream
refactor(ribbon): make StyleContext borrowed to avoid 8 clones per view
Replace the 8 owned clones in StyleContext construction (4 Vec<String> + 4 String) at ribbon/mod.rs:568 and :1059 with borrowed refs. StyleContext is now StyleContext<'a> with &'a [String]/&'a str (widgets.rs:313), RenderCtx holds &'a StyleContext<'a> (:633), and call sites pass &self.x instead of .clone(). The view-local borrow is valid for the duration of Ribbon::view() and the style dropdown overlay. TDD: RED style_context_tests failed E0308 (owned vs borrowed), GREEN added lifetime and updated 4 type sites + 2 construction sites.
This commit is contained in:
1 parent
fddf505c20
commit
50cb21e4d4
2 files changed
+90
-37
No files matched your search
+18
-18
@@ -566,14 +566,14 @@ impl Ribbon {
|
|||||||
if let Some(module) = self.modules.get(effective_active) {
|
if let Some(module) = self.modules.get(effective_active) {
|
||||||
let groups = module.ribbon_groups();
|
let groups = module.ribbon_groups();
|
||||||
let style_ctx = StyleContext {
|
let style_ctx = StyleContext {
|
||||||
text_style_names: self.text_style_names.clone(),
|
text_style_names: &self.text_style_names,
|
||||||
active_text_style: self.active_text_style.clone(),
|
active_text_style: &self.active_text_style,
|
||||||
dim_style_names: self.dim_style_names.clone(),
|
dim_style_names: &self.dim_style_names,
|
||||||
active_dim_style: self.active_dim_style.clone(),
|
active_dim_style: &self.active_dim_style,
|
||||||
mleader_style_names: self.mleader_style_names.clone(),
|
mleader_style_names: &self.mleader_style_names,
|
||||||
active_mleader_style: self.active_mleader_style.clone(),
|
active_mleader_style: &self.active_mleader_style,
|
||||||
table_style_names: self.table_style_names.clone(),
|
table_style_names: &self.table_style_names,
|
||||||
active_table_style: self.active_table_style.clone(),
|
active_table_style: &self.active_table_style,
|
||||||
};
|
};
|
||||||
|
|
||||||
// Adaptive tool area: panels sit on one row; when they don't all
|
// Adaptive tool area: panels sit on one row; when they don't all
|
||||||
@@ -1057,14 +1057,14 @@ impl Ribbon {
|
|||||||
let (style_key, manager_cmd) = found?;
|
let (style_key, manager_cmd) = found?;
|
||||||
|
|
||||||
let ctx = StyleContext {
|
let ctx = StyleContext {
|
||||||
text_style_names: self.text_style_names.clone(),
|
text_style_names: &self.text_style_names,
|
||||||
active_text_style: self.active_text_style.clone(),
|
active_text_style: &self.active_text_style,
|
||||||
dim_style_names: self.dim_style_names.clone(),
|
dim_style_names: &self.dim_style_names,
|
||||||
active_dim_style: self.active_dim_style.clone(),
|
active_dim_style: &self.active_dim_style,
|
||||||
mleader_style_names: self.mleader_style_names.clone(),
|
mleader_style_names: &self.mleader_style_names,
|
||||||
active_mleader_style: self.active_mleader_style.clone(),
|
active_mleader_style: &self.active_mleader_style,
|
||||||
table_style_names: self.table_style_names.clone(),
|
table_style_names: &self.table_style_names,
|
||||||
active_table_style: self.active_table_style.clone(),
|
active_table_style: &self.active_table_style,
|
||||||
};
|
};
|
||||||
let active = ctx.active_for(style_key).to_string();
|
let active = ctx.active_for(style_key).to_string();
|
||||||
|
|
||||||
@@ -1298,7 +1298,7 @@ fn render_group<'a>(
|
|||||||
active_color: AcadColor,
|
active_color: AcadColor,
|
||||||
active_linetype: &'a str,
|
active_linetype: &'a str,
|
||||||
active_lineweight: LineWeight,
|
active_lineweight: LineWeight,
|
||||||
style_ctx: &StyleContext,
|
style_ctx: &StyleContext<'_>,
|
||||||
) -> Element<'a, Message> {
|
) -> Element<'a, Message> {
|
||||||
let mut items_row: Vec<Element<Message>> = Vec::new();
|
let mut items_row: Vec<Element<Message>> = Vec::new();
|
||||||
let mut small_buf: Vec<Element<Message>> = Vec::new();
|
let mut small_buf: Vec<Element<Message>> = Vec::new();
|
||||||
@@ -1419,7 +1419,7 @@ fn collapse_button<'a>(
|
|||||||
active_color: AcadColor,
|
active_color: AcadColor,
|
||||||
active_linetype: &'a str,
|
active_linetype: &'a str,
|
||||||
active_lineweight: LineWeight,
|
active_lineweight: LineWeight,
|
||||||
style_ctx: &StyleContext,
|
style_ctx: &StyleContext<'_>,
|
||||||
// When set, this is the TIGHT step (reached once even the all-collapsed row
|
// When set, this is the TIGHT step (reached once even the all-collapsed row
|
||||||
// overflows): the whole panel becomes a single non-running button — the first
|
// overflows): the whole panel becomes a single non-running button — the first
|
||||||
// tool's icon + a ▾ that opens the tools flyout. Nothing runs directly here.
|
// tool's icon + a ▾ that opens the tools flyout. Nothing runs directly here.
|
||||||
|
|||||||
+72
-19
@@ -310,32 +310,32 @@ pub(super) const PROP_LW_ID: &str = "PROP_LW";
|
|||||||
|
|
||||||
// ── Style context (passed from Ribbon to render_large) ────────────────────
|
// ── Style context (passed from Ribbon to render_large) ────────────────────
|
||||||
|
|
||||||
pub(super) struct StyleContext {
|
pub(super) struct StyleContext<'a> {
|
||||||
pub text_style_names: Vec<String>,
|
pub text_style_names: &'a [String],
|
||||||
pub active_text_style: String,
|
pub active_text_style: &'a str,
|
||||||
pub dim_style_names: Vec<String>,
|
pub dim_style_names: &'a [String],
|
||||||
pub active_dim_style: String,
|
pub active_dim_style: &'a str,
|
||||||
pub mleader_style_names: Vec<String>,
|
pub mleader_style_names: &'a [String],
|
||||||
pub active_mleader_style: String,
|
pub active_mleader_style: &'a str,
|
||||||
pub table_style_names: Vec<String>,
|
pub table_style_names: &'a [String],
|
||||||
pub active_table_style: String,
|
pub active_table_style: &'a str,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl StyleContext {
|
impl<'a> StyleContext<'a> {
|
||||||
pub(super) fn names_for(&self, key: StyleKey) -> &[String] {
|
pub(super) fn names_for(&self, key: StyleKey) -> &[String] {
|
||||||
match key {
|
match key {
|
||||||
StyleKey::TextStyle => &self.text_style_names,
|
StyleKey::TextStyle => self.text_style_names,
|
||||||
StyleKey::DimStyle => &self.dim_style_names,
|
StyleKey::DimStyle => self.dim_style_names,
|
||||||
StyleKey::MLeaderStyle => &self.mleader_style_names,
|
StyleKey::MLeaderStyle => self.mleader_style_names,
|
||||||
StyleKey::TableStyle => &self.table_style_names,
|
StyleKey::TableStyle => self.table_style_names,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
pub(super) fn active_for(&self, key: StyleKey) -> &str {
|
pub(super) fn active_for(&self, key: StyleKey) -> &str {
|
||||||
match key {
|
match key {
|
||||||
StyleKey::TextStyle => &self.active_text_style,
|
StyleKey::TextStyle => self.active_text_style,
|
||||||
StyleKey::DimStyle => &self.active_dim_style,
|
StyleKey::DimStyle => self.active_dim_style,
|
||||||
StyleKey::MLeaderStyle => &self.active_mleader_style,
|
StyleKey::MLeaderStyle => self.active_mleader_style,
|
||||||
StyleKey::TableStyle => &self.active_table_style,
|
StyleKey::TableStyle => self.active_table_style,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -630,7 +630,7 @@ pub(super) struct RenderCtx<'a> {
|
|||||||
pub active_color: AcadColor,
|
pub active_color: AcadColor,
|
||||||
pub active_linetype: &'a str,
|
pub active_linetype: &'a str,
|
||||||
pub active_lineweight: LineWeight,
|
pub active_lineweight: LineWeight,
|
||||||
pub style_ctx: &'a StyleContext,
|
pub style_ctx: &'a StyleContext<'a>,
|
||||||
/// When compact, the Properties panel's Match button shrinks to a small icon.
|
/// When compact, the Properties panel's Match button shrinks to a small icon.
|
||||||
pub compact: bool,
|
pub compact: bool,
|
||||||
}
|
}
|
||||||
@@ -1274,3 +1274,56 @@ pub(super) fn top_hist_btn_style(
|
|||||||
snap: false,
|
snap: false,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
mod style_context_tests {
|
||||||
|
use super::StyleContext;
|
||||||
|
use crate::modules::StyleKey;
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn style_context_borrowed_without_cloning() {
|
||||||
|
// RED: should construct from borrowed slices/strs without cloning.
|
||||||
|
let text_names = vec!["Standard".to_string(), "MyStyle".to_string()];
|
||||||
|
let active_text = "Standard".to_string();
|
||||||
|
let dim_names = vec!["DimStd".to_string()];
|
||||||
|
let active_dim = "DimStd".to_string();
|
||||||
|
let mleader_names: Vec<String> = vec![];
|
||||||
|
let active_mleader = String::new();
|
||||||
|
let table_names: Vec<String> = vec![];
|
||||||
|
let active_table = String::new();
|
||||||
|
|
||||||
|
let ctx = StyleContext {
|
||||||
|
text_style_names: &text_names,
|
||||||
|
active_text_style: &active_text,
|
||||||
|
dim_style_names: &dim_names,
|
||||||
|
active_dim_style: &active_dim,
|
||||||
|
mleader_style_names: &mleader_names,
|
||||||
|
active_mleader_style: &active_mleader,
|
||||||
|
table_style_names: &table_names,
|
||||||
|
active_table_style: &active_table,
|
||||||
|
};
|
||||||
|
assert_eq!(ctx.names_for(StyleKey::TextStyle), &text_names);
|
||||||
|
assert_eq!(ctx.active_for(StyleKey::TextStyle), "Standard");
|
||||||
|
assert_eq!(ctx.names_for(StyleKey::DimStyle), &dim_names);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn style_context_no_clone_needed() {
|
||||||
|
let names = vec!["A".to_string(), "B".to_string()];
|
||||||
|
let active = "A".to_string();
|
||||||
|
let empty: Vec<String> = vec![];
|
||||||
|
let empty_s = String::new();
|
||||||
|
let ctx = StyleContext {
|
||||||
|
text_style_names: &names,
|
||||||
|
active_text_style: &active,
|
||||||
|
dim_style_names: &empty,
|
||||||
|
active_dim_style: &empty_s,
|
||||||
|
mleader_style_names: &empty,
|
||||||
|
active_mleader_style: &empty_s,
|
||||||
|
table_style_names: &empty,
|
||||||
|
active_table_style: &empty_s,
|
||||||
|
};
|
||||||
|
assert_eq!(ctx.active_for(StyleKey::TableStyle), "");
|
||||||
|
assert!(ctx.names_for(StyleKey::MLeaderStyle).is_empty());
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in new issue
Block a user