Allow triggering sort from QML
ClosedPublic

Authored by nicolasfella on Sep 18 2019, 11:06 AM.

Details

Summary

I was missing a way to trigger the sorting of my contacts list from QML

See https://invent.kde.org/kde/plasma-phonebook/merge_requests/17/ for context

Diff Detail

Repository
R307 KPeople
Branch
fo
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19442
Build 19460: arc lint + arc unit
nicolasfella created this revision.Sep 18 2019, 11:06 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 18 2019, 11:06 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
nicolasfella requested review of this revision.Sep 18 2019, 11:06 AM
nicolasfella updated this revision to Diff 66370.
  • Whitespace--
nicolasfella edited the summary of this revision. (Show Details)Sep 18 2019, 11:09 AM

Meh, I could have sworn sort was Q_INVOKABLE :(

jbbgameich added a subscriber: jbbgameich.EditedOct 5 2019, 8:13 AM

why not

Q_INVOKABLE void sort(int column, Qt::SortOrder order = Qt::AscendingOrder) override {
    QSortFilterProxyModel::sort(column, order);
}

instead of adding a new function?

why not

Q_INVOKABLE void sort(int column, Qt::SortOrder order = Qt::AscendingOrder) override {
    QSortFilterProxyModel::sort(column, order);
}

instead of adding a new function?

mostly to get rid of the argument

Can we get one of the two ways accepted so we can progress on the phonebook merge request?

broulik added inline comments.Nov 23 2019, 8:33 AM
src/personssortfilterproxymodel.h
53

Can you just forward the entire thing:

Q_INVOKABLE void sort(int column, Qt::SortOrder order = Qt::AscendingOrder) override;
53

... which people say is ABI-compatible

broulik added inline comments.Nov 23 2019, 8:35 AM
src/personssortfilterproxymodel.h
53

"You can ... reimplement virtual functions defined in the primary base class hierarchy (that is, virtuals defined in the first non-virtual base class, or in that class's first non-virtual base class, and so forth) if it is safe that programs linked with the prior version of the library call the implementation in the base class rather than the derived one. This is tricky and might be dangerous. Think twice before doing it. Alternatively see below for a workaround."
Now I'm confused :(

mpyne added a subscriber: mpyne.Nov 24 2019, 7:07 PM
mpyne added inline comments.
src/personssortfilterproxymodel.h
53

Approximately no one uses C++ virtual classes (which are a separate feature from virtual methods which we are used to).

Basically, reimplementing a virtual function that's already defined in a base class can be binary compatible for already-compiled programs because it doesn't change the size or calling convention of the virtual table: there was already a slot in the virtual table to call the base class's implementation of a virtual function, and we replaced the pointer in the virtual table to instead call the newly-implemented virtual function in the derived class.

The catch is that already-compiled code is not guaranteed to call the derived class implementation (due to inlining or other optimizations, for instance) so you cannot absolutely rely on your new implementation being called.

If you have to think too hard about whether it's safe or not, it's best to assume it's not.

In this case we're literally just calling the original implementation anyways, and Q_INVOKABLE is just to register it to the meta-object system, so I think this should be fine. But if we want to use a separate method I'd label it as "KF6 TODO" or something similar so that we can go back and fix it up.

Override and forward

apol accepted this revision.Dec 3 2019, 6:07 PM
This revision is now accepted and ready to land.Dec 3 2019, 6:07 PM
This revision was automatically updated to reflect the committed changes.