-
Notifications
You must be signed in to change notification settings - Fork 20
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.
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
Conversation
This reverts commit 8a35105.
|
It is also an API break though... |
|
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
left a comment
There was a problem hiding this 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().
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. |
|
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? |
|
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. |
|
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. |
|
Ok, what version are you going to use? I'll add the needed ifdefs in Discover. |
|
1.1.4, most likely. You should be able to use the |
This is, IMO, a better fix for #59
Transaction::RoleandTransaction::Errorenumsreply.argumentAt(n)