Refactor: comment for MODMESH_PYSIDE6_FULL macro#592
Refactor: comment for MODMESH_PYSIDE6_FULL macro#592Telomelonia wants to merge 1 commit intosolvcon:masterfrom
Conversation
yungyuc
left a comment
yungyuc
left a comment
There was a problem hiding this comment.
Thank you, it is a good implementation. But building with PySide6 became a requirement since 2023 (see issue #220). Now it is not making sense to offer a build option to turn it off.
It would make sense to explicitly say that we require PySide6 in the code base. Maybe a cmakelists.txt could be an option.
Telomelonia
force-pushed
the
feature/enable-pyside-ci
branch
from
ad81e22 to
f9b1225
Compare
yungyuc
left a comment
yungyuc
left a comment
There was a problem hiding this comment.
The cpp macro MODMESH_USE_PYSIDE is a better place for the comment. Sorry for giving a contradicting comment previously.
CMakeLists.txt
Outdated
| option(BUILD_QT "build with QT" ON) | ||
| message(STATUS "BUILD_QT: ${BUILD_QT}") | ||
|
|
||
| # PySide6 is required since 2023 (see issue #220) |
There was a problem hiding this comment.
Could you please add the full issue URL?
CMakeLists.txt
Outdated
|
|
||
| include_directories(${MODMESH_INCLUDE_DIR}) | ||
|
|
||
| # PySide6 is mandatory - always enabled |
There was a problem hiding this comment.
Since we really have a cpp macro MODMESH_USE_PYSIDE, it will be more explicit to add the link to the old PYSIDE-on-by-default issue there.
I thought cmakelists.txt could be a good place for the comment but I was wrong. My apologies.
Telomelonia
force-pushed
the
feature/enable-pyside-ci
branch
from
f9b1225 to
5a0fed0
Compare
|
This is where a CPP macro of using PYSIDE is used: modmesh/cpp/modmesh/pilot/wrap_pilot.cpp Line 39 in e3cdb4e It is not named like |
Telomelonia
force-pushed
the
feature/enable-pyside-ci
branch
from
5a0fed0 to
eae7d24
Compare
yungyuc
left a comment
yungyuc
left a comment
There was a problem hiding this comment.
The comment in wrap_pilot.cpp will work the best. Please revert the change in other files.
Telomelonia
force-pushed
the
feature/enable-pyside-ci
branch
from
eae7d24 to
9424ac1
Compare
|
@yungyuc Ready for review, made some changes |
CMakeLists.txt
Outdated
| include_directories(${MODMESH_INCLUDE_DIR}) | ||
|
|
||
| # PySide6 is required since 2023 (see issue https://github.com/solvcon/modmesh/issues/220) | ||
| add_compile_definitions(MODMESH_USE_PYSIDE) |
There was a problem hiding this comment.
This comment duplicates with that in the source code.
There was a problem hiding this comment.
After discussion, I removed the compile definitions for MODMESH_USE_PYSIDE and comment
refactor: remove the switch refactor: comments refactor: update the comment for the PySide6 Change the comment line and remove msg remove unnecesary code
Telomelonia
force-pushed
the
feature/enable-pyside-ci
branch
from
9424ac1 to
b3dece6
Compare
Telomelonia
changed the title
yungyuc
left a comment
yungyuc
left a comment
There was a problem hiding this comment.
Keep both old and new comments.
| #include <QMenu> | ||
|
|
||
| // Usually MODMESH_PYSIDE6_FULL is not defined unless for debugging. | ||
| // PySide6 is required since 2023 (see issue https://github.com/solvcon/modmesh/issues/220) |
There was a problem hiding this comment.
The old comment should be kept. It should read like:
// PySide6 is required since 2023 (see issue https://github.com/solvcon/modmesh/issues/220)
// Usually MODMESH_PYSIDE6_FULL is not defined unless for debugging.2 participants
No description provided.