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

Add support for Qt6 #8

Closed
wants to merge 3 commits into from
Closed

Conversation

Herrie82
Copy link

@Herrie82 Herrie82 commented Jun 8, 2022

Tested with Qt 6.3 and Qt 5.15, no older versions are available for testing here at my end.

Signed-off-by: Herman van Hazendonk github.com@herrie.org

Deprecated for a while, no longer working in Qt6

Fixes compilation issues due deprecated usage:
../../../git/src/nemo-dbus/connection.cpp:255:22: error: invalid use of non-static member function 'const QLoggingCategory& NemoDBus::ConnectionData::logs()'
|   255 |         qCWarning(d->logs) << "Failed to register object on path" << path << object;

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
Fixes:

../../../git/src/plugin/declarativedbusinterface.cpp:827:39: error: 'class QJSValue' has no member named 'qjsEngine'
  827 |         callbackArguments << callback.qjsEngine()->toScriptValue<QVariant>(NemoDBus::demarshallDBusArgument(argument));
      |                                       ^~~~~~~~~
../../../git/src/plugin/declarativedbusinterface.cpp:827:74: error: expected primary-expression before '>' token
  827 |         callbackArguments << callback.qjsEngine()->toScriptValue<QVariant>(NemoDBus::demarshallDBusArgument(argument));
      |                                                                          ^

As per: https://forum.qt.io/topic/115844/qjsvalue-engine-deprecated-in-qt5-15

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
Copy link

@Thaodan Thaodan left a comment

Choose a reason for hiding this comment

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

Isn't more porting away than fixing?

@Thaodan
Copy link

Thaodan commented Jun 9, 2022

A note on your Qt6 porting:
When something breaks research when a method got deprecated, remove the old method if it was deprecated before the current Qt version supported by Sailfish OS (5.6) if not add an ifdef for Qt 5.6 and Qt <6 if necessary.
When you commit the changes provide the context you found and link Qt documentation if necessary.

@Herrie82
Copy link
Author

Herrie82 commented Jun 9, 2022

A note on your Qt6 porting: When something breaks research when a method got deprecated, remove the old method if it was deprecated before the current Qt version supported by Sailfish OS (5.6) if not add an ifdef for Qt 5.6 and Qt <6 if necessary. When you commit the changes provide the context you found and link Qt documentation if necessary.

It's not always easy for me to determine when things were deprecated. I don't have 5.6 at my end to test. Qt's documentation isn't always accurate or complete in this regard. It seems QLoggingCategory was introduced with 5.2, but not sure if this builds with 5.6 or not since I have no way of testing it.

@@ -45,11 +45,13 @@ ConnectionData::ConnectionData(const QDBusConnection &connection, const QLogging
, m_logs(logs)
{
if (connection.isConnected()) {
qCDebug(logs, "Connected to %s", qPrintable(connection.name()));
QLoggingCategory logCat(logs().categoryName());
qCDebug(logCat, "Connected to %s", qPrintable(connection.name()));
Copy link
Contributor

Choose a reason for hiding this comment

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

The repeated recreation of the logging category looks suspicious and I'm not either following what's the deprecated api to avoid. The qCDebug etc are the newer logging api and what I can tell it looks the same on Qt6 too.

Copy link
Author

Choose a reason for hiding this comment

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

The repeated recreation of the logging category looks suspicious and I'm not either following what's the deprecated api to avoid. The qCDebug etc are the newer logging api and what I can tell it looks the same on Qt6 too.

@Tofee any thoughts here?

Copy link
Author

Choose a reason for hiding this comment

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

The repeated recreation of the logging category looks suspicious and I'm not either following what's the deprecated api to avoid. The qCDebug etc are the newer logging api and what I can tell it looks the same on Qt6 too.

I can clean the recreation up a bit. The error we're seeing with Qt6 is the following

| ../../../git/src/nemo-dbus/connection.cpp: In member function 'QDBusMessage NemoDBus::ConnectionData::blockingCallMethod(const QString&, const QString&, const QString&, const QString&, const QVariantList&)':
| ../../../git/src/nemo-dbus/connection.cpp:158:13: error: invalid use of non-static member function 'const QLoggingCategory& NemoDBus::ConnectionData::logs()'
|   158 |     qCDebug(logs, "DBus invocation (%s %s %s.%s)",
|       |             ^~~~
| ../../../recipe-sysroot/usr/include/QtCore/qloggingcategory.h:168:57: note: in definition of macro 'QT_MESSAGE_LOGGER_COMMON'
|   168 |     for (QLoggingCategoryMacroHolder<level> qt_category(category); qt_category; qt_category.control = false) \
|       |                                                         ^~~~~~~~
| ../../../git/src/nemo-dbus/connection.cpp:158:5: note: in expansion of macro 'qCDebug'
|   158 |     qCDebug(logs, "DBus invocation (%s %s %s.%s)",
|       |     ^~~~~~~
| In file included from ../../../git/src/nemo-dbus/connection.h:38,
|                  from ../../../git/src/nemo-dbus/connection.cpp:33:
| ../../../git/src/nemo-dbus/private/connectiondata.h:115:29: note: declared here
|   115 |     const QLoggingCategory &logs()
|       |                             ^~~~
| In file included from ../../../recipe-sysroot/usr/include/QtCore/QLoggingCategory:1,
|                  from ../../../git/src/nemo-dbus/logging.h:36,
|                  from ../../../git/src/nemo-dbus/connection.cpp:36:

Copy link
Author

Choose a reason for hiding this comment

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

@pvuorela I cleaned it up a bit now, so there's a few less QLoggingCategory. Happy to squash the 2 commits together.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I would expect is that there wouldn't be a single extra logging category since there's already an instance which was used earlier. Not sure what has changed on the macro declaration to break it.

Wonder if switching from qCDebug(category, const char*, ...) to qCDebug(category) << QString("..").arg() type of usage would avoid this problem. Or then there's also QLoggingCategory::operator() which talks about usage with qCDebug().

@pvuorela
Copy link
Contributor

pvuorela commented Jul 7, 2022

No update on after my adjustment suggestion so did myself an alternative PR #9

Also the QJSEngine part here was a bit off by creating a separate engine for each argument.

@Herrie82
Copy link
Author

Herrie82 commented Jul 7, 2022

@pvuorela Thanks for picking that up, must have slipped off my radar somehow. Let me quickly build test this on Qt6.

@Herrie82
Copy link
Author

Herrie82 commented Jul 7, 2022

No update on after my adjustment suggestion so did myself an alternative PR #9

Also the QJSEngine part here was a bit off by creating a separate engine for each argument.

Looks like a better solution to me

@Herrie82 Herrie82 closed this Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants