From b8b2547aab22d50464cf6addc9de06147f47273b Mon Sep 17 00:00:00 2001 From: RunasSudo Date: Tue, 27 May 2025 00:21:17 +1000 Subject: [PATCH] Refactor DynamicReport to use RefCell Allows calculations to refer to the results of previous calculations Rather than the same cloned DynamicReport being passed to all calculations --- src/reporting/dynamic_report.rs | 119 +++++++++++++++++++++++++------- src/reporting/steps.rs | 52 +++++++------- 2 files changed, 121 insertions(+), 50 deletions(-) diff --git a/src/reporting/dynamic_report.rs b/src/reporting/dynamic_report.rs index f08276a..7b036d8 100644 --- a/src/reporting/dynamic_report.rs +++ b/src/reporting/dynamic_report.rs @@ -16,6 +16,7 @@ along with this program. If not, see . */ +use std::cell::RefCell; use std::collections::HashMap; use serde::{Deserialize, Serialize}; @@ -29,13 +30,22 @@ use super::types::{GenericReportingProduct, ReportingProduct}; pub struct DynamicReport { pub title: String, pub columns: Vec, - pub entries: Vec, + // This must use RefCell as, during calculation, we iterate while mutating the report + pub entries: Vec>, } impl DynamicReport { + pub fn new(title: String, columns: Vec, entries: Vec) -> Self { + Self { + title, + columns, + entries: entries.into_iter().map(|e| RefCell::new(e)).collect(), + } + } + /// Remove all entries from the report where auto_hide is enabled and quantity is zero pub fn auto_hide(&mut self) { - self.entries.retain_mut(|e| match e { + self.entries.retain(|e| match &mut *e.borrow_mut() { DynamicReportEntry::Section(section) => { section.auto_hide_children(); if section.can_auto_hide_self() { @@ -58,15 +68,26 @@ impl DynamicReport { /// Recursively calculate all [CalculatedRow] entries pub fn calculate(&mut self) { - // FIXME: This is for the borrow checker - can it be avoided? - let report_cloned = self.clone(); + for (entry_idx, entry) in self.entries.iter().enumerate() { + let entry_ref = entry.borrow(); - for entry in self.entries.iter_mut() { - match entry { - DynamicReportEntry::Section(section) => section.calculate(&report_cloned), + match &*entry_ref { + DynamicReportEntry::Section(section) => { + // Clone first, in case calculation needs to take reference to the section + let mut updated_section = section.clone(); + updated_section.calculate(&self); + + drop(entry_ref); // Drop entry_ref so we can borrow mutably + let mut entry_mut = self.entries[entry_idx].borrow_mut(); + *entry_mut = DynamicReportEntry::Section(updated_section); + } DynamicReportEntry::LiteralRow(_) => (), DynamicReportEntry::CalculatedRow(row) => { - *entry = DynamicReportEntry::LiteralRow(row.calculate(&report_cloned)); + let updated_row = row.calculate(&self); + + drop(entry_ref); // Drop entry_ref so we can borrow mutably + let mut entry_mut = self.entries[entry_idx].borrow_mut(); + *entry_mut = DynamicReportEntry::LiteralRow(updated_row); } DynamicReportEntry::Spacer => (), } @@ -74,13 +95,18 @@ impl DynamicReport { } /// Look up [DynamicReportEntry] by id - pub fn by_id(&self, id: &str) -> Option<&DynamicReportEntry> { + /// + /// Returns a cloned copy of the [DynamicReportEntry]. This is necessary because the entry may be within a [Section], and [RefCell] semantics cannot express this type of nested borrow. + pub fn by_id(&self, id: &str) -> Option { + // Manually iterate over self.entries rather than self.entries() + // To catch the situation where entry is already mutably borrowed for entry in self.entries.iter() { - match entry { + match entry.try_borrow() { + Ok(entry) => match &*entry { DynamicReportEntry::Section(section) => { if let Some(i) = §ion.id { if i == id { - return Some(entry); + return Some(entry.clone()); } } if let Some(e) = section.by_id(id) { @@ -90,12 +116,17 @@ impl DynamicReport { DynamicReportEntry::LiteralRow(row) => { if let Some(i) = &row.id { if i == id { - return Some(entry); + return Some(entry.clone()); } } } DynamicReportEntry::CalculatedRow(_) => (), DynamicReportEntry::Spacer => (), + }, + Err(err) => panic!( + "Attempt to call by_id on DynamicReportEntry which is mutably borrowed: {}", + err + ), } } @@ -136,12 +167,28 @@ pub struct Section { pub id: Option, pub visible: bool, pub auto_hide: bool, - pub entries: Vec, + pub entries: Vec>, } impl Section { + pub fn new( + text: String, + id: Option, + visible: bool, + auto_hide: bool, + entries: Vec, + ) -> Self { + Self { + text, + id, + visible, + auto_hide, + entries: entries.into_iter().map(|e| RefCell::new(e)).collect(), + } + } + fn auto_hide_children(&mut self) { - self.entries.retain_mut(|e| match e { + self.entries.retain(|e| match &mut *e.borrow_mut() { DynamicReportEntry::Section(section) => { section.auto_hide_children(); if section.can_auto_hide_self() { @@ -164,7 +211,7 @@ impl Section { fn can_auto_hide_self(&self) -> bool { self.auto_hide - && self.entries.iter().all(|e| match e { + && self.entries.iter().all(|e| match &*e.borrow() { DynamicReportEntry::Section(section) => section.can_auto_hide_self(), DynamicReportEntry::LiteralRow(row) => row.can_auto_hide(), DynamicReportEntry::CalculatedRow(_) => false, @@ -174,12 +221,26 @@ impl Section { /// Recursively calculate all [CalculatedRow] entries pub fn calculate(&mut self, report: &DynamicReport) { - for entry in self.entries.iter_mut() { - match entry { - DynamicReportEntry::Section(section) => section.calculate(report), + for (entry_idx, entry) in self.entries.iter().enumerate() { + let entry_ref = entry.borrow(); + + match &*entry_ref { + DynamicReportEntry::Section(section) => { + // Clone first, in case calculation needs to take reference to the section + let mut updated_section = section.clone(); + updated_section.calculate(&report); + + drop(entry_ref); // Drop entry_ref so we can borrow mutably + let mut entry_mut = self.entries[entry_idx].borrow_mut(); + *entry_mut = DynamicReportEntry::Section(updated_section); + } DynamicReportEntry::LiteralRow(_) => (), DynamicReportEntry::CalculatedRow(row) => { - *entry = DynamicReportEntry::LiteralRow(row.calculate(report)) + let updated_row = row.calculate(&report); + + drop(entry_ref); // Drop entry_ref so we can borrow mutably + let mut entry_mut = self.entries[entry_idx].borrow_mut(); + *entry_mut = DynamicReportEntry::LiteralRow(updated_row); } DynamicReportEntry::Spacer => (), } @@ -187,13 +248,18 @@ impl Section { } /// Look up [DynamicReportEntry] by id - pub fn by_id(&self, id: &str) -> Option<&DynamicReportEntry> { + /// + /// Returns a cloned copy of the [DynamicReportEntry]. + pub fn by_id(&self, id: &str) -> Option { + // Manually iterate over self.entries rather than self.entries() + // To catch the situation where entry is already mutably borrowed for entry in self.entries.iter() { - match entry { + match entry.try_borrow() { + Ok(entry) => match &*entry { DynamicReportEntry::Section(section) => { if let Some(i) = §ion.id { if i == id { - return Some(entry); + return Some(entry.clone()); } } if let Some(e) = section.by_id(id) { @@ -203,12 +269,17 @@ impl Section { DynamicReportEntry::LiteralRow(row) => { if let Some(i) = &row.id { if i == id { - return Some(entry); + return Some(entry.clone()); } } } DynamicReportEntry::CalculatedRow(_) => (), DynamicReportEntry::Spacer => (), + }, + Err(err) => panic!( + "Attempt to call by_id on DynamicReportEntry which is mutably borrowed: {}", + err + ), } } @@ -219,7 +290,7 @@ impl Section { pub fn subtotal(&self, report: &DynamicReport) -> Vec { let mut subtotals = vec![0; report.columns.len()]; for entry in self.entries.iter() { - match entry { + match &*entry.borrow() { DynamicReportEntry::Section(section) => { for (col_idx, subtotal) in section.subtotal(report).into_iter().enumerate() { subtotals[col_idx] += subtotal; diff --git a/src/reporting/steps.rs b/src/reporting/steps.rs index db9a3fb..b5a2f70 100644 --- a/src/reporting/steps.rs +++ b/src/reporting/steps.rs @@ -372,16 +372,16 @@ impl ReportingStep for BalanceSheet { kinds_for_account(context.db_connection.get_account_configurations()); // Init report - let mut report = DynamicReport { - title: "Balance sheet".to_string(), - columns: self.args.dates.iter().map(|d| d.date.to_string()).collect(), - entries: vec![ - DynamicReportEntry::Section(Section { - text: "Assets".to_string(), - id: Some("assets".to_string()), - visible: true, - auto_hide: false, - entries: { + let mut report = DynamicReport::new( + "Balance sheet".to_string(), + self.args.dates.iter().map(|d| d.date.to_string()).collect(), + vec![ + DynamicReportEntry::Section(Section::new( + "Assets".to_string(), + Some("assets".to_string()), + true, + false, + { let mut entries = entries_for_kind("drcr.asset", false, &balances, &kinds_for_account); entries.push(DynamicReportEntry::CalculatedRow(CalculatedRow { @@ -398,14 +398,14 @@ impl ReportingStep for BalanceSheet { })); entries }, - }), + )), DynamicReportEntry::Spacer, - DynamicReportEntry::Section(Section { - text: "Liabilities".to_string(), - id: Some("liabilities".to_string()), - visible: true, - auto_hide: false, - entries: { + DynamicReportEntry::Section(Section::new( + "Liabilities".to_string(), + Some("liabilities".to_string()), + true, + false, + { let mut entries = entries_for_kind("drcr.liability", true, &balances, &kinds_for_account); entries.push(DynamicReportEntry::CalculatedRow(CalculatedRow { @@ -422,14 +422,14 @@ impl ReportingStep for BalanceSheet { })); entries }, - }), + )), DynamicReportEntry::Spacer, - DynamicReportEntry::Section(Section { - text: "Equity".to_string(), - id: Some("equity".to_string()), - visible: true, - auto_hide: false, - entries: { + DynamicReportEntry::Section(Section::new( + "Equity".to_string(), + Some("equity".to_string()), + true, + false, + { let mut entries = entries_for_kind("drcr.equity", true, &balances, &kinds_for_account); entries.push(DynamicReportEntry::CalculatedRow(CalculatedRow { @@ -446,9 +446,9 @@ impl ReportingStep for BalanceSheet { })); entries }, - }), + )), ], - }; + ); report.calculate(); report.auto_hide();