Unbreak the KNSQuick::Engine::changedEntries functionality
ClosedPublic

Authored by leinir on Thu, Jan 9, 1:41 PM.

Details

Summary

This patch changes the previous naive approach to a more elaborate,
QQmlListProperty based one. It further introduces a QObject based
wrapper EntryInternal, in preparation for refactoring for KF6.

The rationale for this approach (rather than e.g. using a model)
is that the approach makes for the simplest possible porting from
the QWidget based methods.

Add a QObject wrapper for EntryInternal

We can't reasonably change EntryInternal to a QObject at the moment,
as that would make the assumptions about how it's used throughout
the codebase not quite correct, so until breakage is allowed, add
a class which wraps up an object and which works more like one would
expect a QObject to behave.

Register the wrapper with QtQuick (and clean the plugin a tiny bit)

Switch to a QQmlListProperty

The issue with the previous approach is that KNSCore::EntryInternal
is not a QObject, and so the list can't be used verbatim. This
allows us to do things a little bit roundabout, but also reasonably
simply.

Using an alias turns out to be fragile when digging through layers

Previously, the property would fail to resolve on the first try,
and consequently would just stop attempting the resolution entirely.
This binding, while suboptimal compared to aliasing, at least
forwards the data correctly.

Test Plan

Without this patch, changedEntries cannot be read by any user of the code
With it, changedEntries can be passed through to any C++ consumer, and read out using the standard QQmlListReference method

Diff Detail

Repository
R304 KNewStuff
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
leinir created this revision.Thu, Jan 9, 1:41 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptThu, Jan 9, 1:41 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
leinir requested review of this revision.Thu, Jan 9, 1:41 PM
leinir updated this revision to Diff 73187.Fri, Jan 10, 12:08 PM
  • Actually update the entry when it's updated, don't just ignore it
leinir updated this revision to Diff 73408.Mon, Jan 13, 1:33 PM
  • As 5.66 was released, update @since to 5.67
  • CMakeLists version requirement fun (for easier testing)
davidedmundson added inline comments.
src/qtquick/quickengine.h
56

Why QObject here?

One of the main advantages of using QQmlListProperty over QList<QObject*> is that you can specify the derived type.

leinir updated this revision to Diff 73680.Thu, Jan 16, 9:15 AM
leinir marked an inline comment as done.
  • Actually use the correct type for the list property

Thanks to David for making me look at that again, the original choice was based on a false-positive test

src/qtquick/quickengine.h
56

Quite simply because i, in my test code, had something around the wrong way... Updated patch incoming.

I've gone through this again, and I'm somewhat confused.
Entry (via EntryWrapper) doesn't seem usable by QML. It doesn't have any properties.

If it's just proxying through QML to other C++, then QVariant should be fine, no need for a custom box type no need for QQmlListProperty.
A QVariantList would cover everything.

I've gone through this again, and I'm somewhat confused.
Entry (via EntryWrapper) doesn't seem usable by QML. It doesn't have any properties.

If it's just proxying through QML to other C++, then QVariant should be fine, no need for a custom box type no need for QQmlListProperty.
A QVariantList would cover everything.

It does not currently, but the intention is to expand upon it and make it usable from QML. In the spirit of not lumping everything in and making this yet another huge, months long pile of work that nobody has time to review, i thought it reasonable to perhaps get this to happen first, and then expand upon it as needed by KNSQuick.

davidedmundson accepted this revision.Thu, Jan 16, 1:23 PM

Given timeframes and where we've ended up. Accepted.

I would like to see some KF6 workboard tasks for the future.
QVariantList + QGadget should work, though that does require recent Qt.

This revision is now accepted and ready to land.Thu, Jan 16, 1:23 PM

Given timeframes and where we've ended up. Accepted.

Thanks! :)

I would like to see some KF6 workboard tasks for the future.

That would be a good idea, yup. Most of the things that need doing in KNewStuff are marked as TODO KF6 throughout the codebase, but an overview with some more... conceptual goals would be good, so it's more clear precisely what the basic idea is for the future of the framework.

leinir updated this revision to Diff 73698.Thu, Jan 16, 1:48 PM

Some housekeeping (rebase on master)

  • Actually update the entry when it's updated, don't just ignore it
  • As 5.66 was released, update @since to 5.67
  • Actually use the correct type for the list property
This revision was automatically updated to reflect the committed changes.