Skip to content

Add Qt 5 GUI backend#1451

Closed
WickedSmoke wants to merge 39 commits intosolvespace:masterfrom
WickedSmoke:karl
Closed

Add Qt 5 GUI backend#1451
WickedSmoke wants to merge 39 commits intosolvespace:masterfrom
WickedSmoke:karl

Conversation

@WickedSmoke
Copy link
Contributor

@WickedSmoke WickedSmoke commented Mar 1, 2024

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).

@WickedSmoke
Copy link
Contributor Author

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:

78% Compile src/textscreens.cpp
In file included from src/undoredo.cpp:8:
src/solvespace.h:182:28: warning: ‘template<class _Category, class _Tp, class _Distance, class _Pointer, class _Reference> struct std::iterator’ is deprecated [-Wdeprecated-declarations]
  182 | class utf8_iterator : std::iterator<std::forward_iterator_tag, char32_t> {
      |                            ^~~~~~~~
In file included from /usr/include/c++/12/bits/stl_construct.h:61,
                 from /usr/include/c++/12/bits/alloc_traits.h:33,
                 from /usr/include/c++/12/ext/alloc_traits.h:34,
                 from /usr/include/c++/12/unordered_map:41,
                 from /usr/include/c++/12/functional:61,
                 from src/resource.h:10,
                 from src/solvespace.h:10:
/usr/include/c++/12/bits/stl_iterator_base_types.h:127:34: note: declared here
  127 |     struct _GLIBCXX17_DEPRECATED iterator
      |                                  ^~~~~~~~

@WickedSmoke WickedSmoke force-pushed the karl branch 2 times, most recently from b25bff3 to 4e67770 Compare March 3, 2024 04:24
Copy link
Member

@ruevs ruevs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

void OpenInBrowser(const std::string &url);

std::vector<std::string> InitGui(int argc, char **argv);
std::vector<std::string> InitGui(int& argc, char **argv);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why int& argc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Copy link
Contributor Author

@WickedSmoke WickedSmoke Mar 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Gtk::Main *gtkMain;

std::vector<std::string> InitGui(int argc, char **argv) {
std::vector<std::string> InitGui(int& argc, char **argv) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why int& argc?

static SSApplicationDelegate *ssDelegate;

std::vector<std::string> InitGui(int argc, char **argv) {
std::vector<std::string> InitGui(int& argc, char **argv) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why int& argc?

void OpenInBrowser(const std::string &url) {}

std::vector<std::string> InitGui(int argc, char **argv) {
std::vector<std::string> InitGui(int& argc, char **argv) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why int& argc?

}

std::vector<std::string> InitGui(int argc, char **argv) {
std::vector<std::string> InitGui(int& argc, char **argv) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why int& argc?

}

std::vector<std::string> InitGui(int argc, char **argv) {
std::vector<std::string> InitGui(int& argc, char **argv) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why int& argc?

${PNG_LIBRARY}
${FREETYPE_LIBRARY}
${CAIRO_LIBRARIES}
#${CAIRO_LIBRARIES}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

@ruevs
Copy link
Member

ruevs commented Mar 3, 2024

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:

78% Compile src/textscreens.cpp
In file included from src/undoredo.cpp:8:
src/solvespace.h:182:28: warning: ‘template<class _Category, class _Tp, class _Distance, class _Pointer, class _Reference> struct std::iterator’ is deprecated [-Wdeprecated-declarations]
  182 | class utf8_iterator : std::iterator<std::forward_iterator_tag, char32_t> {
      |                            ^~~~~~~~
In file included from /usr/include/c++/12/bits/stl_construct.h:61,
                 from /usr/include/c++/12/bits/alloc_traits.h:33,
                 from /usr/include/c++/12/ext/alloc_traits.h:34,
                 from /usr/include/c++/12/unordered_map:41,
                 from /usr/include/c++/12/functional:61,
                 from src/resource.h:10,
                 from src/solvespace.h:10:
/usr/include/c++/12/bits/stl_iterator_base_types.h:127:34: note: declared here
  127 |     struct _GLIBCXX17_DEPRECATED iterator
      |                                  ^~~~~~~~

This is known. std::iterator was deprecated in C++17, but at this point SolveSopace is strictly C++11. In C++11 it is not deprecated and the warning does not happen.

Can the QT version be compiled in C++11 mode? If not the warning is benign anyway.

@WickedSmoke
Copy link
Contributor Author

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.

@WickedSmoke
Copy link
Contributor Author

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.

@phkahler
Copy link
Member

phkahler commented Mar 4, 2024

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.

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.

@WickedSmoke
Copy link
Contributor Author

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?

@WickedSmoke
Copy link
Contributor Author

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?

@phkahler
Copy link
Member

phkahler commented Mar 8, 2024

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.

Copy link
Member

@ruevs ruevs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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\SolveSpace and Qt ends up in HKEY_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)
    image
    image
    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

@ruevs
Copy link
Member

ruevs commented Mar 18, 2024

And single window UI is a reality with the Qt (and the Web/emscripten) port :-D :-D
image

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.

@ruevs
Copy link
Member

ruevs commented Mar 18, 2024

I spoke a bit too early opening and saving files does not work on Windows... I'll debug...

@WickedSmoke
Copy link
Contributor Author

WickedSmoke commented Mar 18, 2024

  • On Windows the application icon and window title in the top left corner are missing

It shows up on Linux because of these lines:

#ifdef __linux
        QIcon icon("/usr/share/icons/hicolor/48x48/apps/solvespace.png");
        setWindowIcon(icon);
#endif

Once I un-dock the property browser I can not figure out how to dock it again

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 saveGeometry & saveState).

@WickedSmoke
Copy link
Contributor Author

WickedSmoke commented May 10, 2024

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.

shingen75 and others added 9 commits September 5, 2024 12:48
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
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.
@WickedSmoke
Copy link
Contributor Author

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.

@ruevs
Copy link
Member

ruevs commented Sep 9, 2024

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 Windows

Add CMAKE_PREFIX_PATH = [path to your Qt]/Qt/Qt5.12.12/5.12.12/msvc2017_64/lib/cmake/Qt5 to the CMake command line to make it work.

@dgramop
Copy link
Contributor

dgramop commented Sep 12, 2024

@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?

@ruevs
Copy link
Member

ruevs commented Sep 12, 2024

@ruevs as far as you know has this branch been tried on MacOS yet?

@dgramop as far as I am aware it has not been. As I mentioned my only macOS is a VirtualBox VM - slow and thus does not stimulate me to use it :-)

@WickedSmoke
Copy link
Contributor Author

WickedSmoke commented Sep 21, 2024

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).

  1. The OpenGL widgets are not being resized properly, so the rendered area only occupies the lower-left quadrant of the widget.
  2. The TextWindow cannot be re-docked with the main window. See https://bugreports.qt.io/browse/QTBUG-87332.

[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.

@WickedSmoke
Copy link
Contributor Author

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
all the CMake changes currently in commit ec7a2cb as most get reverted later.

I'm considering only carrying forward my WickedSmoke:qt branch and leaving WickedSmoke:karl as is.

@WickedSmoke WickedSmoke mentioned this pull request Dec 11, 2024
@ruevs
Copy link
Member

ruevs commented Feb 26, 2025

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
all the CMake changes currently in commit ec7a2cb as most get reverted later.

@WickedSmoke I did exactly that here https://github.com/ruevs/solvespace/tree/qt but have not debugged the problems on Windows yet.

@ruevs ruevs added the UI label May 2, 2025
@ruevs ruevs mentioned this pull request Sep 4, 2025
@ruevs ruevs linked an issue Sep 4, 2025 that may be closed by this pull request
@vthriller
Copy link
Contributor

vthriller commented Nov 29, 2025

No idea what branch are you going to merge so I tried all three mentioned here. Nitpicks so far:

  • On Wayland with scale of 1.5, text input is rendered at offset. Not reproducible on X11 or when scale is 1.
  • If I'm not saving sketch on exit, I am asked again whether I want to save it or not.

Other than that, this is pretty impressive.

@WickedSmoke
Copy link
Contributor Author

WickedSmoke commented Nov 30, 2025

@vthriller There's a fix for the QLineEdit position in commit 2daac18.

@vthriller
Copy link
Contributor

2daac18 indeed fixes the issue. Thanks!

@vthriller
Copy link
Contributor

If I'm not saving sketch on exit, I am asked again whether I want to save it or not.

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.)

@WickedSmoke
Copy link
Contributor Author

@vthriller Commit 891e7e1 should fix the quit issues so please give it a try.

@vthriller
Copy link
Contributor

891e7e1

Yep, that seems to work as expected.

@WickedSmoke
Copy link
Contributor Author

WickedSmoke commented Dec 1, 2025

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.

@phkahler
Copy link
Member

phkahler commented Dec 1, 2025

@WickedSmoke github here is saying there are still conflicts.
@ruevs I haven't been following this (or much else lately) what do you think?
I really hate to leave people maintaining things on their own branches - it's an indication that the maintained thing is important to them and we should try to get it in. I also like the docked text window ;-) I don't even care if it's removable so long as it can hide.

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.

@WickedSmoke
Copy link
Contributor Author

WickedSmoke commented Dec 1, 2025

@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.

@WickedSmoke WickedSmoke mentioned this pull request Dec 1, 2025
@ruevs
Copy link
Member

ruevs commented Dec 2, 2025

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

@ruevs ruevs closed this Dec 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Qt5 UI for SolveSpace

6 participants