Skip to content

PackageKit::Offline::Results: Make castable to QDBusPendingCall#68

Open
arrowd wants to merge 1 commit into
PackageKit:mainfrom
arrowd:offline-results-watcher
Open

PackageKit::Offline::Results: Make castable to QDBusPendingCall#68
arrowd wants to merge 1 commit into
PackageKit:mainfrom
arrowd:offline-results-watcher

Conversation

@arrowd

@arrowd arrowd commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

This makes it possible to feed the Results object into QDBusPendingCallWatcher

This makes it possible to feed the Results object into QDBusPendingCallWatcher
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 70faae50-3e01-4bde-9771-213d92a16445

📥 Commits

Reviewing files that changed from the base of the PR and between c9651ad and 419eec6.

📒 Files selected for processing (1)
  • src/offline.h

📝 Walkthrough

Walkthrough

An implicit conversion operator to QDBusPendingCall is added to the Offline::Results nested class in src/offline.h, allowing Results instances to be used directly as QDBusPendingCall by returning the internal m_reply member.

Changes

Offline Results Conversion Operator

Layer / File(s) Summary
Conversion operator addition
src/offline.h
Added operator QDBusPendingCall() const to Offline::Results, returning m_reply.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Related PRs

  • None identified.

Suggested labels

  • None

Suggested reviewers

  • None
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and clearly describes the main change to Offline::Results.
Description check ✅ Passed The description is directly related to the change and matches the stated intent.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@aleixpol aleixpol left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do you have a usage change counter-part to review together?

Comment thread src/offline.h
Transaction::Error error() const;
QString errorDescription() const;

operator QDBusPendingCall() const { return m_reply; }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why not add a getter? Explicit is often clearer, I can see how it would seem magical in this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My take is that PackageKit::Offline::Results is a QDBusPendingCall. It wraps this single value, so making it act as plain QDBusPendingCall makes sense to me.

@arrowd

arrowd commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Do you have a usage change counter-part to review together?

Not yet, but I plan to use to together with QDBusPendingCallWatcher in Discover to fix https://invent.kde.org/plasma/discover/-/merge_requests/1354 the right way.

@aleixpol

Copy link
Copy Markdown
Collaborator

Not yet, but I plan to use to together with QDBusPendingCallWatcher in Discover to fix https://invent.kde.org/plasma/discover/-/merge_requests/1354 the right way.

I thought so and hence my asking. ::prepare is somewhat blocking right now, we might need refactoring in the interface.

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.

2 participants