D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:198e589d5df5: Unbreak the 
KNSQuick::Engine::changedEntries functionality (authored by leinir).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26543?vs=73698&id=73699

REVISION DETAIL
  https://phabricator.kde.org/D26543

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/entrywrapper.cpp
  src/core/entrywrapper.h
  src/qtquick/qml/Button.qml
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h

To: leinir, #frameworks, #plasma, ngraham, broulik, mart, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 73698.
leinir added a comment.


  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

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26543?vs=73680&id=73698

BRANCH
  unbreak-changedentries-in-qtquick (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26543

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/entrywrapper.cpp
  src/core/entrywrapper.h
  src/qtquick/qml/Button.qml
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h

To: leinir, #frameworks, #plasma, ngraham, broulik, mart, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[Dan Leinir Turthra Jensen]

leinir added a comment.


  In D26543#595374 , @davidedmundson 
wrote:
  
  > 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.

REPOSITORY
  R304 KNewStuff

BRANCH
  unbreak-changedentries-in-qtquick (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26543

To: leinir, #frameworks, #plasma, ngraham, broulik, mart, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added a comment.
This revision is now accepted and ready to land.


  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.

REPOSITORY
  R304 KNewStuff

BRANCH
  unbreak-changedentries-in-qtquick (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26543

To: leinir, #frameworks, #plasma, ngraham, broulik, mart, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[Dan Leinir Turthra Jensen]

leinir added a comment.


  In D26543#595368 , @davidedmundson 
wrote:
  
  > 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.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D26543

To: leinir, #frameworks, #plasma, ngraham, broulik, mart
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[David Edmundson]

davidedmundson added a comment.


  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.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D26543

To: leinir, #frameworks, #plasma, ngraham, broulik, mart
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[Dan Leinir Turthra Jensen]

leinir added a comment.


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

INLINE COMMENTS

> davidedmundson wrote in quickengine.h:54
> Why QObject here?
> 
> One of the main advantages of using QQmlListProperty over QList is 
> that you can specify the derived type.

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

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D26543

To: leinir, #frameworks, #plasma, ngraham, broulik, mart
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 73680.
leinir marked an inline comment as done.
leinir added a comment.


  - Actually use the correct type for the list property

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26543?vs=73408&id=73680

BRANCH
  unbreak-changedentries-in-qtquick (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26543

AFFECTED FILES
  CMakeLists.txt
  src/core/CMakeLists.txt
  src/core/entrywrapper.cpp
  src/core/entrywrapper.h
  src/qtquick/qml/Button.qml
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h

To: leinir, #frameworks, #plasma, ngraham, broulik, mart
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> quickengine.h:54
>  Q_PROPERTY(QString searchTerm READ searchTerm WRITE setSearchTerm RESET 
> resetSearchTerm NOTIFY searchTermChanged)
> -Q_PROPERTY(KNSCore::EntryInternal::List changedEntries READ 
> changedEntries RESET resetChangedEntries NOTIFY changedEntriesChanged)
> +Q_PROPERTY(QQmlListProperty changedEntries READ changedEntries 
> NOTIFY changedEntriesChanged)
>  Q_PROPERTY(int changedEntriesCount READ changedEntriesCount NOTIFY 
> changedEntriesChanged)

Why QObject here?

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

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D26543

To: leinir, #frameworks, #plasma, ngraham, broulik, mart
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 73408.
leinir added a comment.


  - As 5.66 was released, update @since to 5.67
  - CMakeLists version requirement fun (for easier testing)

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26543?vs=73187&id=73408

BRANCH
  unbreak-changedentries-in-qtquick (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26543

AFFECTED FILES
  CMakeLists.txt
  src/core/CMakeLists.txt
  src/core/entrywrapper.cpp
  src/core/entrywrapper.h
  src/qtquick/qml/Button.qml
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h

To: leinir, #frameworks, #plasma, ngraham, broulik, mart
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 73187.
leinir added a comment.


  - Actually update the entry when it's updated, don't just ignore it

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26543?vs=73126&id=73187

BRANCH
  unbreak-changedentries-in-qtquick (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26543

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/entrywrapper.cpp
  src/core/entrywrapper.h
  src/qtquick/qml/Button.qml
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h

To: leinir, #frameworks, #plasma, ngraham, broulik, mart
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[Dan Leinir Turthra Jensen]

leinir added a dependent revision: D26544: [WIP] Switch the Plasma Desktop KCMs 
to using KNewStuffQuick.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D26543

To: leinir, #frameworks, #plasma, ngraham, broulik, mart
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[Dan Leinir Turthra Jensen]

leinir added reviewers: Frameworks, Plasma, ngraham, broulik, mart.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D26543

To: leinir, #frameworks, #plasma, ngraham, broulik, mart
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26543: Unbreak the KNSQuick::Engine::changedEntries functionality

[Dan Leinir Turthra Jensen]

leinir created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
leinir requested review of this revision.

REVISION 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

REPOSITORY
  R304 KNewStuff

BRANCH
  unbreak-changedentries-in-qtquick (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26543

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/entrywrapper.cpp
  src/core/entrywrapper.h
  src/qtquick/qml/Button.qml
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h

To: leinir
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns