Skip to content

Follow-up fix for Offline::getResults() #60

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Follow-up fix for Offline::getResults() #60

wants to merge 2 commits into from

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
Collaborator

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 •
edited
Loading

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?

aleixpol
aleixpol reviewed Aug 17, 2025
View reviewed changes
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?

@arrowd
Make Offline::getResults return a wrapper object
e64723a 
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
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aleixpol aleixpol aleixpol left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arrowd @ximion @aleixpol