OlivierLDff / Qaterial

🧩 Collection of Material Components based on QtQuickControls2.
https://olivierldff.github.io/Qaterial/
MIT License
291 stars 54 forks source link

QATERIAL_ENABLE_ICONS=OFF code path is broken. #150

Closed MarekPasnikowski closed 4 months ago

MarekPasnikowski commented 4 months ago

https://github.com/OlivierLDff/Qaterial/blame/cf5a5ef064c3deaaf54faff722a808544e7a87f0/cmake/QaterialGenerateIcons.cmake#L184

The last update to the function introduced new arguments, but this change was not implemented in the ENABLE_ICONS=OFF branch.

I was able to complete the compilation with the attached change, but this patch is a blind copy-paste of code with some cuts and renames — the chance that this is the correct solution is very low.

PS: To my frustration, GitHub does not allow me to attach neither a .patch, nor a .cmake file, so I am dumping the .patch below:

From 1c6b82f51a29f184727256da40c649bc46d81e57 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Pa=C5=9Bnikowski?= <marek@marekpasnikowski.pl>
Date: Wed, 27 Mar 2024 10:50:08 +0100
Subject: [PATCH] Bug Fix

---
 cmake/QaterialGenerateIcons.cmake | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/cmake/QaterialGenerateIcons.cmake b/cmake/QaterialGenerateIcons.cmake
index d28d45d..3b54ae9 100644
--- a/cmake/QaterialGenerateIcons.cmake
+++ b/cmake/QaterialGenerateIcons.cmake
@@ -200,8 +200,8 @@ function(qaterial_generate_icons_class OUTPUT_FILE_HPP OUTPUT_FILE_CPP)
   else()

     # Generate fake Qaterial.Impl.Icons.Icons.qml
-    message(STATUS "Generate Fake ${OUTPUT_FILE}")
-    file(WRITE ${OUTPUT_FILE}
+    message(STATUS "Generate Fake ${OUTPUT_FILE_HPP}")
+    file(WRITE ${OUTPUT_FILE_HPP}
       "// Dummy file generated with CMake to mock the absence of Mdi icons.\n"
       "// Everything written here will be lost.\n\n"
       "#ifndef __QATERIAL_ICONS_HPP__\n"
@@ -221,6 +221,27 @@ function(qaterial_generate_icons_class OUTPUT_FILE_HPP OUTPUT_FILE_CPP)
       "#endif"
     )

-  endif()
+    # Generate fake Qaterial.Impl.Icons.Icons.qml
+    message(STATUS "Generate Fake ${OUTPUT_FILE_CPP}")
+    file(WRITE ${OUTPUT_FILE_CPP}
+      "// Dummy file generated with CMake to mock the absence of Mdi icons.\n"
+      "// Everything written here will be lost.\n\n"
+      "#ifndef __QATERIAL_ICONS_HPP__\n"
+      "#define __QATERIAL_ICONS_HPP__\n\n"
+      "#include <Qaterial/Details/Export.hpp>\n"
+      "#include <Qaterial/Details/Property.hpp>\n\n"
+      "#include <QtCore/QObject>\n\n"
+      "namespace qaterial {\n\n"
+      "class QATERIAL_API_ Icons : public QObject\n"
+      "{\n"
+      "    QATERIAL_SINGLETON_IMPL(Icons, icons, Icons);\n\n"
+      "public:\n"
+      "    Icons(QObject* parent = nullptr) : QObject(parent) {}\n\n"
+      "};\n\n"
+      "}\n\n"
+      "#endif"
+    )
+
+endif()

 endfunction()
-- 
2.41.0
OlivierLDff commented 4 months ago

You are right, as this option is not tested in CI. Would you provide your patch as a PR?

MarekPasnikowski commented 4 months ago

I could.

Does the dummy code look appropriate to you? I have zero knowledge of the programming languages involved here and this report is the result of my work towards packaging an application in Guix.

OlivierLDff commented 4 months ago

I will do the appropriate PR, and add the appropriate tests in CI. The file that is generated should be OUTPUT_FILE_HPP, and I guess an empty OUTPUT_FILE_CPP should be generated

OlivierLDff commented 4 months ago

https://github.com/OlivierLDff/Qaterial/pull/151 should fix your issue :)