Spreadsheet: struct Cell => class Cell

Hide private members, and make the odd update() -> sheet->update(cell)
-> update(Badge<Sheet>) -> update_data() less odd by removing the
update(Badge<Sheet>) step.

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<Cell&>(*this), m_type_metadata);
 }
 
-void Cell::update_data()
+void Cell::update_data(Badge<Sheet>)
 {
-    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;
 }
 
 }

Applications/Spreadsheet/Cell.h

@@ -38,21 +38,26 @@
 namespace Spreadsheet {
 
 struct Cell : public Weakable<Cell> {
+    enum Kind {
+        LiteralString,
+        Formula,
+    };
+
     Cell(String data, Position position, WeakPtr<Sheet> 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> 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<Cell> {
 
     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<WeakPtr<Cell>>& 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<Cell> {
     const Position& position() const { return m_position; }
     void set_position(Position position, Badge<Sheet>)
     {
-        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<ConditionalFormat>& conditional_formats() const { return m_conditional_formats; }
     void set_conditional_formats(Vector<ConditionalFormat>&& fmts)
     {
-        dirty = true;
+        m_dirty = true;
         m_conditional_formats = move(fmts);
     }
 
@@ -92,30 +104,28 @@ struct Cell : public Weakable<Cell> {
 
     JS::Value js_data();
 
-    void update(Badge<Sheet>) { update_data(); }
     void update();
+    void update_data(Badge<Sheet>);
 
-    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> sheet;
-    Vector<WeakPtr<Cell>> 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<Sheet> m_sheet;
+    Vector<WeakPtr<Cell>> m_referencing_cells;
     const CellType* m_type { nullptr };
     CellTypeMetadata m_type_metadata;
     Position m_position;
 
     Vector<ConditionalFormat> m_conditional_formats;
     Format m_evaluated_formats;
-
-private:
-    void update_data();
 };
 
 }

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
 }
 

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());
 }
 
 }

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);
 }
 
 }

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<SheetGlobalObject>(*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<Position> from, Vector<Position> to, Optional<Posi
             return;
         }
 
-        auto ref_cells = target_cell.referencing_cells;
-        target_cell = *source_cell;
-        target_cell.dirty = true;
-        target_cell.referencing_cells = move(ref_cells);
+        target_cell.copy_from(*source_cell);
     };
 
     if (from.size() == to.size()) {
@@ -467,7 +461,7 @@ RefPtr<Sheet> 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

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);
         }
 

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<String, String> metadata;