-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SofaGui] Pass QDocBrowser as an option #1315
Conversation
…he computer. Set an option to force it if wanted.
Thanks @epernod but why? |
I did like for QtChart. As it is not a module installed by default in Qt. |
Just my 2 cents, if there is an option to disable it, I'll probably use it even tho I have Qt5WebEngine installed, since I like to keep only the minimum options activated to reduce compilation time and I don't think I'll use this feature. |
Hi all, Thanks Erik for the PR. I'm not opposed to have this kind of option but I think it would be better to have and implementation that is not based on #ifdef (for reason we already discussed a lot). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I would rename SOFAGUIQT_ENABLE_DOCBROWSER into SOFAGUIQT_ENABLE_QDOCBROWSER
- To avoid duplicated variables, use SOFAGUIQT_HAVE_QT5_WEBENGINE in code.
- In applications/sofa/gui/qt/SofaGuiQt.h.in, you have to add:
#cmakedefine01 SOFAGUIQT_HAVE_QT5_WEBENGINE
#QDocBrowser | ||
option(SOFAGUIQT_ENABLE_DOCBROWSER "Build the QDocBrowser. Qt5WebEngine is needed." OFF) | ||
if (SOFAGUIQT_ENABLE_DOCBROWSER) | ||
sofa_find_package(Qt5 COMPONENTS WebEngine WebEngineWidgets REQUIRED BOTH_SCOPES) | ||
else() | ||
sofa_find_package(Qt5 COMPONENTS WebEngine WebEngineWidgets QUIET BOTH_SCOPES) | ||
endif() | ||
sofa_set_01(SOFAGUIQT_HAVE_DOCBROWSER VALUE ${SOFAGUIQT_ENABLE_DOCBROWSER} BOTH_SCOPES) | ||
|
||
if (Qt5WebEngine_FOUND) | ||
set(QT_TARGETS ${QT_TARGETS} Qt5::WebEngine Qt5::WebEngineWidgets) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be enough:
# QDocBrowser
option(SOFAGUIQT_ENABLE_QDOCBROWSER "Build the QDocBrowser. Qt5WebEngine is needed." OFF)
if (SOFAGUIQT_ENABLE_QDOCBROWSER)
sofa_find_package(Qt5 COMPONENTS WebEngine WebEngineWidgets REQUIRED BOTH_SCOPES)
set(QT_TARGETS ${QT_TARGETS} Qt5::WebEngine Qt5::WebEngineWidgets)
endif()
If a solution is not found on the crash closing SOFA, I really ++++ this PR |
…check if (SOFAGUIQT_HAVE_QT5_WEBENGINE) in code
I added the missing "enable by default if QDocBrowser is found" part 😉 |
It does fix my bug (prev!) @fredroy 👍 |
This PR:
Reviewers will merge only if all these checks are true.