Skip to content
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

cxx-qt-gen: remove generation of wrapper for emitting signals #556

Merged

Conversation

ahayzen-kdab
Copy link
Collaborator

@ahayzen-kdab ahayzen-kdab commented May 29, 2023

This is now unused as the type conversion has been removed.

Requires #554
Requires #571

@ahayzen-kdab
Copy link
Collaborator Author

This doesn't work yet as we have two different cases for inheriting signals

  • inherit a signal from the base class
  • inherit a signal as a property has generated it

For the first we need to generate a connect + emit method, the second we need to generate a connect method but no emit as the qproperty has done this already.

    #[cxx_qt::qsignals(CustomBaseClass)]
    pub enum Signals<'a> {
        /// Inherit the DataChanged signal from the QAbstractListModel base class
        #[inherit]
        DataChanged {
            /// Top left affected index
            top_left: &'a QModelIndex,
            /// Bottom right affected index
            bottom_right: &'a QModelIndex,
            /// Roles that have been modified
            roles: &'a QVector_i32,
        },
    }

    #[cxx_qt::qsignals(RustSignals)]
    pub enum Connection<'a> {
        // Example of using #[inherit] so that connections to the logging_enabled property can be made
        #[inherit]
        // We don't ever emit this enum, so silence clippy warnings
        #[allow(dead_code)]
        /// The Q_SIGNAL emitted when the Q_PROPERTY logging_enabled changes
        LoggingEnabledChanged,
    }

@ahayzen-kdab
Copy link
Collaborator Author

This will be solved by the new API as in that when there is a qproperty a myPropertyChanged signal will be generated and the connect method. So we don't have the enum craziness.

@ahayzen-kdab ahayzen-kdab force-pushed the no-wrapper-for-signals-emitter branch 2 times, most recently from 5e84ebf to a482d20 Compare June 2, 2023 16:21
@ahayzen-kdab
Copy link
Collaborator Author

Requires #571

@ahayzen-kdab ahayzen-kdab force-pushed the no-wrapper-for-signals-emitter branch from a482d20 to 0be3a72 Compare June 5, 2023 11:00
@ahayzen-kdab ahayzen-kdab added the 🥳🎉 1.0 This issue is part of stabilization for 1.0 release label Jun 5, 2023
@ahayzen-kdab ahayzen-kdab force-pushed the no-wrapper-for-signals-emitter branch 2 times, most recently from e8a8729 to 6487d11 Compare June 7, 2023 14:06
@ahayzen-kdab ahayzen-kdab marked this pull request as ready for review June 7, 2023 14:06
@ahayzen-kdab
Copy link
Collaborator Author

This now works and ready for review :-)

This is now unused as the type conversion has been removed.

Related to KDAB#289
@ahayzen-kdab ahayzen-kdab force-pushed the no-wrapper-for-signals-emitter branch from 6487d11 to d7fc274 Compare June 8, 2023 08:57
Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

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

LGTM, always good to see things simplified.

@ahayzen-kdab ahayzen-kdab merged commit c663fbf into KDAB:main Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🥳🎉 1.0 This issue is part of stabilization for 1.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants