Skip to content

Refactor: comment for MODMESH_PYSIDE6_FULL macro #592

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor: comment for MODMESH_PYSIDE6_FULL macro #592

wants to merge 1 commit into from

Conversation

@Telomelonia
Copy link

@Telomelonia Telomelonia commented Sep 28, 2025 •
edited
Loading

No description provided.

@Telomelonia Telomelonia mentioned this pull request Sep 28, 2025
Open
2 tasks
@Telomelonia Telomelonia marked this pull request as ready for review September 28, 2025 06:22
yungyuc
yungyuc requested changes Sep 28, 2025
View reviewed changes
Copy link
Member

@yungyuc yungyuc left a comment

Choose a reason for hiding this comment

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

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.

@yungyuc yungyuc added the documentation Improvements or additions to documentation label Sep 28, 2025
@Telomelonia Telomelonia force-pushed the feature/enable-pyside-ci branch from ad81e22 to f9b1225 Compare September 28, 2025 07:00
@Telomelonia Telomelonia requested a review from yungyuc September 28, 2025 07:00
yungyuc
yungyuc requested changes Sep 28, 2025
View reviewed changes
Copy link
Member

@yungyuc yungyuc left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Could you please add the full issue URL?

CMakeLists.txt Outdated

include_directories(${MODMESH_INCLUDE_DIR})

# PySide6 is mandatory - always enabled
Copy link
Member

Choose a reason for hiding this comment

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

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 Telomelonia force-pushed the feature/enable-pyside-ci branch from f9b1225 to 5a0fed0 Compare September 28, 2025 07:36
@yungyuc
Copy link
Member

yungyuc commented Sep 28, 2025 •
edited
Loading

This is where a CPP macro of using PYSIDE is used:

modmesh/cpp/modmesh/pilot/wrap_pilot.cpp

Line 39 in e3cdb4e

// Usually MODMESH_PYSIDE6_FULL is not defined unless for debugging.

It is not named like MODMESH_USE_PYSIDE. My apologies for misremembering that too.

@Telomelonia Telomelonia force-pushed the feature/enable-pyside-ci branch from 5a0fed0 to eae7d24 Compare September 28, 2025 07:59
yungyuc
yungyuc requested changes Sep 28, 2025
View reviewed changes
Copy link
Member

@yungyuc yungyuc left a comment

Choose a reason for hiding this comment

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

The comment in wrap_pilot.cpp will work the best. Please revert the change in other files.

@Telomelonia Telomelonia force-pushed the feature/enable-pyside-ci branch from eae7d24 to 9424ac1 Compare September 28, 2025 08:12
@Telomelonia
Copy link
Author

@yungyuc Ready for review, made some changes

@Telomelonia Telomelonia requested a review from yungyuc September 28, 2025 08:13
yungyuc
yungyuc reviewed Sep 28, 2025
View reviewed 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)
Copy link
Member

Choose a reason for hiding this comment

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

This comment duplicates with that in the source code.

Copy link
Author

@Telomelonia Telomelonia Sep 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

After discussion, I removed the compile definitions for MODMESH_USE_PYSIDE and comment

@Telomelonia
feat: enable MODMESH_USE_PYSIDE to on, by default
b3dece6 
refactor: remove the switch

refactor: comments

refactor: update the comment for the PySide6

Change the comment line and remove msg

remove unnecesary code
@Telomelonia Telomelonia force-pushed the feature/enable-pyside-ci branch from 9424ac1 to b3dece6 Compare September 28, 2025 08:26
@Telomelonia Telomelonia requested a review from yungyuc September 28, 2025 08:29
@Telomelonia Telomelonia changed the title Enable MODMESH_USE_PYSIDE for all CI environments Refactor: comment for MODMESH_PYSIDE6_FULL macro Sep 28, 2025
yungyuc
yungyuc requested changes Sep 28, 2025
View reviewed changes
Copy link
Member

@yungyuc yungyuc left a comment

Choose a reason for hiding this comment

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

Keep both old and new comments.

cpp/modmesh/pilot/wrap_pilot.cpp
#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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yungyuc yungyuc yungyuc requested changes

Assignees

@Telomelonia Telomelonia

Labels

documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Telomelonia @yungyuc