fix ods regressions

Author: andiwand

README.md

@@ -38,3 +38,13 @@ As an alternative to the Conan remote you can also export the package locally vi
 ## Version
 
 Versions and history are tracked on [GitHub](https://github.com/opendocument-app/OpenDocument.core).
+
+## Testing
+
+### Running the HTML Comparison Server
+
+Scripts and Docker images can be found here https://github.com/opendocument-app/compare-html
+
+```bash
+./test/scripts/compare_output_server.sh
+```

cli/src/server.cpp

@@ -1,5 +1,8 @@
+#include "odr/archive.hpp"
+
 #include <odr/exceptions.hpp>
 #include <odr/file.hpp>
+#include <odr/filesystem.hpp>
 #include <odr/html.hpp>
 #include <odr/http_server.hpp>
 
@@ -9,7 +12,8 @@
 using namespace odr;
 
 int main(const int argc, char **argv) {
-  auto logger = Logger::create_stdio("odr-server", LogLevel::verbose);
+  std::shared_ptr logger =
+      Logger::create_stdio("odr-server", LogLevel::verbose);
 
   std::string input{argv[1]};
 
@@ -48,15 +52,36 @@ int main(const int argc, char **argv) {
   html_config.editable = true;
 
   {
-    std::string prefix = "one_file";
-    HtmlViews views = server.serve_file(decoded_file, prefix, html_config);
+    constexpr std::string prefix = "file";
+    const HtmlViews views =
+        server.serve_file(decoded_file, prefix, html_config);
     ODR_INFO(*logger, "hosted decoded file with id: " << prefix);
     for (const auto &view : views) {
       ODR_INFO(*logger,
                "http://localhost:8080/file/" << prefix << "/" << view.path());
     }
   }
 
+  if (decoded_file.is_document_file() || decoded_file.is_archive_file()) {
+    std::optional<Filesystem> filesystem;
+    if (decoded_file.is_document_file()) {
+      filesystem = decoded_file.as_document_file().document().as_filesystem();
+    } else if (decoded_file.is_archive_file()) {
+      filesystem = decoded_file.as_archive_file().archive().as_filesystem();
+    }
+
+    constexpr std::string prefix = "filesystem";
+    const HtmlService filesystem_service = html::translate(
+        filesystem.value(), server_config.cache_path + "/" + prefix,
+        html_config, logger);
+    server.connect_service(filesystem_service, prefix);
+    ODR_INFO(*logger, "hosted filesystem with id: " << prefix);
+    for (const auto &view : filesystem_service.list_views()) {
+      ODR_INFO(*logger,
+               "http://localhost:8080/file/" << prefix << "/" << view.path());
+    }
+  }
+
   server.listen("localhost", 8080);
 
   return 0;

src/odr/document_element.cpp

@@ -276,6 +276,12 @@ TableRowStyle Sheet::row_style(const std::uint32_t row) const {
                    : TableRowStyle();
 }
 
+TableCellStyle Sheet::cell_style(const std::uint32_t column,
+                                 const std::uint32_t row) const {
+  return exists_() ? m_adapter2->sheet_cell_style(m_identifier, column, row)
+                   : TableCellStyle();
+}
+
 TablePosition SheetCell::position() const {
   return exists_() ? m_adapter2->sheet_cell_position(m_identifier)
                    : TablePosition(0, 0);
@@ -295,11 +301,6 @@ ValueType SheetCell::value_type() const {
                    : ValueType::unknown;
 }
 
-TableCellStyle SheetCell::style() const {
-  return exists_() ? m_adapter2->sheet_cell_style(m_identifier)
-                   : TableCellStyle();
-}
-
 std::string Page::name() const {
   return exists_() ? m_adapter2->page_name(m_identifier) : "";
 }

src/odr/document_element.hpp

@@ -294,6 +294,8 @@ class Sheet final : public ElementBase<internal::abstract::SheetAdapter> {
   [[nodiscard]] TableStyle style() const;
   [[nodiscard]] TableColumnStyle column_style(std::uint32_t column) const;
   [[nodiscard]] TableRowStyle row_style(std::uint32_t row) const;
+  [[nodiscard]] TableCellStyle cell_style(std::uint32_t column,
+                                          std::uint32_t row) const;
 };
 
 /// @brief Represents a sheet cell element in a document.
@@ -306,8 +308,6 @@ class SheetCell final
   [[nodiscard]] bool is_covered() const;
   [[nodiscard]] TableDimensions span() const;
   [[nodiscard]] ValueType value_type() const;
-
-  [[nodiscard]] TableCellStyle style() const;
 };
 
 /// @brief Represents a page element in a document.

src/odr/filesystem.cpp

@@ -7,10 +7,12 @@
 
 namespace odr {
 
-FileWalker::FileWalker() = default;
-
 FileWalker::FileWalker(std::unique_ptr<internal::abstract::FileWalker> impl)
-    : m_impl{std::move(impl)} {}
+    : m_impl{std::move(impl)} {
+  if (m_impl == nullptr) {
+    throw std::invalid_argument("impl must not be null");
+  }
+}
 
 FileWalker::FileWalker(const FileWalker &other)
     : m_impl{other.m_impl->clone()} {}
@@ -29,71 +31,48 @@ FileWalker &FileWalker::operator=(const FileWalker &other) {
 
 FileWalker &FileWalker::operator=(FileWalker &&) noexcept = default;
 
-FileWalker::operator bool() const { return m_impl != nullptr; }
+bool FileWalker::end() const { return m_impl->end(); }
 
-bool FileWalker::end() const { return m_impl == nullptr || m_impl->end(); }
+std::uint32_t FileWalker::depth() const { return m_impl->depth(); }
 
-std::uint32_t FileWalker::depth() const {
-  return m_impl != nullptr ? m_impl->depth() : 0;
-}
+std::string FileWalker::path() const { return m_impl->path().string(); }
 
-std::string FileWalker::path() const {
-  return m_impl != nullptr ? m_impl->path().string() : std::string("");
-}
+bool FileWalker::is_file() const { return m_impl->is_file(); }
 
-bool FileWalker::is_file() const {
-  return m_impl != nullptr && m_impl->is_file();
-}
+bool FileWalker::is_directory() const { return m_impl->is_directory(); }
 
-bool FileWalker::is_directory() const {
-  return m_impl != nullptr && m_impl->is_directory();
-}
+void FileWalker::pop() const { m_impl->pop(); }
 
-void FileWalker::pop() const {
-  if (m_impl != nullptr) {
-    m_impl->pop();
-  }
-}
+void FileWalker::next() const { m_impl->next(); }
 
-void FileWalker::next() const {
-  if (m_impl != nullptr) {
-    m_impl->next();
-  }
-}
-
-void FileWalker::flat_next() const {
-  if (m_impl != nullptr) {
-    m_impl->flat_next();
-  }
-}
+void FileWalker::flat_next() const { m_impl->flat_next(); }
 
 Filesystem::Filesystem(
     std::shared_ptr<internal::abstract::ReadableFilesystem> impl)
-    : m_impl{std::move(impl)} {}
-
-Filesystem::operator bool() const { return m_impl != nullptr; }
+    : m_impl{std::move(impl)} {
+  if (m_impl == nullptr) {
+    throw std::invalid_argument("impl must not be null");
+  }
+}
 
 bool Filesystem::exists(const std::string &path) const {
-  return m_impl != nullptr && m_impl->exists(internal::AbsPath(path));
+  return m_impl->exists(internal::AbsPath(path));
 }
 
 bool Filesystem::is_file(const std::string &path) const {
-  return m_impl != nullptr && m_impl->is_file(internal::AbsPath(path));
+  return m_impl->is_file(internal::AbsPath(path));
 }
 
 bool Filesystem::is_directory(const std::string &path) const {
-  return m_impl != nullptr && m_impl->is_directory(internal::AbsPath(path));
+  return m_impl->is_directory(internal::AbsPath(path));
 }
 
 FileWalker Filesystem::file_walker(const std::string &path) const {
-  return m_impl != nullptr
-             ? FileWalker(m_impl->file_walker(internal::AbsPath(path)))
-             : FileWalker();
+  return FileWalker(m_impl->file_walker(internal::AbsPath(path)));
 }
 
 File Filesystem::open(const std::string &path) const {
-  return m_impl != nullptr ? File(m_impl->open(internal::AbsPath(path)))
-                           : File();
+  return File(m_impl->open(internal::AbsPath(path)));
 }
 
 } // namespace odr

src/odr/filesystem.hpp

@@ -14,15 +14,13 @@ class File;
 /// @brief FileWalker class
 class FileWalker {
 public:
-  FileWalker();
   explicit FileWalker(std::unique_ptr<internal::abstract::FileWalker>);
   FileWalker(const FileWalker &);
   FileWalker(FileWalker &&) noexcept;
   ~FileWalker();
 
   FileWalker &operator=(const FileWalker &);
   FileWalker &operator=(FileWalker &&) noexcept;
-  [[nodiscard]] explicit operator bool() const;
 
   [[nodiscard]] bool end() const;
   [[nodiscard]] std::uint32_t depth() const;
@@ -43,8 +41,6 @@ class Filesystem {
 public:
   explicit Filesystem(std::shared_ptr<internal::abstract::ReadableFilesystem>);
 
-  [[nodiscard]] explicit operator bool() const;
-
   [[nodiscard]] bool exists(const std::string &path) const;
   [[nodiscard]] bool is_file(const std::string &path) const;
   [[nodiscard]] bool is_directory(const std::string &path) const;

src/odr/html.cpp

@@ -326,13 +326,21 @@ HtmlService html::translate(const PdfFile &pdf_file,
                                             std::move(logger));
 }
 
-HtmlService html::translate(const Archive &archive,
+HtmlService html::translate(const Filesystem &filesystem,
                             const std::string &cache_path,
                             const HtmlConfig &config,
                             std::shared_ptr<Logger> logger) {
   std::filesystem::create_directories(cache_path);
-  return internal::html::create_filesystem_service(
-      archive.as_filesystem(), cache_path, config, std::move(logger));
+  return internal::html::create_filesystem_service(filesystem, cache_path,
+                                                   config, std::move(logger));
+}
+
+HtmlService html::translate(const Archive &archive,
+                            const std::string &cache_path,
+                            const HtmlConfig &config,
+                            std::shared_ptr<Logger> logger) {
+  return translate(archive.as_filesystem(), cache_path, config,
+                   std::move(logger));
 }
 
 HtmlService html::translate(const Document &document,

src/odr/html.hpp

@@ -19,6 +19,7 @@ class HtmlResource;
 
 namespace odr {
 class Archive;
+class Filesystem;
 struct HtmlPage;
 class HtmlService;
 struct HtmlConfig;
@@ -259,6 +260,16 @@ HtmlService translate(const PdfFile &pdf_file, const std::string &cache_path,
                       const HtmlConfig &config,
                       std::shared_ptr<Logger> logger = Logger::create_null());
 
+/// @brief Translates a filesystem to HTML.
+///
+/// @param filesystem Filesystem to translate.
+/// @param cache_path Directory path for temporary output.
+/// @param config Configuration for the HTML output.
+/// @param logger Logger to use for logging.
+/// @return HTML output.
+HtmlService translate(const Filesystem &filesystem,
+                      const std::string &cache_path, const HtmlConfig &config,
+                      std::shared_ptr<Logger> logger = Logger::create_null());
 /// @brief Translates an archive to HTML.
 ///
 /// @param archive Archive to translate.

src/odr/http_server.hpp

@@ -18,6 +18,7 @@ class HttpServer {
   constexpr static auto prefix_pattern = R"(([a-zA-Z0-9_-]+))";
 
   struct Config {
+    // TODO remove
     std::string cache_path{"/tmp/odr"};
   };
 
@@ -28,6 +29,7 @@ class HttpServer {
 
   void connect_service(HtmlService service, const std::string &prefix) const;
 
+  // TODO remove
   [[nodiscard]] HtmlViews serve_file(const DecodedFile &file,
                                      const std::string &prefix,
                                      const HtmlConfig &config) const;

src/odr/internal/abstract/document_element.hpp

@@ -172,6 +172,9 @@ class SheetAdapter {
                      std::uint32_t column) const = 0;
   [[nodiscard]] virtual TableRowStyle
   sheet_row_style(ElementIdentifier element_id, std::uint32_t row) const = 0;
+  [[nodiscard]] virtual TableCellStyle
+  sheet_cell_style(ElementIdentifier element_id, std::uint32_t column,
+                   std::uint32_t row) const = 0;
 };
 
 class SheetCellAdapter {
@@ -187,9 +190,6 @@ class SheetCellAdapter {
   sheet_cell_span(ElementIdentifier element_id) const = 0;
   [[nodiscard]] virtual ValueType
   sheet_cell_value_type(ElementIdentifier element_id) const = 0;
-
-  [[nodiscard]] virtual TableCellStyle
-  sheet_cell_style(ElementIdentifier element_id) const = 0;
 };
 
 class MasterPageAdapter {