Conversation
|
The code compiles and runs with Qt 6, albeit with many warnings as that requires C++-17. Most of the warnings seem to be from this iterator in solvespace.h: |
b25bff3 to
4e67770
Compare
ruevs
left a comment
There was a problem hiding this comment.
The changes in
CMakeLists.txt
and
src/CMakeLists.txt
are pretty intrusive and definitely should be reworked. I'm not particularly skilled with CMake but please refer to how the EMSCRIPTEN changes were integrated (look in the log) - it was the latest platform added.
In addition the following files should NOT be modified by this pull request:
gui.h
guigtk.cpp
guihtml.cpp
guimac.mm
guinone.cpp
guiwin.cpp
Please revert those changes.
CMakeLists.txt
Outdated
| VERSION 3.1 | ||
| LANGUAGES C CXX ASM) | ||
|
|
||
| set(USE_QT_GUI OFF CACHE BOOL |
There was a problem hiding this comment.
The CMakeLists.txt is not acceptable in this state. As it is all checks fail.
The QT version needs to be an optional non default target on any of the three platforms supported by QT.
Unless this option is set SolveSpace should continue to build just like before on all four platforms using the native/GTK GUI (Windows, Linux, macOS, Web/Emscripten).
There was a problem hiding this comment.
I don't know CMake well (all that is from shingen75), so it may take some time to cleanup things.
I'm currently building solvespace by other means as I was unable to get a debug build using cmake (hence solvespace/libdxfrw#14). I tried this...
cmake -DOPENGL=3 -DUSE_QT_GUI=ON -DCMAKE_BUILD_TYPE=Debug ..
...but I don't get any symbols in the debugger.
There was a problem hiding this comment.
@WickedSmoke To get symbols in the debugger, just making the following changes:
diff --git a/cmake/c_flag_overrides.cmake b/cmake/c_flag_overrides.cmake
index 978b6b65..014d7ada 100644
--- a/cmake/c_flag_overrides.cmake
+++ b/cmake/c_flag_overrides.cmake
@@ -8,3 +8,5 @@ endif()
if(EMSCRIPTEN)
set(CMAKE_C_FLAGS_DEBUG_INIT "-g4")
endif()
+
+set(CMAKE_C_FLAGS_DEBUG_INIT "-g")
diff --git a/cmake/cxx_flag_overrides.cmake b/cmake/cxx_flag_overrides.cmake
index 9c8d15fe..ec43ae18 100644
--- a/cmake/cxx_flag_overrides.cmake
+++ b/cmake/cxx_flag_overrides.cmake
@@ -8,3 +8,5 @@ endif()
if(EMSCRIPTEN)
set(CMAKE_CXX_FLAGS_DEBUG_INIT "-g4")
endif()
+
+set(CMAKE_CXX_FLAGS_DEBUG_INIT "-g")
It's a bit of a hack, and make sure not to commit those changes!
src/platform/gui.h
Outdated
| void OpenInBrowser(const std::string &url); | ||
|
|
||
| std::vector<std::string> InitGui(int argc, char **argv); | ||
| std::vector<std::string> InitGui(int& argc, char **argv); |
There was a problem hiding this comment.
The abstraction layer gets in the way of normal QApplication usage where the main argc and argv are edited to remove any Qt specific options. See https://doc.qt.io/qt-5/qguiapplication.html#QGuiApplication
There was a problem hiding this comment.
When I have some time I'll install QT 5 and try to clean this up. It should not be a problem to do it, but I do not want to give suggestions without a compiler :-)
There was a problem hiding this comment.
There's so little going on in entrygui.cpp that it seems cleaner to just have each platform provide this. A pure Qt version would be:
int main(int argc, char** argv) {
QApplication app(argc, argv);
Platform::Open3DConnexion();
SS.Init();
if(argc >= 2) {
if(argc > 2) {
dbp("Only the first file passed on command line will be opened.");
}
SS.Load(argv[1]);
}
app.exec();
Platform::Close3DConnexion();
SS.Clear();
SK.Clear();
return 0;
}
This eliminates InitGui(), RunGui(), ClearGui() and the argument string vector.
The argument parsing could be replaced with something like:
SS.ParseArgs(argc, argv);
src/platform/guigtk.cpp
Outdated
| Gtk::Main *gtkMain; | ||
|
|
||
| std::vector<std::string> InitGui(int argc, char **argv) { | ||
| std::vector<std::string> InitGui(int& argc, char **argv) { |
src/platform/guimac.mm
Outdated
| static SSApplicationDelegate *ssDelegate; | ||
|
|
||
| std::vector<std::string> InitGui(int argc, char **argv) { | ||
| std::vector<std::string> InitGui(int& argc, char **argv) { |
src/platform/guinone.cpp
Outdated
| void OpenInBrowser(const std::string &url) {} | ||
|
|
||
| std::vector<std::string> InitGui(int argc, char **argv) { | ||
| std::vector<std::string> InitGui(int& argc, char **argv) { |
src/platform/guiwin.cpp
Outdated
| } | ||
|
|
||
| std::vector<std::string> InitGui(int argc, char **argv) { | ||
| std::vector<std::string> InitGui(int& argc, char **argv) { |
src/platform/guihtml.cpp
Outdated
| } | ||
|
|
||
| std::vector<std::string> InitGui(int argc, char **argv) { | ||
| std::vector<std::string> InitGui(int& argc, char **argv) { |
src/CMakeLists.txt
Outdated
| ${PNG_LIBRARY} | ||
| ${FREETYPE_LIBRARY} | ||
| ${CAIRO_LIBRARIES} | ||
| #${CAIRO_LIBRARIES} |
This is known. Can the QT version be compiled in C++11 mode? If not the warning is benign anyway. |
C++17 is a requirement for Qt 6, not Qt 5. |
|
The "Fix QApplication::exec() crash." commit has been modified to not touch the InitGui() function. Instead it bypasses the InitGui/RunGui/ClearGui interface and simply provides main itself (as suggested in my post last night). It looks like there's a bug in entrygui.cpp where the last argument is passed to SS.Load rather then the first. |
I don't have a problem with C++17, the GTK4 version will also require it. The question is if our CI/CD stuff is up to date to allow it on all platforms. |
|
I've gone over (and modified) most of the Qt code now and think it's in a decent shape to merge. I just wrote a .spec file and can make RPMs on Copr (including the -devel package). The .desktop file is broken as it does not reference the Qt binary, which is currently solvespaceQt. I'd prefer to rename it to solvespace-qt. Does that sound OK? |
|
Fedora provides mimalloc (2.1.2 on Fedora 37). I'm using that on my debug builds and it seems to work. Is there any particular reason to use the extlib/ version? |
We had some tricky issues and settled on a specific version that worked. If you really want to test a version it's best to turn on address sanitizer and run for a while. |
There was a problem hiding this comment.
@WickedSmoke the Qt port now looks much better. Thank you for your contributions. I tested it on Windows and it compiles and works with a small change. Since you have not enabled maintainer commits in your branch I made a pull request against it here WickedSmoke#2.
Please merge it into your branch so that this pull request gets updated.
Remaining things to do:
- Fix opening and saving files on Windows
- Try it on macOS - I only have a virtual machine with macOS, which is slow and a bit limited. Perhaps @vespakoen could try it.
- Check how the settings storage on Windows (in the registry) interacts with the native version. I'll do this. Currently native goes in
HKEY_CURRENT_USER\SOFTWARE\SolveSpaceand Qt ends up inHKEY_CURRENT_USER\SOFTWARE\SolveSpace\solvespace - On Windows the application icon and window title in the top left corner are missing in the Qt version (but this is very minor and in my opinion not a problem for merging this since Qt will not be the official port)


How is it on Linux? - Figure out how to statically link Qt. As it is the Qt port requires Qt5Core.dll, Qt5Gui.dll and Qt5Widgets.dll to run - a total of 17.2 MB of runtime. But again - this is not a show stopper.
- I still need to actually review the code in guiqt.cpp and guiqt.h
|
And single window UI is a reality with the Qt (and the Web/emscripten) port :-D :-D Once I un-dock the property browser I can not figure out how to dock it again apart from clearing the settings from the Registry. |
|
I spoke a bit too early opening and saving files does not work on Windows... I'll debug... |
It shows up on Linux because of these lines:
You should be able to drag the text window over the left or right sides of the main window to see the docking area appear. I would give up on the idea of settings compatibility. The window & dock settings are stored using custom Qt entries (using |
|
I've created a couple more Linux packages for the Qt version. It is now available for Fedora 40 and Debian 12. The .deb was built with the .spec file and debbuild. For Linux users that have Qt installed using these native packages will save a ton of disk space & RAM as compared to the Flatpak which brings in over 760MB of dependencies. |
rendergl1.cpp ->selectTexture changed id from 1 to 2. It seems that Qt reserves 1 for its own purposes.. not confirmed
…th the rest of the solvespace code reflected the change in src/CMakeLists.txt and the include statement in the src/platform/guiqt.cpp
…e files modified earlier.
When dragging with the LMB the window would often move rather than manipulating the document. This occurred with Qt 5.15 on KDE 5.27.
|
This branch has been rebased on master. The change to rendergl1.cpp has also been reverted so that fewer files get touched. Linux uses the GL3 renderer, but did this do anything on Windows? The Linux version seems to work OK so I would really like to see this work captured on master so I don't have to keep rebasing in the future. As an alternative, there is now a Qt branch in my repo which squashes everything into just four commits and omits the tinyxml2 files from history. |
|
Thank you for rebasing. @WickedSmoke the machine I used to use for SolveSpace development "died" a few months ago and I have not set up Qt on the new one yet. I will do it, debug why opening and saving files fails on Windows and will merge the Qt port. As for which branch - I was thinking of rebasing and squashing some commits anyway so I the "qt" would be easier - you did most of the work already. At the same time I am thinking of committing "karl" as "solvespace_qt5" in the main repository to preserve the detailed history. Building on WindowsAdd |
|
@ruevs as far as you know has this branch been tried on MacOS yet? Qt on my Mac always ends up crashy for some reason... but it seems to be just on my builds. Also what are our thoughts on https://nixos.org? |
|
Merging "qt" and adding a "solvespace_qt5" branch sounds great, but note that there is one commit hash in a log message that has to be manually updated after rebasing "solvespace_qt5" again. I see that Wayland breaks a couple things with Qt 5 here (but of course it does).
[EDIT] Qt 6 improves things on Wayland a bit as the QDockWidget does dock properly. Discussion in the QTBUG above indicates that this may not work on all window managers. For OpenGL, the rendered area is the lower-left 3/4 of the widget. |
|
These changes can no longer be rebased on master as there are conflicts between the shingen75 changes to CMakeLists.txt and commit a208201. Rather than spending time trying to resolve these I'm simply going to remove I'm considering only carrying forward my WickedSmoke:qt branch and leaving WickedSmoke:karl as is. |
The menus don't work properly on Wayland with Qt 5 on Fedora 40.
@WickedSmoke I did exactly that here https://github.com/ruevs/solvespace/tree/qt but have not debugged the problems on Windows yet. |
|
@vthriller There's a fix for the QLineEdit position in commit 2daac18. |
|
2daac18 indeed fixes the issue. Thanks! |
It's actually a bit more interesting: if I Ctrl-Q in the detached property browser, I get the dialog once, then browser window closes, and the main window stays on the screen. (Ditto for when there are no unsaved changes, property browser just hides instead of terminating the whole thing.) |
|
@vthriller Commit 891e7e1 should fix the quit issues so please give it a try. |
|
Yep, that seems to work as expected. |
|
Another year passes and another rebase of the WickedSmoke:qt branch occurs. The 2daac18 & 891e7e1 patches have been squashed into it. The CMakeLists changes are now in their own commit. The Property window behavior on Wayland is still janky with the latest KWin/Qt versions on Fedora 43. The docker window can appear hidden behind the main window. It's rock-solid on X11 as it always remains in front. |
|
@WickedSmoke github here is saying there are still conflicts. This is not my preferred long term direction. I'd prefer GTK4 on all platforms (except web) but that's not going to happen any time soon, so I think we should allow this if it's not too invasive and only built as an option. |
|
@phkahler The pull request was opened on the "karl" branch but the work has moved to the "qt" branch which was just rebased. I can open a new pull request or swap branches around if you like. The only changes to existing files are the cmake files in commit 7a13672. [Edit] I went ahead and opened a new pull request. |
|
I definitely want to have the QT port merged. The only reason it remained hanging is that while it built and ran on Windows it is a bit buggy (see above). Anyway the "karl" branch is worth keeping just for the history, so I will chose this in favour of #1641 |


This builds upon the work of @shingen75 in issue #1406. There is a fair amount of cleanup, the scroll bar was implemented, and a few serious problems fixed (commits 1cc39e3 & 3ce0cee). It has only been built & run on Linux (KDE).