D24046: Allow triggering sort from QML

2019-12-03 Thread Nicolas Fella 
This revision was automatically updated to reflect the committed changes.
Closed by commit R307:1e339edef781: Allow triggering sort from QML (authored by 
nicolasfella).

REPOSITORY
  R307 KPeople

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24046?vs=70833&id=70838

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

AFFECTED FILES
  src/personssortfilterproxymodel.cpp
  src/personssortfilterproxymodel.h

To: nicolasfella, apol
Cc: mpyne, jbbgameich, broulik, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D24046: Allow triggering sort from QML

2019-12-03 Thread Aleix Pol Gonzalez 
apol accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R307 KPeople

BRANCH
  fo

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

To: nicolasfella, apol
Cc: mpyne, jbbgameich, broulik, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D24046: Allow triggering sort from QML

2019-12-03 Thread Nicolas Fella 
nicolasfella updated this revision to Diff 70833.
nicolasfella added a comment.


  Override and forward

REPOSITORY
  R307 KPeople

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24046?vs=66370&id=70833

BRANCH
  fo

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

AFFECTED FILES
  src/personssortfilterproxymodel.cpp
  src/personssortfilterproxymodel.h

To: nicolasfella, apol
Cc: mpyne, jbbgameich, broulik, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D24046: Allow triggering sort from QML

2019-11-24 Thread Michael Pyne 
mpyne added inline comments.

INLINE COMMENTS

> broulik wrote in 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 :(

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.

REPOSITORY
  R307 KPeople

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

To: nicolasfella, apol
Cc: mpyne, jbbgameich, broulik, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D24046: Allow triggering sort from QML

2019-11-23 Thread Kai Uwe Broulik 
broulik added inline comments.

INLINE COMMENTS

> broulik wrote in personssortfilterproxymodel.h:53
> ... which people say is ABI-compatible

"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 :(

REPOSITORY
  R307 KPeople

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

To: nicolasfella, apol
Cc: jbbgameich, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24046: Allow triggering sort from QML

2019-11-23 Thread Kai Uwe Broulik 
broulik added inline comments.

INLINE COMMENTS

> broulik wrote in personssortfilterproxymodel.h:53
> Can you just forward the entire thing:
> 
>   Q_INVOKABLE void sort(int column, Qt::SortOrder order = Qt::AscendingOrder) 
> override;

... which people say is ABI-compatible

REPOSITORY
  R307 KPeople

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

To: nicolasfella, apol
Cc: jbbgameich, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24046: Allow triggering sort from QML

2019-11-23 Thread Kai Uwe Broulik 
broulik added inline comments.

INLINE COMMENTS

> personssortfilterproxymodel.h:53
>  bool filterAcceptsRow(int source_row, const QModelIndex &source_parent) 
> const override;
> +Q_INVOKABLE void sortNow();
>  

Can you just forward the entire thing:

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

REPOSITORY
  R307 KPeople

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

To: nicolasfella, apol
Cc: jbbgameich, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24046: Allow triggering sort from QML

2019-11-09 Thread Jonah Brüchert 
jbbgameich added a comment.


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

REPOSITORY
  R307 KPeople

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

To: nicolasfella, apol
Cc: jbbgameich, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24046: Allow triggering sort from QML

2019-10-06 Thread Nicolas Fella 
nicolasfella added a comment.


  In D24046#542147 , @jbbgameich 
wrote:
  
  > 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

REPOSITORY
  R307 KPeople

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

To: nicolasfella, apol
Cc: jbbgameich, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24046: Allow triggering sort from QML

2019-10-05 Thread Jonah Brüchert 
jbbgameich added a comment.


  why not
  
Q_INVOKABLE void sort(int column, Qt::SortOrder order = Qt::AscendingOrder) 
override {
QSortFilterProxyModel::sort(column, order);
}
  
  instead of adding a new function?

REPOSITORY
  R307 KPeople

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

To: nicolasfella, apol
Cc: jbbgameich, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D24046: Allow triggering sort from QML

2019-09-18 Thread Kai Uwe Broulik 
broulik added a comment.


  Meh, I could have sworn `sort` was `Q_INVOKABLE` :(

REPOSITORY
  R307 KPeople

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

To: nicolasfella, apol
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24046: Allow triggering sort from QML

2019-09-18 Thread Nicolas Fella 
nicolasfella edited the summary of this revision.

REPOSITORY
  R307 KPeople

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

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

D24046: Allow triggering sort from QML

2019-09-18 Thread Nicolas Fella 
nicolasfella updated this revision to Diff 66370.
nicolasfella added a comment.


  - Whitespace--

REPOSITORY
  R307 KPeople

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24046?vs=66369&id=66370

BRANCH
  sort

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

AFFECTED FILES
  src/personssortfilterproxymodel.cpp
  src/personssortfilterproxymodel.h

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

D24046: Allow triggering sort from QML

2019-09-18 Thread Nicolas Fella 
nicolasfella created this revision.
nicolasfella added a reviewer: apol.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
nicolasfella requested review of this revision.

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

REPOSITORY
  R307 KPeople

BRANCH
  sort

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

AFFECTED FILES
  src/personssortfilterproxymodel.cpp
  src/personssortfilterproxymodel.h

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