Update the ContentList QML plugin to make it more general

Authored by ahiemstra on Jan 15 2018, 9:53 PM.



This updates the ContentList plugin with a slightly more
declarative syntax. It also makes changes to accomodate using
it in different applications.

Test Plan

Tested with master of Peruse.

Diff Detail

R157 Peruse
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
ahiemstra requested review of this revision.Jan 15 2018, 9:53 PM
ahiemstra created this revision.
leinir requested changes to this revision.Jan 16 2018, 11:28 AM

Nice work on the new API, very much better than the sort of slap-dash version in the original. Unfortunately, as the inline comments suggest, needs a bit of work ;)


This line crashes for me, when attempting to read the __location property (also, a quick check and it looks to be crashing on the very first result):
Thread 1 (Thread 0x7ffff7f9a8c0 (LWP 27790)):

#0  0x000000000130c310 in ?? ()
#1  0x00007ffff4bdd023 in QObject::property(char const*) const () from /usr/lib64/libQt5Core.so.5
#2  0x00007fffd23d4365 in BalooContentLister::queryResult (this=0xb0e030, query=0x231eec0, file=...) at /home/leinir/projects/comicreader/peruse/src/contentlist/BalooContentLister.cpp:129
#3  0x00007fffd23d5f60 in QtPrivate::FunctorCall<QtPrivate::IndexesList<0, 1>, QtPrivate::List<Baloo::QueryRunnable*, QString const&>, void, void (BalooContentLister::*)(Baloo::QueryRunnable*, QString)>::call (f=
    (void (BalooContentLister::*)(BalooContentLister * const, Baloo::QueryRunnable *, QString)) 0x7fffd23d430e <BalooContentLister::queryResult(Baloo::QueryRunnable*, QString)>, o=0xb0e030, arg=0x7fffbc0069c0)
    at /usr/include/qt5/QtCore/qobjectdefs_impl.h:136
#4  0x00007fffd23d5dde in QtPrivate::FunctionPointer<void (BalooContentLister::*)(Baloo::QueryRunnable*, QString)>::call<QtPrivate::List<Baloo::QueryRunnable*, QString const&>, void> (f=
    (void (BalooContentLister::*)(BalooContentLister * const, Baloo::QueryRunnable *, QString)) 0x7fffd23d430e <BalooContentLister::queryResult(Baloo::QueryRunnable*, QString)>, o=0xb0e030, arg=0x7fffbc0069c0)
    at /usr/include/qt5/QtCore/qobjectdefs_impl.h:169
#5  0x00007fffd23d5a4f in QtPrivate::QSlotObject<void (BalooContentLister::*)(Baloo::QueryRunnable*, QString), QtPrivate::List<Baloo::QueryRunnable*, QString const&>, void>::impl (which=1, this_=0x2382370, r=0xb0e030, a=0x7fffbc0069c0, 
    ret=0x0) at /usr/include/qt5/QtCore/qobjectdefs_impl.h:398
#6  0x00007ffff4bdf7a2 in QObject::event(QEvent*) () from /usr/lib64/libQt5Core.so.5
#7  0x00007ffff5927e6c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib64/libQt5Widgets.so.5
#8  0x00007ffff592f164 in QApplication::notify(QObject*, QEvent*) () from /usr/lib64/libQt5Widgets.so.5
#9  0x00007ffff4bb0c98 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /usr/lib64/libQt5Core.so.5
#10 0x00007ffff4bb3675 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /usr/lib64/libQt5Core.so.5
#11 0x00007ffff4c08ee3 in postEventSourceDispatch(_GSource*, int (*)(void*), void*) () from /usr/lib64/libQt5Core.so.5
#12 0x00007ffff0e80f97 in g_main_context_dispatch () from /usr/lib64/libglib-2.0.so.0
#13 0x00007ffff0e811d0 in ?? () from /usr/lib64/libglib-2.0.so.0
#14 0x00007ffff0e8125c in g_main_context_iteration () from /usr/lib64/libglib-2.0.so.0
#15 0x00007ffff4c0855f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/libQt5Core.so.5
#16 0x00007fffe9138f61 in ?? () from /usr/lib64/libQt5XcbQpa.so.5
#17 0x00007ffff4baf4aa in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/libQt5Core.so.5
#18 0x00007ffff4bb7fe4 in QCoreApplication::exec() () from /usr/lib64/libQt5Core.so.5
#19 0x0000000000403758 in main (argc=1, argv=0x7fffffffdbb8) at /home/leinir/projects/comicreader/peruse/src/app/main.cpp:146

Are you sure?


This breaks the consumption end for Peruse, so even if files are found for me (with baloo turned off), it fails to add the files properly, as it doesn't read filePath. It likely is a bug in the original use anyway, but this still breaks it, so it needs changing there as well :)


This was deliberate, please put back the original logic. Not too keen on returning in the middle of functions unless it's unavoidable within reason.


surely this would be be if(d->autosearch) { automatically start search } ;)

This revision now requires changes to proceed.Jan 16 2018, 11:28 AM
ahiemstra updated this revision to Diff 25556.Jan 17 2018, 7:20 PM
ahiemstra marked 2 inline comments as done.
  • Change ContentList::data back to single return point
  • Fix FileSystem searcher when locations is not set
  • Properly perform mime-type filtering in Baloo searcher
  • Use the right role from ContentList for retrieving the file path
  • Simplify if statement in ContentList::componentComplete
ahiemstra marked 2 inline comments as done.Jan 17 2018, 7:22 PM
ahiemstra added inline comments.

It seems Baloo passes an invalid runnable. :( Fixed now by using a lambda as discussed on IRC. :)


Qt5::Gui is needed for QImageReader::supportedFormats(), which is used in ContentQuery.cpp line 151. I could use a hardcoded list for images as well, but I was hoping to have a bit more flexible approach for mapping "query type" to mimetype.

I also originally assumed Qt5Qml was already linking to Qt5Gui but that seems to be false. :)


Woops, you are right. Fixed the use in BookListModel. Are there any other places where ContentList is used directly? As far as I could tell, everything else uses BookListModel.


Ah, I generally prefer early return instead. But fair enough, changed it back.


Hah, good point. I think I originally had some more code following this if, but now it no longer makes sense indeed. :)

leinir accepted this revision.Jan 18 2018, 10:21 AM

Right! It looks like this does the trick! Push away :D


Silly Baloo... Oh well, this is nice and clean anyway, so that works! :) Yay for lamdas ;)


Right, i guess that's ok, yeah - certainly better than a random bit of hardcoding and such :)


Thank you! Nope, i deliberately kept it centralised, so there's only the only place that connects to the ContentList :)

This revision is now accepted and ready to land.Jan 18 2018, 10:21 AM
This revision was automatically updated to reflect the committed changes.
ahiemstra marked 3 inline comments as done.