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

Implement Debug for all types that work with QDebug #1162

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jnbooth
Copy link
Contributor

@jnbooth jnbooth commented Jan 18, 2025

This PR adds Debug implementations to all types that a QDebug data stream accepts.

Note: Some types incorrectly used QDebug output to implement Display instead of Debug. This PR does not remove those implementations because that would be a breaking change, but they probably should be removed for the next major release.

@jnbooth jnbooth changed the title Debug and format Implement Debug for all types that work with QDebug Jan 18, 2025
@jnbooth jnbooth changed the title Implement Debug for all types that work with QDebug Implement Debug for all types that work with QDebug Jan 18, 2025
@jnbooth jnbooth changed the title Implement Debug for all types that work with QDebug Implement Debug for all types that work with QDebug Jan 18, 2025
Copy link

codecov bot commented Jan 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e84662a) to head (bb6f2e5).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1162   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           71        71           
  Lines        11967     11967           
=========================================
  Hits         11967     11967           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Hi @jnbooth, thank you for this contribution!
It's great to see you're using CXX-Qt actively and I'm very happy to see the improvements that come out of it 😃

This PR mostly looks good. I believe it can be simplified a little bit more, as the toQString function should actually not be necessary anymore, and there are a few nitpicks to take care of.

Then we should be able to merge this 🥳

@@ -79,6 +111,13 @@ mod ffi {
#[rust_name = "qtimezone_default"]
fn qtimezoneDefault() -> UniquePtr<QTimeZone>;
#[doc(hidden)]
#[rust_name = "qtimezone_display_name"]
fn qtimezoneDisplayName(
Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB Jan 20, 2025

Choose a reason for hiding this comment

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

Is there a reason why this isn't wrapped as a member function?

If not, please replace it with a member function in CXX, then you can also remove the added code in the C++ header and source :)
e.g.:

        fn qtimezoneDisplayName(
            self: &QTimeZone,
            time_type: QTimeZoneTimeType,
            name_type: QTimeZoneNameType,
        ) -> QString;

Comment on lines +36 to +48
namespace rust {
namespace cxxqtlib1 {
const QVector<QPoint>&
qpolygonAsQVectorQPointRef(const QPolygon& shape)
{
return static_cast<const QVector<QPoint>&>(shape);
}

QVector<QPoint>&
qpolygonAsQVectorQPointRef(QPolygon& shape)
{
return static_cast<QVector<QPoint>&>(shape);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I appreciate the effort here to also allow upcasting the QPolygon to it's base class QList<QPoint>.

Note that we will replace this implementation with the generalized version that @BenFordTytherington is working one once it lands:
#1159
For now we can keep it as it should be compatible API-wise


#[doc(hidden)]
#[rust_name = "QAnyStringView_to_qstring"]
fn toQString(string: &QAnyStringView) -> QString;
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is already a to_string function on QAnyStringView (see line 34) which you can use.

@@ -33,6 +33,13 @@ drop(T& value)
template<typename T>
QString
toQString(const T& value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This template should no longer be necessary.
If toString() is already a member function, we don't need to create a free function toQString, but can just access it directly via CXX:
i.e.:

// Maybe with #[doc(hidden)], depending on the situation
#[rust_name="to_qstring"]
fn toQString(self: &T);

Please remove this function and replace the existing references with direct member access.

Copy link
Contributor Author

@jnbooth jnbooth Jan 20, 2025

Choose a reason for hiding this comment

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

The idea of this template is to allow types to have more flexibility in how they define to_qstring(). Some types have extra parameters for toString. For others, we might not want to have a to_qstring method at all (rather than relying on #[doc(hidden)]. Or we might decide not to have methods like that in general and use From<&Type> for QString, which is more idiomatic. So by using this template, types aren't "locked in" to a particular implementation; we can keep the Display logic separate from everything else.

That said, I of course defer to you. What should be the rubric for whether or not to use #[doc(hidden)]?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants