From 6a6f19a72fe15c05c39a53fa5be2b4ca3d65edf3 Mon Sep 17 00:00:00 2001 From: AnotherTest Date: Tue, 23 Feb 2021 17:06:07 +0330 Subject: [PATCH] Spreadsheet: Store the column index in a Position instead of its name This will make constructing (and destructing) Positions a lot cheaper (as it no longer needs to ref() and unref() a String). Resulted from #5483, but doesn't fix it. --- .../Spreadsheet/JSIntegration.cpp | 35 +++++++++----- Userland/Applications/Spreadsheet/Position.h | 36 +++++++++----- .../Applications/Spreadsheet/Spreadsheet.cpp | 48 +++++++++++-------- .../Applications/Spreadsheet/Spreadsheet.h | 6 +-- .../Spreadsheet/SpreadsheetModel.cpp | 16 +++---- .../Spreadsheet/SpreadsheetView.cpp | 16 +++---- Userland/Applications/Spreadsheet/main.cpp | 8 ++-- 7 files changed, 99 insertions(+), 66 deletions(-) diff --git a/Userland/Applications/Spreadsheet/JSIntegration.cpp b/Userland/Applications/Spreadsheet/JSIntegration.cpp index 12a249593ef..834f129fa02 100644 --- a/Userland/Applications/Spreadsheet/JSIntegration.cpp +++ b/Userland/Applications/Spreadsheet/JSIntegration.cpp @@ -125,7 +125,7 @@ JS::Value SheetGlobalObject::get(const JS::PropertyName& name, JS::Value receive return JS::js_undefined(); } - if (auto pos = Sheet::parse_cell_name(name.as_string()); pos.has_value()) { + if (auto pos = m_sheet.parse_cell_name(name.as_string()); pos.has_value()) { auto& cell = m_sheet.ensure(pos.value()); cell.reference_from(m_sheet.current_evaluated_cell()); return cell.typed_js_data(); @@ -138,7 +138,7 @@ JS::Value SheetGlobalObject::get(const JS::PropertyName& name, JS::Value receive bool SheetGlobalObject::put(const JS::PropertyName& name, JS::Value value, JS::Value receiver) { if (name.is_string()) { - if (auto pos = Sheet::parse_cell_name(name.as_string()); pos.has_value()) { + if (auto pos = m_sheet.parse_cell_name(name.as_string()); pos.has_value()) { auto& cell = m_sheet.ensure(pos.value()); if (auto current = m_sheet.current_evaluated_cell()) current->reference_from(&cell); @@ -178,7 +178,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::get_real_cell_contents) if (!this_object) return JS::js_null(); - if (StringView("SheetGlobalObject") != this_object->class_name()) { + if (!is(this_object)) { vm.throw_exception(global_object, JS::ErrorType::NotA, "SheetGlobalObject"); return {}; } @@ -195,7 +195,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::get_real_cell_contents) vm.throw_exception(global_object, "Expected a String argument to get_real_cell_contents()"); return {}; } - auto position = Sheet::parse_cell_name(name_value.as_string().string()); + auto position = sheet_object->m_sheet.parse_cell_name(name_value.as_string().string()); if (!position.has_value()) { vm.throw_exception(global_object, "Invalid cell name"); return {}; @@ -217,7 +217,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::set_real_cell_contents) if (!this_object) return JS::js_null(); - if (StringView("SheetGlobalObject") != this_object->class_name()) { + if (!is(this_object)) { vm.throw_exception(global_object, JS::ErrorType::NotA, "SheetGlobalObject"); return {}; } @@ -234,7 +234,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::set_real_cell_contents) vm.throw_exception(global_object, "Expected the first argument of set_real_cell_contents() to be a String"); return {}; } - auto position = Sheet::parse_cell_name(name_value.as_string().string()); + auto position = sheet_object->m_sheet.parse_cell_name(name_value.as_string().string()); if (!position.has_value()) { vm.throw_exception(global_object, "Invalid cell name"); return {}; @@ -254,6 +254,17 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::set_real_cell_contents) JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::parse_cell_name) { + auto* this_object = vm.this_value(global_object).to_object(global_object); + if (!this_object) + return JS::js_null(); + + if (!is(this_object)) { + vm.throw_exception(global_object, JS::ErrorType::NotA, "SheetGlobalObject"); + return {}; + } + + auto sheet_object = static_cast(this_object); + if (vm.argument_count() != 1) { vm.throw_exception(global_object, "Expected exactly one argument to parse_cell_name()"); return {}; @@ -263,12 +274,12 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::parse_cell_name) vm.throw_exception(global_object, "Expected a String argument to parse_cell_name()"); return {}; } - auto position = Sheet::parse_cell_name(name_value.as_string().string()); + auto position = sheet_object->m_sheet.parse_cell_name(name_value.as_string().string()); if (!position.has_value()) return JS::js_undefined(); auto object = JS::Object::create_empty(global_object); - object->put("column", JS::js_string(vm, position.value().column)); + object->put("column", JS::js_string(vm, sheet_object->m_sheet.column(position.value().column))); object->put("row", JS::Value((unsigned)position.value().row)); return object; @@ -285,7 +296,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::current_cell_position) if (!this_object) return JS::js_null(); - if (StringView("SheetGlobalObject") != this_object->class_name()) { + if (!is(this_object)) { vm.throw_exception(global_object, JS::ErrorType::NotA, "SheetGlobalObject"); return {}; } @@ -298,7 +309,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::current_cell_position) auto position = current_cell->position(); auto object = JS::Object::create_empty(global_object); - object->put("column", JS::js_string(vm, position.column)); + object->put("column", JS::js_string(vm, sheet_object->m_sheet.column(position.column))); object->put("row", JS::Value((unsigned)position.row)); return object; @@ -323,7 +334,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::column_index) if (!this_object) return JS::js_null(); - if (StringView("SheetGlobalObject") != this_object->class_name()) { + if (!is(this_object)) { vm.throw_exception(global_object, JS::ErrorType::NotA, "SheetGlobalObject"); return {}; } @@ -364,7 +375,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::column_arithmetic) if (!this_object) return JS::js_null(); - if (StringView("SheetGlobalObject") != this_object->class_name()) { + if (!is(this_object)) { vm.throw_exception(global_object, JS::ErrorType::NotA, "SheetGlobalObject"); return {}; } diff --git a/Userland/Applications/Spreadsheet/Position.h b/Userland/Applications/Spreadsheet/Position.h index af7d7f70a94..72b1f0a7ffa 100644 --- a/Userland/Applications/Spreadsheet/Position.h +++ b/Userland/Applications/Spreadsheet/Position.h @@ -32,9 +32,25 @@ namespace Spreadsheet { +class Sheet; + struct Position { - String column; - size_t row { 0 }; + Position() = default; + + Position(size_t column, size_t row) + : column(column) + , row(row) + , m_hash(pair_int_hash(column, row)) + { + } + + ALWAYS_INLINE u32 hash() const + { + if (m_hash == 0) + return m_hash = int_hash(column * 65537 + row); + + return m_hash; + } bool operator==(const Position& other) const { @@ -46,15 +62,13 @@ struct Position { return !(other == *this); } - URL to_url() const - { - URL url; - url.set_protocol("spreadsheet"); - url.set_host("cell"); - url.set_path(String::formatted("/{}", getpid())); - url.set_fragment(String::formatted("{}{}", column, row)); - return url; - } + URL to_url(const Sheet& sheet) const; + + size_t column { 0 }; + size_t row { 0 }; + +private: + mutable u32 m_hash { 0 }; }; } diff --git a/Userland/Applications/Spreadsheet/Spreadsheet.cpp b/Userland/Applications/Spreadsheet/Spreadsheet.cpp index be176029554..fdcb7be63d9 100644 --- a/Userland/Applications/Spreadsheet/Spreadsheet.cpp +++ b/Userland/Applications/Spreadsheet/Spreadsheet.cpp @@ -233,7 +233,7 @@ Cell* Sheet::at(const Position& position) return it->value; } -Optional Sheet::parse_cell_name(const StringView& name) +Optional Sheet::parse_cell_name(const StringView& name) const { GenericLexer lexer(name); auto col = lexer.consume_while(isalpha); @@ -242,7 +242,11 @@ Optional Sheet::parse_cell_name(const StringView& name) if (!lexer.is_eof() || row.is_empty() || col.is_empty()) return {}; - return Position { col, row.to_uint().value() }; + auto it = m_columns.find(col); + if (it == m_columns.end()) + return {}; + + return Position { it.index(), row.to_uint().value() }; } Optional Sheet::column_index(const StringView& column_name) const @@ -302,27 +306,23 @@ Optional Sheet::position_from_url(const URL& url) const Position Sheet::offset_relative_to(const Position& base, const Position& offset, const Position& offset_base) const { - auto offset_column_it = m_columns.find(offset.column); - auto offset_base_column_it = m_columns.find(offset_base.column); - auto base_column_it = m_columns.find(base.column); - - if (offset_column_it.is_end()) { + if (offset.column >= m_columns.size()) { dbgln("Column '{}' does not exist!", offset.column); return base; } - if (offset_base_column_it.is_end()) { - dbgln("Column '{}' does not exist!", offset.column); + if (offset_base.column >= m_columns.size()) { + dbgln("Column '{}' does not exist!", offset_base.column); return base; } - if (base_column_it.is_end()) { - dbgln("Column '{}' does not exist!", offset.column); + if (base.column >= m_columns.size()) { + dbgln("Column '{}' does not exist!", base.column); return offset; } - auto new_column = column(offset_column_it.index() + base_column_it.index() - offset_base_column_it.index()); + auto new_column = offset.column + base.column - offset_base.column; auto new_row = offset.row + base.row - offset_base.row; - return { move(new_column), new_row }; + return { new_column, new_row }; } void Sheet::copy_cells(Vector from, Vector to, Optional resolve_relative_to) @@ -353,7 +353,7 @@ void Sheet::copy_cells(Vector from, Vector to, Optional from, Vector to, Optional Sheet::from_json(const JsonObject& object, Workbook& workbook) }; cells.for_each_member([&](auto& name, JsonValue& value) { - auto position_option = parse_cell_name(name); + auto position_option = sheet->parse_cell_name(name); if (!position_option.has_value()) return IterationDecision::Continue; @@ -600,7 +600,7 @@ Vector> Sheet::to_xsv() const // First row = headers. size_t column_count = m_columns.size(); if (columns_are_standard()) { - column_count = convert_from_string(bottom_right.column) + 1; + column_count = bottom_right.column + 1; Vector cols; for (size_t i = 0; i < column_count; ++i) cols.append(m_columns[i]); @@ -613,7 +613,7 @@ Vector> Sheet::to_xsv() const Vector row; row.resize(column_count); for (size_t j = 0; j < column_count; ++j) { - auto cell = at({ m_columns[j], i }); + auto cell = at({ j, i }); if (cell) row[j] = cell->typed_display(); } @@ -643,7 +643,7 @@ RefPtr Sheet::from_xsv(const Reader::XSV& xsv, Workbook& workbook) auto str = row[i]; if (str.is_empty()) continue; - Position position { cols[i], row.index() }; + Position position { i, row.index() }; auto cell = make(str, position, *sheet); sheet->m_cells.set(position, move(cell)); } @@ -726,4 +726,14 @@ String Sheet::generate_inline_documentation_for(StringView function, size_t argu return builder.build(); } +URL Position::to_url(const Sheet& sheet) const +{ + URL url; + url.set_protocol("spreadsheet"); + url.set_host("cell"); + url.set_path(String::formatted("/{}", getpid())); + url.set_fragment(String::formatted("{}{}", sheet.column(column), row)); + return url; +} + } diff --git a/Userland/Applications/Spreadsheet/Spreadsheet.h b/Userland/Applications/Spreadsheet/Spreadsheet.h index 66c87f6a454..2b903dea46c 100644 --- a/Userland/Applications/Spreadsheet/Spreadsheet.h +++ b/Userland/Applications/Spreadsheet/Spreadsheet.h @@ -51,7 +51,7 @@ public: ~Sheet(); - static Optional parse_cell_name(const StringView&); + Optional parse_cell_name(const StringView&) const; Optional column_index(const StringView& column_name) const; Optional column_arithmetic(const StringView& column_name, int offset); @@ -179,9 +179,7 @@ struct Traits : public GenericTraitsat({ m_sheet->column(index.column()), (size_t)index.row() }); + const auto* cell = m_sheet->at({ (size_t)index.column(), (size_t)index.row() }); if (!cell) return String::empty(); @@ -73,10 +73,10 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role) } if (role == GUI::ModelRole::MimeData) - return Position { m_sheet->column(index.column()), (size_t)index.row() }.to_url().to_string(); + return Position { (size_t)index.column(), (size_t)index.row() }.to_url(m_sheet).to_string(); if (role == GUI::ModelRole::TextAlignment) { - const auto* cell = m_sheet->at({ m_sheet->column(index.column()), (size_t)index.row() }); + const auto* cell = m_sheet->at({ (size_t)index.column(), (size_t)index.row() }); if (!cell) return {}; @@ -84,7 +84,7 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role) } if (role == GUI::ModelRole::ForegroundColor) { - const auto* cell = m_sheet->at({ m_sheet->column(index.column()), (size_t)index.row() }); + const auto* cell = m_sheet->at({ (size_t)index.column(), (size_t)index.row() }); if (!cell) return {}; @@ -103,7 +103,7 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role) } if (role == GUI::ModelRole::BackgroundColor) { - const auto* cell = m_sheet->at({ m_sheet->column(index.column()), (size_t)index.row() }); + const auto* cell = m_sheet->at({ (size_t)index.column(), (size_t)index.row() }); if (!cell) return {}; @@ -134,9 +134,9 @@ RefPtr SheetModel::mime_data(const GUI::ModelSelection& selectio VERIFY(cursor); - Position cursor_position { m_sheet->column(cursor->column()), (size_t)cursor->row() }; + Position cursor_position { (size_t)cursor->column(), (size_t)cursor->row() }; auto new_data = String::formatted("{}\n{}", - cursor_position.to_url().to_string(), + cursor_position.to_url(m_sheet).to_string(), StringView(mime_data->data("text/x-spreadsheet-data"))); mime_data->set_data("text/x-spreadsheet-data", new_data.to_byte_buffer()); @@ -164,7 +164,7 @@ void SheetModel::set_data(const GUI::ModelIndex& index, const GUI::Variant& valu if (!index.is_valid()) return; - auto& cell = m_sheet->ensure({ m_sheet->column(index.column()), (size_t)index.row() }); + auto& cell = m_sheet->ensure({ (size_t)index.column(), (size_t)index.row() }); cell.set_data(value.to_string()); update(); } diff --git a/Userland/Applications/Spreadsheet/SpreadsheetView.cpp b/Userland/Applications/Spreadsheet/SpreadsheetView.cpp index 45dbf275a25..1a28d813627 100644 --- a/Userland/Applications/Spreadsheet/SpreadsheetView.cpp +++ b/Userland/Applications/Spreadsheet/SpreadsheetView.cpp @@ -57,7 +57,7 @@ void SpreadsheetView::EditingDelegate::set_value(const GUI::Variant& value) return StringModelEditingDelegate::set_value(value); m_has_set_initial_value = true; - const auto option = m_sheet.at({ m_sheet.column(index().column()), (size_t)index().row() }); + const auto option = m_sheet.at({ (size_t)index().column(), (size_t)index().row() }); if (option) return StringModelEditingDelegate::set_value(option->source()); @@ -198,7 +198,7 @@ SpreadsheetView::SpreadsheetView(Sheet& sheet) m_table_view->on_selection_change = [&] { m_sheet->selected_cells().clear(); for (auto& index : m_table_view->selection().indexes()) { - Position position { m_sheet->column(index.column()), (size_t)index.row() }; + Position position { (size_t)index.column(), (size_t)index.row() }; m_sheet->selected_cells().set(position); } @@ -208,7 +208,7 @@ SpreadsheetView::SpreadsheetView(Sheet& sheet) Vector selected_positions; selected_positions.ensure_capacity(m_table_view->selection().size()); for (auto& selection : m_table_view->selection().indexes()) - selected_positions.empend(m_sheet->column(selection.column()), (size_t)selection.row()); + selected_positions.empend((size_t)selection.column(), (size_t)selection.row()); if (on_selection_changed) { on_selection_changed(move(selected_positions)); @@ -229,13 +229,13 @@ SpreadsheetView::SpreadsheetView(Sheet& sheet) m_cell_range_context_menu->add_action(GUI::Action::create("Type and Formatting...", [this](auto&) { Vector positions; for (auto& index : m_table_view->selection().indexes()) { - Position position { m_sheet->column(index.column()), (size_t)index.row() }; + Position position { (size_t)index.column(), (size_t)index.row() }; positions.append(move(position)); } if (positions.is_empty()) { auto& index = m_table_view->cursor_index(); - Position position { m_sheet->column(index.column()), (size_t)index.row() }; + Position position { (size_t)index.column(), (size_t)index.row() }; positions.append(move(position)); } @@ -270,7 +270,7 @@ SpreadsheetView::SpreadsheetView(Sheet& sheet) } // Drop always has a single target. - Position target { m_sheet->column(index.column()), (size_t)index.row() }; + Position target { (size_t)index.column(), (size_t)index.row() }; target_positions.append(move(target)); if (source_positions.is_empty()) @@ -283,7 +283,7 @@ SpreadsheetView::SpreadsheetView(Sheet& sheet) } if (event.mime_data().has_text()) { - auto* target_cell = m_sheet->at({ m_sheet->column(index.column()), (size_t)index.row() }); + auto* target_cell = m_sheet->at({ (size_t)index.column(), (size_t)index.row() }); VERIFY(target_cell); target_cell->set_data(event.text()); @@ -304,7 +304,7 @@ void SpreadsheetView::show_event(GUI::ShowEvent&) Vector selected_positions; selected_positions.ensure_capacity(m_table_view->selection().size()); for (auto& selection : m_table_view->selection().indexes()) - selected_positions.empend(m_sheet->column(selection.column()), (size_t)selection.row()); + selected_positions.empend((size_t)selection.column(), (size_t)selection.row()); on_selection_changed(move(selected_positions)); } diff --git a/Userland/Applications/Spreadsheet/main.cpp b/Userland/Applications/Spreadsheet/main.cpp index 1f6539f3246..cbf85ab0517 100644 --- a/Userland/Applications/Spreadsheet/main.cpp +++ b/Userland/Applications/Spreadsheet/main.cpp @@ -170,18 +170,18 @@ int main(int argc, char* argv[]) bool first = true; auto cursor = spreadsheet_widget.current_selection_cursor(); if (cursor) { - Spreadsheet::Position position { spreadsheet_widget.current_worksheet().column(cursor->column()), (size_t)cursor->row() }; - url_builder.append(position.to_url().to_string()); + Spreadsheet::Position position { (size_t)cursor->column(), (size_t)cursor->row() }; + url_builder.append(position.to_url(spreadsheet_widget.current_worksheet()).to_string()); url_builder.append('\n'); } for (auto& cell : cells) { if (first && !cursor) { - url_builder.append(cell.to_url().to_string()); + url_builder.append(cell.to_url(spreadsheet_widget.current_worksheet()).to_string()); url_builder.append('\n'); } - url_builder.append(cell.to_url().to_string()); + url_builder.append(cell.to_url(spreadsheet_widget.current_worksheet()).to_string()); url_builder.append('\n'); auto cell_data = spreadsheet_widget.current_worksheet().at(cell);