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

BugFix: Compilation Failure for Qt6 Due to Deprecated Members #1104

Merged
merged 1 commit into from May 7, 2024

Conversation

ShaopengLin
Copy link
Collaborator

@ShaopengLin ShaopengLin commented May 5, 2024

Fixes #1102

Tested on Qt 6.2.4.

src/contentmanagermodel.cpp:40:16: error: ‘class QVariant’ has no member named ‘type’; did you mean ‘typeId’?
   39 |         if ( r.type() == QVariant::ByteArray )
      |                ^~~~
      |                typeId
src/contentmanagermodel.cpp:40:36: error: ‘ByteArray’ is not a member of ‘QVariant’
   39 |         if ( r.type() == QVariant::ByteArray )
      |

QVariant.type() and QVariant::ByteArray has been deprecated. We will use the new function QVariant.typeId() and QMetaType::QByteArray from Qt6.

I recommend we have compilation pipelines to check both Qt versions.

Fix:

  • Added macro to make the application still compatible with Qt 5
  • Used equivalent new features from Qt 6 to replace the lines.

@kelson42
Copy link
Collaborator

kelson42 commented May 6, 2024

I recommend we have compilation pipelines to check both Qt versions.

Yes, see #1103

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

Maybe your change is the best way to fix the current problem. However I suspect that you don't know about src/portutils.h. See if you can come up with a convenient and future-proof utility that can be used for portably checking the type of a QVariant. If not, we will merge the PR as is.

Comment on lines 39 to 45
#if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)
if ( r.type() == QVariant::ByteArray )
return r;
#else
if ( r.typeId() == QMetaType::QByteArray )
return r;
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

On a second thought, in case there is no good way to move the conditional compilation into portutils.h, I would like to restrict the conditional compilation to the least amount of code as follows:

        const bool gotThumbnailData =
#if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)
            r.type() == QVariant::ByteArray;
#else
            r.typeId() == QMetaType::QByteArray;
#endif

        if ( gotThumbnailData )
            return r;

Above can be turned into a more readable form if we introduce a special macro that will select during compile time the right expression depending on the version of Qt used:

        const bool gotThumbnailData = QT5VS6(r.type() == QVariant::ByteArray,
                                             r.typeId() == QMetaType::QByteArray);

        if ( gotThumbnailData )
            return r;

Comment on lines 144 to 150
#if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)
if ( faviconEntry.type() == QVariant::ByteArray )
return faviconEntry;
#else
if ( faviconEntry.typeId() == QMetaType::QByteArray )
return faviconEntry;
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

In case you wonder what the proposed change improves, it minimizes the amount of code duplication between the conditionally compiled pieces and hence the risk that, for whatever reasons, one of the branches is updated and the other is not.

@ShaopengLin
Copy link
Collaborator Author

@veloman-yunkan I found two ways for type checking for both Qt5 and 6:

  • r.typeName() == QMetaType(QMetaType::QByteArray).name()
  • r.userType() == QMetaType::QByteArray

❌ The typeName approach isn't something we want as string comparison is unnecessary overhead when we have ints.

✅ The userType approach works well as it is:

  • Same as type() for non-user defined type in Qt5
  • Exact same behavior as typeId() in Qt6
  • The change commit also recommends using userType instead, which I will use in this commit.

I imagine in the future, they might decide to remove either typeId() or userType(). Since its 50/50 we can just use userType.

Replaced deprecated member type() and QVariant::Type to compatible members in both Qt5&6
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

Perfect!

@kelson42 kelson42 merged commit 851ccc5 into kiwix:main May 7, 2024
4 checks passed
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.

Unable to compile anymore with Qt6
3 participants