Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ set(ODR_SOURCE_FILES
"src/odr/internal/crypto/crypto_argon2.cpp"
"src/odr/internal/crypto/crypto_util.cpp"

"src/odr/internal/csv/csv_document.cpp"
"src/odr/internal/csv/csv_file.cpp"
"src/odr/internal/csv/csv_util.cpp"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,10 @@ class DocumentTest {
val file = Odr.open(TestFiles.csvFile(tempDir).toString())
val service = Html.translate(file, cache.toString(), HtmlConfig())

// a spreadsheet: a document view plus one per sheet
val views = service.listViews()
assertEquals(1, views.size)
assertTrue(views[0].writeHtml().html.contains("alpha"))
assertEquals(2, views.size)
assertTrue(views.last().writeHtml().html.contains("alpha"))
}

private fun read(path: Path): String = String(Files.readAllBytes(path), StandardCharsets.UTF_8)
Expand Down
5 changes: 3 additions & 2 deletions apple/tests/OdrCoreTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,9 @@ final class DecodeTests: XCTestCase {
}
}

/// A csv is a *text* file to odrcore, not a document β€” it has no element
/// tree. Worth pinning: the extension suggests otherwise.
/// A csv is a *text* file to odrcore, not a document file. It does have an
/// element tree β€” `CsvFile.document()` is a second view of the same bytes β€”
/// but that does not move it out of `FileCategory.text`.
func testCsvIsTextRatherThanADocument() throws {
let path = try write("a,b\n1,2\n", as: "table.csv")
let decoded = try DecodedFile.decode(path: path)
Expand Down
6 changes: 4 additions & 2 deletions jni/tests/app/opendocument/core/HtmlTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,11 @@ void translateText() throws IOException {
@Test
void translateCsv() throws IOException {
Html html = translateOffline(TestFiles.csvFile(tempDir));
assertEquals(1, html.pages().size());
String content = Files.readString(Path.of(html.pages().get(0).path));
// a spreadsheet: a document view plus one per sheet
assertEquals(2, html.pages().size());
String content = Files.readString(Path.of(html.pages().get(1).path));
assertTrue(content.contains("alpha"));
assertTrue(content.contains("<table"));
}

@Test
Expand Down
6 changes: 4 additions & 2 deletions python/tests/test_html.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,11 @@ def test_translate_text(txt_path, tmp_path):
def test_translate_csv(csv_path, tmp_path):
html = translate_offline(csv_path, tmp_path)
pages = html.pages()
assert len(pages) == 1
content = Path(pages[0].path).read_text()
# a spreadsheet: a document view plus one per sheet
assert len(pages) == 2
content = Path(pages[-1].path).read_text()
assert "alpha" in content
assert "<table" in content


def test_translate_document(odt_path, tmp_path):
Expand Down
2 changes: 2 additions & 0 deletions src/odr/file.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,8 @@ CsvFile CsvFile::from_file(const File &file, const CsvOptions &options,
CsvFile::CsvFile(std::shared_ptr<internal::abstract::CsvFile> impl)
: DecodedFile(impl), m_impl{std::move(impl)} {}

Document CsvFile::document() const { return Document(m_impl->document()); }

CsvOptions CsvFile::options() const { return m_impl->options(); }

CsvFile CsvFile::with_options(const CsvOptions &options) const {
Expand Down
5 changes: 5 additions & 0 deletions src/odr/file.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,11 @@ class CsvFile final : public DecodedFile {

explicit CsvFile(std::shared_ptr<internal::abstract::CsvFile>);

/// @brief The csv as a one-sheet spreadsheet. The other view of the same
/// bytes β€” a csv stays a text file, so @ref TextFile::text still works.
/// @throws UnsupportedTextEncoding if the encoding cannot be decoded.
[[nodiscard]] Document document() const;

/// @brief The options in use, every field resolved.
[[nodiscard]] CsvOptions options() const;

Expand Down
5 changes: 5 additions & 0 deletions src/odr/html.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,11 @@ void HtmlResource::write_resource(std::ostream &os) const {

HtmlService html::translate(const DecodedFile &file, const HtmlConfig &config,
const Logger &logger) {
// before the text branch: a csv is a text file, and rendering one as a line
// list rather than a table is never what a viewer wants
if (file.is_csv_file()) {
return translate(file.as_csv_file().document(), config, logger);
}
if (file.is_text_file()) {
return translate(file.as_text_file(), config, logger);
}
Expand Down
26 changes: 16 additions & 10 deletions src/odr/internal/abstract/file.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,16 +67,6 @@ class TextFile : public DecodedFile {
[[nodiscard]] virtual TextEncoding encoding() const noexcept = 0;
};

class CsvFile : public TextFile {
public:
/// The options in use, every field resolved.
[[nodiscard]] virtual CsvOptions options() const = 0;

/// The same file read with @p options.
[[nodiscard]] virtual std::shared_ptr<CsvFile>
with_options(const CsvOptions &options) const = 0;
};

class ImageFile : public DecodedFile {
public:
[[nodiscard]] FileCategory file_category() const noexcept final {
Expand Down Expand Up @@ -106,6 +96,22 @@ class DocumentFile : public DecodedFile {
[[nodiscard]] virtual std::shared_ptr<Document> document() const = 0;
};

/// A csv is a text file that can also be loaded as a document β€” a one-sheet
/// spreadsheet. It stays in @ref FileCategory::text, so reading it as text
/// needs no reopening; @ref document is the other view of the same bytes.
class CsvFile : public TextFile {
public:
/// The options in use, every field resolved.
[[nodiscard]] virtual CsvOptions options() const = 0;

/// The same file read with @p options.
[[nodiscard]] virtual std::shared_ptr<CsvFile>
with_options(const CsvOptions &options) const = 0;

/// The csv as a one-sheet spreadsheet.
[[nodiscard]] virtual std::shared_ptr<Document> document() const = 0;
};

class PdfFile : public DecodedFile {
public:
[[nodiscard]] FileType file_type() const noexcept final {
Expand Down
71 changes: 71 additions & 0 deletions src/odr/internal/csv/AGENTS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
# AGENTS.md β€” `internal/csv`

Read the root [`AGENTS.md`](../../../../AGENTS.md) first. This file covers what
csv does differently, and why.

## Cells are not elements

The root `AGENTS.md` prescribes an `ElementRegistry`: a flat `std::vector` of
elements, id = index + 1. **Csv does not use one**, deliberately.

A registry costs an entry per element. A sheet has one per *cell*, so a file
with a million rows would cost millions of entries before a single one is
looked at β€” and `spreadsheet_limit` means the renderer will ask for ten
thousand rows of them at most.

`ElementIdentifier` is a `std::uint64_t`, which is room to spare:

```
63..61 kind root | sheet | cell | text
60..24 row 37 bits
23..0 column 24 bits
```

So an id *is* the coordinate, and the adapter decodes rather than looks up.
`null_element_id` is zero, so no kind may be.

The consequence to respect: **a sheet's cells are not reachable by walking**.
`element_first_child` of a sheet is `null_element_id`; cells come from
`SheetAdapter::sheet_cell(column, row)`, which is how the renderer asks for
them anyway (`html/document_element.cpp:163`).

## Everything goes through `cell` and `dimensions`

`CsvDocument` holds the whole file decoded, and the adapter never touches that
storage β€” it calls `cell(column, row)` and `dimensions()`. That is the seam a
later streaming implementation needs: an index and a window can move in behind
those two without the adapter noticing.

## Detection rejects; the parser does not

Two jobs, two places, and mixing them is the mistake this module already made
once.

- `probe` is detection. It scores a bounded sample and may say "not a csv".
Its rules β€” at least two columns, no dangling quote in a complete file β€” are
*heuristics for recognising an unknown file*, not statements about validity.
- `RecordReader` is parsing. Given a separator it is total: ragged rows, one
column, an empty file and a truncated quoted field all read as some csv.

So a one-column csv is perfectly legitimate and `CsvOptions{.separator = ','}`
reads it. `NoCsvFile` is a detection failure only. An incoherent dialect β€” a
separator equal to the quote, a line break as a separator β€” is
`std::invalid_argument`, a caller mistake rather than bad input.

## A csv is a text file that also loads as a document

`FileCategory::text`, `DocumentType::spreadsheet` β€” so `is_text_file()` is true
for a csv and `is_document_file()` is false. `abstract::CsvFile` derives from
`abstract::TextFile`, and `CsvFile::document()` is the *other* view of the same
bytes rather than the only one: `TextFile::text()` keeps working, so reading a
csv as text needs no reopening. Opening it as `FileType::text_file` stays the
escape hatch when detection was wrong about it being a csv at all.

The one thing that costs: `html::translate` has to test `is_csv_file()` ahead of
its text branch (`html.cpp:215`), because a csv answers `is_text_file()` and a
line list is never what a viewer wants from a table.

Text has to be UTF-8 by the time it reaches a cell: `Text::content()` returns
`std::string` and every binding treats it as UTF-8. That is why an encoding
`internal/encoding` cannot decode has no document at all, while the *text*
rendering path stays open to it.
Loading
Loading