Skip to content

Conversation

@arrowd
Copy link
Contributor

@arrowd arrowd commented Aug 16, 2025

This is, IMO, a better fix for #59

  • It handles both Transaction::Role and Transaction::Error enums
  • It does not require registering the metatype
  • It is cleaner from the user perspective as it is now possible to call named getters rather than reply.argumentAt(n)
  • It actually works, while offline: Ensure the enum is registered #59 didn't fix the problem for me.

@ximion
Copy link
Member

ximion commented Aug 16, 2025

It is also an API break though...

@ximion ximion requested a review from aleixpol August 16, 2025 08:24
@arrowd
Copy link
Contributor Author

arrowd commented Aug 16, 2025

Yes, this is unfortunate. But v1.1.3 is out for a short time (and it was only tagged, no entry in Releases section) and I imagine that the largest PackageKit-Qt consumer is KDE itself, so maybe make an exception?

Copy link
Collaborator

@aleixpol aleixpol left a comment

Choose a reason for hiding this comment

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

Is this tested to work?

This is required to transform qulongulong's into Transaction::Role and
Transaction::Error enums. Using these enums directly in QDBusPendingReply<>
requires registering them with qDBusRegisterMetaType().
@aleixpol
Copy link
Collaborator

It is also an API break though...

How worried are we about it?

@ximion
Copy link
Member

ximion commented Aug 19, 2025

How worried are we about it?

If this goes in, it will have to be a SONAME bump - yes, it was only out for one release for a new feature, but you never know what else is using it, and people trust me to not randomly break their code with API incompatibilities that aren't communicated.

Fortunately, I added version-check macros to QPK a while back, so supporting multiple versions on the same codebase will be rather trivial.

@aleixpol
Copy link
Collaborator

Okay, then let's include the SONAME bump here?

@ximion
Copy link
Member

ximion commented Aug 25, 2025

Okay, then let's include the SONAME bump here?

I could do that later and use that opportunity to drop the Qt5 version of the library - unless there are objections to going Qt6-only?

@aleixpol
Copy link
Collaborator

I don't have a need for a Qt 5 version.

FWIW, this issue has reached our users already, so it needs landing & releasing soon.

@ximion ximion merged commit 47ad32f into PackageKit:main Sep 12, 2025
2 checks passed
@ximion
Copy link
Member

ximion commented Sep 12, 2025

Let's merge it. I do plan on making a new PK release soon (this month, depending on a few bugfixes and organizational changes that need to land first), releasing PK and QPK at the same time would make sense to me.

@aleixpol
Copy link
Collaborator

Ok, what version are you going to use? I'll add the needed ifdefs in Discover.

@ximion
Copy link
Member

ximion commented Sep 12, 2025

1.1.4, most likely. You should be able to use the QPK_CHECK_VERSION macro for this :-)

@aleixpol
Copy link
Collaborator

@arrowd arrowd deleted the getresults-fix branch October 11, 2025 19:38
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.

3 participants