From f1f9fd1c60141e7ea5253c0c5bceb3adcfa23539 Mon Sep 17 00:00:00 2001 From: AnotherTest Date: Sun, 20 Dec 2020 12:58:14 +0330 Subject: [PATCH] Spreadsheet: struct Cell => class Cell Hide private members, and make the odd update() -> sheet->update(cell) -> update(Badge) -> update_data() less odd by removing the update(Badge) step. --- Applications/Spreadsheet/Cell.cpp | 77 +++++++++++-------- Applications/Spreadsheet/Cell.h | 62 ++++++++------- Applications/Spreadsheet/CellType/Date.cpp | 4 +- Applications/Spreadsheet/CellType/Numeric.cpp | 4 +- Applications/Spreadsheet/CellType/String.cpp | 2 +- Applications/Spreadsheet/Spreadsheet.cpp | 44 +++++------ Applications/Spreadsheet/SpreadsheetModel.cpp | 8 +- Applications/Spreadsheet/main.cpp | 2 +- 8 files changed, 110 insertions(+), 93 deletions(-) diff --git a/Applications/Spreadsheet/Cell.cpp b/Applications/Spreadsheet/Cell.cpp index a9dc2e93000..4db2682a59c 100644 --- a/Applications/Spreadsheet/Cell.cpp +++ b/Applications/Spreadsheet/Cell.cpp @@ -33,32 +33,32 @@ namespace Spreadsheet { void Cell::set_data(String new_data) { - if (data == new_data) + if (m_data == new_data) return; if (new_data.starts_with("=")) { new_data = new_data.substring(1, new_data.length() - 1); - kind = Formula; + m_kind = Formula; } else { - kind = LiteralString; + m_kind = LiteralString; } - data = move(new_data); - dirty = true; - evaluated_externally = false; + m_data = move(new_data); + m_dirty = true; + m_evaluated_externally = false; } void Cell::set_data(JS::Value new_data) { - dirty = true; - evaluated_externally = true; + m_dirty = true; + m_evaluated_externally = true; StringBuilder builder; builder.append(new_data.to_string_without_side_effects()); - data = builder.build(); + m_data = builder.build(); - evaluated_data = move(new_data); + m_evaluated_data = move(new_data); } void Cell::set_type(const CellType* type) @@ -86,8 +86,8 @@ const CellType& Cell::type() const if (m_type) return *m_type; - if (kind == LiteralString) { - if (data.to_int().has_value()) + if (m_kind == LiteralString) { + if (m_data.to_int().has_value()) return *CellType::get_by_name("Numeric"); } @@ -104,22 +104,22 @@ JS::Value Cell::typed_js_data() const return type().js_value(const_cast(*this), m_type_metadata); } -void Cell::update_data() +void Cell::update_data(Badge) { - TemporaryChange cell_change { sheet->current_evaluated_cell(), this }; - if (!dirty) + TemporaryChange cell_change { m_sheet->current_evaluated_cell(), this }; + if (!m_dirty) return; - if (dirty) { - dirty = false; - if (kind == Formula) { - if (!evaluated_externally) - evaluated_data = sheet->evaluate(data, this); + if (m_dirty) { + m_dirty = false; + if (m_kind == Formula) { + if (!m_evaluated_externally) + m_evaluated_data = m_sheet->evaluate(m_data, this); } - for (auto& ref : referencing_cells) { + for (auto& ref : m_referencing_cells) { if (ref) { - ref->dirty = true; + ref->m_dirty = true; ref->update(); } } @@ -134,7 +134,7 @@ void Cell::update_data() builder.append("return ("); builder.append(fmt.condition); builder.append(')'); - auto value = sheet->evaluate(builder.string_view(), this); + auto value = m_sheet->evaluate(builder.string_view(), this); if (value.to_boolean()) { if (fmt.background_color.has_value()) m_evaluated_formats.background_color = fmt.background_color; @@ -147,26 +147,26 @@ void Cell::update_data() void Cell::update() { - sheet->update(*this); + m_sheet->update(*this); } JS::Value Cell::js_data() { - if (dirty) + if (m_dirty) update(); - if (kind == Formula) - return evaluated_data; + if (m_kind == Formula) + return m_evaluated_data; - return JS::js_string(sheet->interpreter().heap(), data); + return JS::js_string(m_sheet->interpreter().heap(), m_data); } String Cell::source() const { StringBuilder builder; - if (kind == Formula) + if (m_kind == Formula) builder.append('='); - builder.append(data); + builder.append(m_data); return builder.to_string(); } @@ -176,10 +176,23 @@ void Cell::reference_from(Cell* other) if (!other || other == this) return; - if (!referencing_cells.find([other](auto& ptr) { return ptr.ptr() == other; }).is_end()) + if (!m_referencing_cells.find([other](auto& ptr) { return ptr.ptr() == other; }).is_end()) return; - referencing_cells.append(other->make_weak_ptr()); + m_referencing_cells.append(other->make_weak_ptr()); +} + +void Cell::copy_from(const Cell& other) +{ + m_dirty = true; + m_evaluated_externally = other.m_evaluated_externally; + m_data = other.m_data; + m_evaluated_data = other.m_evaluated_data; + m_kind = other.m_kind; + m_type = other.m_type; + m_type_metadata = other.m_type_metadata; + m_conditional_formats = other.m_conditional_formats; + m_evaluated_formats = other.m_evaluated_formats; } } diff --git a/Applications/Spreadsheet/Cell.h b/Applications/Spreadsheet/Cell.h index 04c10a66c0f..19e63e1328d 100644 --- a/Applications/Spreadsheet/Cell.h +++ b/Applications/Spreadsheet/Cell.h @@ -38,21 +38,26 @@ namespace Spreadsheet { struct Cell : public Weakable { + enum Kind { + LiteralString, + Formula, + }; + Cell(String data, Position position, WeakPtr sheet) - : dirty(false) - , data(move(data)) - , kind(LiteralString) - , sheet(sheet) + : m_dirty(false) + , m_data(move(data)) + , m_kind(LiteralString) + , m_sheet(sheet) , m_position(move(position)) { } Cell(String source, JS::Value&& cell_value, Position position, WeakPtr sheet) - : dirty(false) - , data(move(source)) - , evaluated_data(move(cell_value)) - , kind(Formula) - , sheet(sheet) + : m_dirty(false) + , m_data(move(source)) + , m_evaluated_data(move(cell_value)) + , m_kind(Formula) + , m_sheet(sheet) , m_position(move(position)) { } @@ -61,6 +66,12 @@ struct Cell : public Weakable { void set_data(String new_data); void set_data(JS::Value new_data); + bool dirty() const { return m_dirty; } + + const String& data() const { return m_data; } + const JS::Value& evaluated_data() const { return m_evaluated_data; } + Kind kind() const { return m_kind; } + const Vector>& referencing_cells() const { return m_referencing_cells; } void set_type(const StringView& name); void set_type(const CellType*); @@ -69,15 +80,16 @@ struct Cell : public Weakable { const Position& position() const { return m_position; } void set_position(Position position, Badge) { - dirty = true; + m_dirty = true; m_position = move(position); } const Format& evaluated_formats() const { return m_evaluated_formats; } + Format& evaluated_formats() { return m_evaluated_formats; } const Vector& conditional_formats() const { return m_conditional_formats; } void set_conditional_formats(Vector&& fmts) { - dirty = true; + m_dirty = true; m_conditional_formats = move(fmts); } @@ -92,30 +104,28 @@ struct Cell : public Weakable { JS::Value js_data(); - void update(Badge) { update_data(); } void update(); + void update_data(Badge); - enum Kind { - LiteralString, - Formula, - }; + const Sheet& sheet() const { return *m_sheet; } + Sheet& sheet() { return *m_sheet; } - bool dirty { false }; - bool evaluated_externally { false }; - String data; - JS::Value evaluated_data; - Kind kind { LiteralString }; - WeakPtr sheet; - Vector> referencing_cells; + void copy_from(const Cell&); + +private: + bool m_dirty { false }; + bool m_evaluated_externally { false }; + String m_data; + JS::Value m_evaluated_data; + Kind m_kind { LiteralString }; + WeakPtr m_sheet; + Vector> m_referencing_cells; const CellType* m_type { nullptr }; CellTypeMetadata m_type_metadata; Position m_position; Vector m_conditional_formats; Format m_evaluated_formats; - -private: - void update_data(); }; } diff --git a/Applications/Spreadsheet/CellType/Date.cpp b/Applications/Spreadsheet/CellType/Date.cpp index c7a4ddc06d5..eb0937ed550 100644 --- a/Applications/Spreadsheet/CellType/Date.cpp +++ b/Applications/Spreadsheet/CellType/Date.cpp @@ -43,7 +43,7 @@ DateCell::~DateCell() String DateCell::display(Cell& cell, const CellTypeMetadata& metadata) const { auto timestamp = js_value(cell, metadata); - auto string = Core::DateTime::from_timestamp(timestamp.to_i32(cell.sheet->global_object())).to_string(metadata.format.is_empty() ? "%Y-%m-%d %H:%M:%S" : metadata.format.characters()); + auto string = Core::DateTime::from_timestamp(timestamp.to_i32(cell.sheet().global_object())).to_string(metadata.format.is_empty() ? "%Y-%m-%d %H:%M:%S" : metadata.format.characters()); if (metadata.length >= 0) return string.substring(0, metadata.length); @@ -53,7 +53,7 @@ String DateCell::display(Cell& cell, const CellTypeMetadata& metadata) const JS::Value DateCell::js_value(Cell& cell, const CellTypeMetadata&) const { - auto value = cell.js_data().to_double(cell.sheet->global_object()); + auto value = cell.js_data().to_double(cell.sheet().global_object()); return JS::Value(value / 1000); // Turn it to seconds } diff --git a/Applications/Spreadsheet/CellType/Numeric.cpp b/Applications/Spreadsheet/CellType/Numeric.cpp index 1863cbf24f0..b69ca1bf210 100644 --- a/Applications/Spreadsheet/CellType/Numeric.cpp +++ b/Applications/Spreadsheet/CellType/Numeric.cpp @@ -47,7 +47,7 @@ String NumericCell::display(Cell& cell, const CellTypeMetadata& metadata) const if (metadata.format.is_empty()) string = value.to_string_without_side_effects(); else - string = format_double(metadata.format.characters(), value.to_double(cell.sheet->global_object())); + string = format_double(metadata.format.characters(), value.to_double(cell.sheet().global_object())); if (metadata.length >= 0) return string.substring(0, metadata.length); @@ -57,7 +57,7 @@ String NumericCell::display(Cell& cell, const CellTypeMetadata& metadata) const JS::Value NumericCell::js_value(Cell& cell, const CellTypeMetadata&) const { - return cell.js_data().to_number(cell.sheet->global_object()); + return cell.js_data().to_number(cell.sheet().global_object()); } } diff --git a/Applications/Spreadsheet/CellType/String.cpp b/Applications/Spreadsheet/CellType/String.cpp index ad1b7dd95c4..ccda9947fea 100644 --- a/Applications/Spreadsheet/CellType/String.cpp +++ b/Applications/Spreadsheet/CellType/String.cpp @@ -51,7 +51,7 @@ String StringCell::display(Cell& cell, const CellTypeMetadata& metadata) const JS::Value StringCell::js_value(Cell& cell, const CellTypeMetadata& metadata) const { auto string = display(cell, metadata); - return JS::js_string(cell.sheet->interpreter().heap(), string); + return JS::js_string(cell.sheet().interpreter().heap(), string); } } diff --git a/Applications/Spreadsheet/Spreadsheet.cpp b/Applications/Spreadsheet/Spreadsheet.cpp index e26740de317..881dfb4abf1 100644 --- a/Applications/Spreadsheet/Spreadsheet.cpp +++ b/Applications/Spreadsheet/Spreadsheet.cpp @@ -60,8 +60,8 @@ Sheet::Sheet(Workbook& workbook) : m_workbook(workbook) { m_global_object = m_workbook.interpreter().heap().allocate_without_global_object(*this); - m_global_object->set_prototype(&m_workbook.global_object()); m_global_object->initialize(); + m_global_object->put("workbook", m_workbook.workbook_object()); m_global_object->put("thisSheet", m_global_object); // Self-reference is unfortunate, but required. // Sadly, these have to be evaluated once per sheet. @@ -160,26 +160,23 @@ void Sheet::update() for (auto& it : m_cells) cells_copy.append(it.value); - for (auto& cell : cells_copy) { - if (has_been_visited(cell)) - continue; - m_visited_cells_in_update.set(cell); - if (cell->dirty) { - // Re-evaluate the cell value, if any. - cell->update({}); - } - } + for (auto& cell : cells_copy) + update(*cell); m_visited_cells_in_update.clear(); } void Sheet::update(Cell& cell) { - if (has_been_visited(&cell)) - return; - - m_visited_cells_in_update.set(&cell); - cell.update({}); + if (cell.dirty()) { + if (has_been_visited(&cell)) { + // This may be part of an cyclic reference chain + // just break the chain, but leave the cell dirty. + return; + } + m_visited_cells_in_update.set(&cell); + cell.update_data({}); + } } JS::Value Sheet::evaluate(const StringView& source, Cell* on_behalf_of) @@ -323,10 +320,7 @@ void Sheet::copy_cells(Vector from, Vector to, Optional Sheet::from_json(const JsonObject& object, Workbook& workbook) auto evaluated_format = obj.get("evaluated_formats"); if (evaluated_format.is_object()) { auto& evaluated_format_obj = evaluated_format.as_object(); - auto& evaluated_fmts = cell->m_evaluated_formats; + auto& evaluated_fmts = cell->evaluated_formats(); read_format(evaluated_fmts, evaluated_format_obj); } @@ -535,14 +529,14 @@ JsonObject Sheet::to_json() const auto key = builder.to_string(); JsonObject data; - data.set("kind", it.value->kind == Cell::Kind::Formula ? "Formula" : "LiteralString"); - if (it.value->kind == Cell::Formula) { - data.set("source", it.value->data); + data.set("kind", it.value->kind() == Cell::Kind::Formula ? "Formula" : "LiteralString"); + if (it.value->kind() == Cell::Formula) { + data.set("source", it.value->data()); auto json = interpreter().global_object().get("JSON"); - auto stringified = interpreter().vm().call(json.as_object().get("stringify").as_function(), json, it.value->evaluated_data); + auto stringified = interpreter().vm().call(json.as_object().get("stringify").as_function(), json, it.value->evaluated_data()); data.set("value", stringified.to_string_without_side_effects()); } else { - data.set("value", it.value->data); + data.set("value", it.value->data()); } // Set type & meta diff --git a/Applications/Spreadsheet/SpreadsheetModel.cpp b/Applications/Spreadsheet/SpreadsheetModel.cpp index 5cc55acf081..2c3826801eb 100644 --- a/Applications/Spreadsheet/SpreadsheetModel.cpp +++ b/Applications/Spreadsheet/SpreadsheetModel.cpp @@ -57,8 +57,8 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role) if (!cell) return String::empty(); - if (cell->kind == Spreadsheet::Cell::Formula) { - if (auto object = as_error(cell->evaluated_data)) { + if (cell->kind() == Spreadsheet::Cell::Formula) { + if (auto object = as_error(cell->evaluated_data())) { StringBuilder builder; auto error = object->get("message").to_string_without_side_effects(); builder.append("Error: "); @@ -86,8 +86,8 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role) if (!cell) return {}; - if (cell->kind == Spreadsheet::Cell::Formula) { - if (as_error(cell->evaluated_data)) + if (cell->kind() == Spreadsheet::Cell::Formula) { + if (as_error(cell->evaluated_data())) return Color(Color::Red); } diff --git a/Applications/Spreadsheet/main.cpp b/Applications/Spreadsheet/main.cpp index a9adb3676f9..c9ab4321bbb 100644 --- a/Applications/Spreadsheet/main.cpp +++ b/Applications/Spreadsheet/main.cpp @@ -181,7 +181,7 @@ int main(int argc, char* argv[]) if (!first) text_builder.append('\t'); if (cell_data) - text_builder.append(cell_data->data); + text_builder.append(cell_data->data()); first = false; } HashMap metadata;