From ce104cac500096734a94e6d132e342d07d7e8af0 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Thu, 12 Jan 2023 11:33:30 +0100 Subject: [PATCH] QPermission: replace T data() with std::optional value() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As discussed in API review, the default-constructed T() returned from a mismatched data() call is indistinguishable from a real T with default state. To make them distinguishable, return optional. Call the new function value(), mimicking QVariant::value(), and suggested in API review, because data() is usually used to return raw pointers, not values. Remove the qWarning() on requestedType and actualType mismatch, as the new function can be used in std::get_if/dynamic_cast-like if-then-else chains, in which failure is part of the normal operation, and a warning message misplaced: if (auto loc = perm.value()) ~~~ use *loc ~~~ else if (auto con = perm.value()) ~~~ use *con ~~~ ~~~ etc ~~~ Pick-to: 6.5 Change-Id: I799a58e930307323ebce8f9ac50a42455e9c017f Reviewed-by: Tor Arne Vestbø Reviewed-by: Qt CI Bot --- src/corelib/kernel/qpermissions.cpp | 18 ++++----- src/corelib/kernel/qpermissions.h | 7 ++-- src/corelib/kernel/qpermissions_android.cpp | 6 +-- src/corelib/kernel/qpermissions_wasm.cpp | 2 +- .../qdarwinpermissionplugin_location.mm | 6 +-- .../kernel/qpermission/tst_qpermission.cpp | 39 ++++++++++++------- 6 files changed, 42 insertions(+), 36 deletions(-) diff --git a/src/corelib/kernel/qpermissions.cpp b/src/corelib/kernel/qpermissions.cpp index 3f97154921..e67b50c10b 100644 --- a/src/corelib/kernel/qpermissions.cpp +++ b/src/corelib/kernel/qpermissions.cpp @@ -211,8 +211,8 @@ Q_LOGGING_CATEGORY(lcPermissions, "qt.permissions", QtWarningMsg); { if (permission.status() != Qt::PermissionStatus:Granted) return; - auto locationPermission = permission.data(); - if (locationPermission.accuracy() != QLocationPermission::Precise) + auto locationPermission = permission.value(); + if (!locationPermission || locationPermission->accuracy() != QLocationPermission::Precise) return; updatePreciseLocation(); } @@ -243,13 +243,12 @@ Q_LOGGING_CATEGORY(lcPermissions, "qt.permissions", QtWarningMsg); */ /*! - \fn template > T QPermission::data() const + \fn template > std::optional QPermission::value() const - Returns the \l{typed permission} of type \c T. + Returns the \l{typed permission} of type \c T, or \c{std::nullopt} if this + QPermission object doesn't contain one. - If the type doesn't match the type that was originally used to request the - permission, returns a default-constructed \c T. Use type() for dynamically - choosing which typed permission to request. + Use type() for dynamically choosing which typed permission to request. This function participates in overload resolution only if \c T is one of the \l{typed permission} classes: @@ -273,11 +272,8 @@ Q_LOGGING_CATEGORY(lcPermissions, "qt.permissions", QtWarningMsg); const void *QPermission::data(QMetaType requestedType) const { const auto actualType = type(); - if (requestedType != actualType) { - qCWarning(lcPermissions, "Cannot convert from %s to %s", - actualType.name(), requestedType.name()); + if (requestedType != actualType) return nullptr; - } return m_data.data(); } diff --git a/src/corelib/kernel/qpermissions.h b/src/corelib/kernel/qpermissions.h index 9c47df3eb1..7f56e9625e 100644 --- a/src/corelib/kernel/qpermissions.h +++ b/src/corelib/kernel/qpermissions.h @@ -16,6 +16,8 @@ #include #include +#include + #if !defined(Q_QDOC) QT_REQUIRE_CONFIG(permissions); #endif @@ -52,12 +54,11 @@ public: QMetaType type() const { return m_data.metaType(); } template = true> - T data() const + std::optional value() const { if (auto p = data(QMetaType::fromType())) return *static_cast(p); - else - return T{}; + return std::nullopt; } #ifndef QT_NO_DEBUG_STREAM diff --git a/src/corelib/kernel/qpermissions_android.cpp b/src/corelib/kernel/qpermissions_android.cpp index 9b3aa07db7..a0899ab673 100644 --- a/src/corelib/kernel/qpermissions_android.cpp +++ b/src/corelib/kernel/qpermissions_android.cpp @@ -53,7 +53,7 @@ static QStringList nativeStringsFromPermission(const QPermission &permission) { const auto id = permission.type().id(); if (id == qMetaTypeId()) { - return nativeLocationPermission(permission.data()); + return nativeLocationPermission(*permission.value()); } else if (id == qMetaTypeId()) { return { u"android.permission.CAMERA"_s }; } else if (id == qMetaTypeId()) { @@ -63,12 +63,12 @@ static QStringList nativeStringsFromPermission(const QPermission &permission) return { u"android.permission.BLUETOOTH"_s }; } else if (id == qMetaTypeId()) { const auto readContactsString = u"android.permission.READ_CONTACTS"_s; - if (!permission.data().isReadWrite()) + if (!permission.value()->isReadWrite()) return { readContactsString }; return { readContactsString, u"android.permission.WRITE_CONTACTS"_s }; } else if (id == qMetaTypeId()) { const auto readContactsString = u"android.permission.READ_CALENDAR"_s; - if (!permission.data().isReadWrite()) + if (!permission.value()->isReadWrite()) return { readContactsString }; return { readContactsString, u"android.permission.WRITE_CALENDAR"_s }; } diff --git a/src/corelib/kernel/qpermissions_wasm.cpp b/src/corelib/kernel/qpermissions_wasm.cpp index 60e2def853..175fc89dd0 100644 --- a/src/corelib/kernel/qpermissions_wasm.cpp +++ b/src/corelib/kernel/qpermissions_wasm.cpp @@ -216,7 +216,7 @@ namespace Q_ASSERT(!geolocation.isNull()); const auto &permission = geolocationRequestQueue->front().first; - const auto &locationPermission = permission.data(); + const auto locationPermission = *permission.value(); const bool highAccuracy = locationPermission.accuracy() == QLocationPermission::Precise; val options = val::object(); diff --git a/src/corelib/platform/darwin/qdarwinpermissionplugin_location.mm b/src/corelib/platform/darwin/qdarwinpermissionplugin_location.mm index cf27a9e837..39bae8cccb 100644 --- a/src/corelib/platform/darwin/qdarwinpermissionplugin_location.mm +++ b/src/corelib/platform/darwin/qdarwinpermissionplugin_location.mm @@ -55,7 +55,7 @@ struct PermissionRequest - (Qt::PermissionStatus)checkPermission:(QPermission)permission { - const auto locationPermission = permission.data(); + const auto locationPermission = *permission.value(); auto status = [self authorizationStatus:locationPermission]; if (status != Qt::PermissionStatus::Granted) @@ -118,7 +118,7 @@ struct PermissionRequest - (QStringList)usageDescriptionsFor:(QPermission)permission { QStringList usageDescriptions = { "NSLocationWhenInUseUsageDescription" }; - const auto locationPermission = permission.data(); + const auto locationPermission = *permission.value(); if (locationPermission.availability() == QLocationPermission::Always) usageDescriptions << "NSLocationAlwaysUsageDescription"; return usageDescriptions; @@ -150,7 +150,7 @@ struct PermissionRequest self.manager.delegate = self; } - const auto locationPermission = permission.data(); + const auto locationPermission = *permission.value(); switch (locationPermission.availability()) { case QLocationPermission::WhenInUse: // The documentation specifies that requestWhenInUseAuthorization can diff --git a/tests/auto/corelib/kernel/qpermission/tst_qpermission.cpp b/tests/auto/corelib/kernel/qpermission/tst_qpermission.cpp index fa3d444e80..9eb7f7e829 100644 --- a/tests/auto/corelib/kernel/qpermission/tst_qpermission.cpp +++ b/tests/auto/corelib/kernel/qpermission/tst_qpermission.cpp @@ -54,11 +54,12 @@ void tst_QPermission::converting_impl() const QCOMPARE_EQ(p.type(), metaType); } - // data<>() compiles: + // value<>() compiles: { const QPermission p = concrete; - [[maybe_unused]] auto r = p.data(); - static_assert(std::is_same_v); + auto v = p.value(); + static_assert(std::is_same_v>); + QCOMPARE_NE(v, std::nullopt); } } @@ -100,35 +101,43 @@ void tst_QPermission::conversionMaintainsState() const { p = dummy; - auto r = p.data(); + auto v = p.value(); + QCOMPARE_NE(v, std::nullopt); + auto &r = *v; QCOMPARE_EQ(r.state, dummy.state); - // check mismatched returns default-constructed value: - QCOMPARE_EQ(p.data().isReadWrite(), cal_default.isReadWrite()); + // check mismatched returns nullopt: + QCOMPARE_EQ(p.value(), std::nullopt); } { p = loc; - auto r = p.data(); + auto v = p.value(); + QCOMPARE_NE(v, std::nullopt); + auto &r = *v; QCOMPARE_EQ(r.accuracy(), loc.accuracy()); QCOMPARE_EQ(r.availability(), loc.availability()); - // check mismatched returns default-constructed value: - QCOMPARE_EQ(p.data().state, dummy_default.state); + // check mismatched returns nullopt: + QCOMPARE_EQ(p.value(), std::nullopt); } { p = con; - auto r = p.data(); + auto v = p.value(); + QCOMPARE_NE(v, std::nullopt); + auto &r = *v; QCOMPARE_EQ(r.isReadWrite(), con.isReadWrite()); - // check mismatched returns default-constructed value: - QCOMPARE_EQ(p.data().accuracy(), loc_default.accuracy()); + // check mismatched returns nullopt: + QCOMPARE_EQ(p.value(), std::nullopt); } { p = cal; - auto r = p.data(); + auto v = p.value(); + QCOMPARE_NE(v, std::nullopt); + auto &r = *v; QCOMPARE_EQ(r.isReadWrite(), cal.isReadWrite()); - // check mismatched returns default-constructed value: - QCOMPARE_EQ(p.data().isReadWrite(), con_default.isReadWrite()); + // check mismatched returns nullopt: + QCOMPARE_EQ(p.value(), std::nullopt); } }