address reviews suggestions

jasp-stats · Sep 6, 2024 · 0da1937 · 0da1937
1 parent 10f4a08
commit 0da1937
Show file tree

Hide file tree

Showing 7 changed files with 18 additions and 838 deletions.
diff --git a/Desktop/data/importers/excel/__freexl.h b/Desktop/data/importers/excel/__freexl.h
diff --git a/Desktop/data/importers/excel/excel.cpp b/Desktop/data/importers/excel/excel.cpp
@@ -31,7 +31,7 @@ Excel::Excel(const string &locator)
 
 void Excel::open()
 {
-	_fileSize = Utils::getFileSize(_path);
+	_fileSize = QFileInfo::size(_path);
 
 	if (_fileSize < 0)
 		throw runtime_error("Could not access file");
@@ -41,8 +41,8 @@ void Excel::open()
 }
 void Excel::openWorkbook() 
 {
-	QString xlsFilePath = QString::fromStdString(_path);
-	const char* utf8Path = xlsFilePath.toUtf8();
+	QString xlsFilePath = tq(_path);
+	const char* utf8Path = _path.c_str(); //But it would be better to just use _path.c_str() directly if you need it. It is in utf8 in any case.
 	QString extension = QFileInfo(xlsFilePath).suffix().toLower();
 
 	int ret = 0;

diff --git a/Desktop/data/importers/excel/excelimportcolumn.cpp b/Desktop/data/importers/excel/excelimportcolumn.cpp
@@ -13,7 +13,6 @@ ExcelImportColumn::ExcelImportColumn(ImportDataSet *importDataSet, std::string n
 ExcelImportColumn::~ExcelImportColumn()
 {
 	JASPTIMER_SCOPE(ExcelImportColumn::~ExcelImportColumn());
-	_data.clear();
 }
 
 size_t ExcelImportColumn::size() const

diff --git a/Desktop/data/importers/excelimporter.cpp b/Desktop/data/importers/excelimporter.cpp
@@ -18,11 +18,10 @@
 #include "excelimporter.h"
 #include "data/importers/excel/excel.h"
 #include "data/importers/excel/excelimportcolumn.h"
-#include <string>
+
 #include <QFileInfo>
 #include <QDebug>
-
-using namespace std;
+#include "columnutils.h"
 
 ExcelImporter::ExcelImporter() : Importer() {}
 
@@ -74,26 +73,16 @@ ImportDataSet* ExcelImporter::loadFile(const std::string &locator, std::function
 
 			for (int i = 0; i < colNames.size(); ++i) 
 			{
+				string colName = colNames[i];
 				string colName = colNames[i];
 				if (colName.empty()) 
-				{
 					colName = "V" + std::to_string(i + 1);
-				} else 
-				{
-					try 
-					{
-						if (std::to_string(std::stoi(colName)) == colName)
-							colName = "V" + colName;
-					} catch (...) {}
-				}
+				else if(ColumnUtils::isIntValue(colName))
+					colName = "V" + colName;
+
+				//What is the purpose of the `find` here? Maybe explain in a comment here
 				if (std::find(colNames.begin(), colNames.begin() + i, colName) != colNames.begin() + i)
 					colName += "_" + std::to_string(i + 1);
-
-				colNames[i] = colName;
-				importColumns.push_back(new ExcelImportColumn(data, colName, rows - 1));
-			}
-		}
-		else 
 		{
 			for (int i = 0; i < importColumns.size(); ++i) 
 			{
@@ -104,13 +93,6 @@ ImportDataSet* ExcelImporter::loadFile(const std::string &locator, std::function
 
 	for (ExcelImportColumn* col : importColumns) 
 	{
-		data->addColumn(col);
-	}
-
-	data->buildDictionary();
-	excel.close();
-
-	JASPTIMER_STOP(ExcelImporter::loadFile);
-
-	return data;
-}
+		else 
+			for (int i = 0; i < importColumns.size(); ++i) 
+				importColumns[i]->addValue(i < lineValues.size() ? lineValues[i] : "");
diff --git a/Docs/development/jasp-build-guide-linux.md b/Docs/development/jasp-build-guide-linux.md
@@ -40,7 +40,7 @@ Based on your system, you can install the mentioned libraries using your package
 On Ubuntu, you can use `apt`.
 
 ```
-sudo apt install libboost-dev libjsoncpp25 libjsoncpp-dev libarchive13 libarchive-dev libxcb-xkb-dev libxcb-xkb1 libxcb-xinerama0 libxcb-cursor0 libxkbcommon-dev libxkbcommon-x11-dev autoconf zlib1g zlib1g-dev cmake gfortran build-essential flex libssl-dev libgl1-mesa-dev libsqlite3-dev r-base libglpk-dev libminizip-dev
+sudo apt install libboost-dev libjsoncpp25 libjsoncpp-dev libarchive13 libarchive-dev libxcb-xkb-dev libxcb-xkb1 libxcb-xinerama0 libxcb-cursor0 libxkbcommon-dev libxkbcommon-x11-dev autoconf zlib1g zlib1g-dev cmake gfortran build-essential flex libssl-dev libgl1-mesa-dev libsqlite3-dev r-base libglpk-dev libminizip-dev libfreexl-dev
 ```
 
 > ⚠️ Some of these libraries might not be up-to-date and as a result JASP will complain. If this happens, you need to download, make and install those libraries individually. Alternatively, you can use the [Linux version of Homebrew](https://docs.brew.sh/Homebrew-on-Linux) and install the up-to-dated libraries locally.

diff --git a/Docs/development/jasp-build-guide-windows.md b/Docs/development/jasp-build-guide-windows.md
@@ -67,12 +67,12 @@ After installing Rtools44, you will find a new program in your Start Menu. Searc
 Copy and paste the following line into the `ucrt64` command line and press Enter. With this command, we are installing some of required packages and libraries necessary for building JASP. Run this command at least twice to make sure all required packages are installed.
 
 ```bash
-pacman -Syu mingw-w64-ucrt-x86_64-toolchain mingw-w64-ucrt-x86_64-boost jsoncpp bison flex make autoconf automake git wget cmake  mingw-w64-ucrt-x86_64-libiconv  libiconv-devel libtool zlib-devel zlib mingw-w64-ucrt-x86_64-zlib mingw-w64-ucrt-x86_64-jsoncpp mingw-w64-ucrt-x86_64-minizip
+pacman -Syu mingw-w64-ucrt-x86_64-toolchain mingw-w64-ucrt-x86_64-boost jsoncpp bison flex make autoconf automake git wget cmake  mingw-w64-ucrt-x86_64-libiconv  libiconv-devel libtool zlib-devel zlib mingw-w64-ucrt-x86_64-zlib mingw-w64-ucrt-x86_64-jsoncpp
 ```
 
-#### Downloading and Building libraries (on Rtools44)
+#### Downloading and Building ReadStat (on Rtools44)
 
-In addition to these libraries, you need to manually download and install the ReadStat and FreeXL libraries. You can do that by typing the following commands into the `ucrt64` command line.
+In addition to these libraries, you need to manually download and install the ReadStat library. You can do that by typing the following commands into the `ucrt64` command line.
 
 ```
 git clone https://github.com/WizardMac/ReadStat.git
@@ -84,15 +84,6 @@ make -j
 make install
 ```
 
-```
-git clone https://github.com/jasp-stats/freexl.git
-cd freexl
-autoreconf -i -f
-./configure --host=x86_64-ucrt-mingw32 --build=x86_64-ucrt-mingw32
-make -j
-make install
-```
-
 This will build and install these libraries inside the Rtools44 environment where JASP will look for them. If any of these steps goes wrong, JASP's build system cannot configure the build.
 
 #### Adding Rtools44 to your PATH

diff --git a/Tools/CMake/Libraries.cmake b/Tools/CMake/Libraries.cmake
@@ -289,63 +289,7 @@ if(WIN32)
         "ReadStat is required for building on Windows, please follow the build instruction before you continue."
     )
   endif()
-
-  # FreeXL library ---
-  message(CHECK_START "Looking for freexl.dll.a")
-  find_file(
-    RTOOLS_LIBFREEXL_DLL_A
-    NAMES libfreexl.dll.a
-    PATHS ${RTOOLS_PATH}/lib
-    NO_DEFAULT_PATH)
-
-  if(EXISTS ${RTOOLS_LIBFREEXL_DLL_A})
-    message(CHECK_PASS "found")
-    message(STATUS "  ${RTOOLS_LIBFREEXL_DLL_A}")
-  else()
-    message(CHECK_FAIL "not found")
-    message(
-      FATAL_ERROR
-        "FreeXL is required for building on Windows, please follow the build instruction before you continue."
-    )
-  endif()
-
-  message(CHECK_START "Looking for freexl.h")
-  find_file(
-    RTOOLS_LIBFREEXL_H
-    NAMES freexl.h
-    PATHS ${RTOOLS_PATH}/include
-    NO_DEFAULT_PATH)
-
-  if(EXISTS ${RTOOLS_LIBFREEXL_H})
-    message(CHECK_PASS "found")
-    message(STATUS "  ${RTOOLS_LIBFREEXL_H}")
-  else()
-    message(CHECK_FAIL "not found")
-    message(
-      FATAL_ERROR
-        "FreeXL is required for building on Windows, please follow the build instruction before you continue."
-    )
-  endif()
-
-  message(CHECK_START "Looking for libfreexl-1.dll")
-  find_file(
-    RTOOLS_LIBFREEXL_DLL
-    NAMES libfreexl-1.dll
-    PATHS ${RTOOLS_PATH}/bin
-    NO_DEFAULT_PATH)
-
-  if(EXISTS ${RTOOLS_LIBFREEXL_DLL})
-    message(CHECK_PASS "found")
-    message(STATUS "  ${RTOOLS_LIBFREEXL_DLL}")
-  else()
-    message(CHECK_FAIL "not found")
-    message(
-      FATAL_ERROR
-        "FreeXL is required for building on Windows, please follow the build instruction before you continue."
-    )
-  endif()
-  # End FreeXL library ---
-
+
   message(CHECK_START "Looking for zlib1.dll")
   find_file(
     RTOOLS_ZLIB_DLL