D25311: Add KColumnHeadersProxyModel

2019-12-10 Thread Arjen Hiemstra 
This revision was automatically updated to reflect the committed changes.
Closed by commit R275:241e9dec6896: Add KColumnHeadersProxyModel (authored by 
ahiemstra).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D25311?vs=70948&id=71204#toc

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=70948&id=71204

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersmodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersmodel.cpp
  src/core/kcolumnheadersmodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

To: ahiemstra, davidedmundson
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-12-05 Thread Arjen Hiemstra 
ahiemstra added a comment.


  Sure, I can land it after saturday.

REPOSITORY
  R275 KItemModels

BRANCH
  columnheadersmodel

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

To: ahiemstra, davidedmundson
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-12-05 Thread David Edmundson 
davidedmundson accepted this revision.
davidedmundson added a comment.
This revision is now accepted and ready to land.


  Though maybe we should wait till after tagging (Saturday) ?

REPOSITORY
  R275 KItemModels

BRANCH
  columnheadersmodel

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

To: ahiemstra, davidedmundson
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-12-05 Thread Arjen Hiemstra 
ahiemstra marked an inline comment as done.

REPOSITORY
  R275 KItemModels

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

To: ahiemstra
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-12-05 Thread Arjen Hiemstra 
ahiemstra updated this revision to Diff 70948.
ahiemstra added a comment.


  - Add missing layoutChanged/reset signals

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=70437&id=70948

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersmodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersmodel.cpp
  src/core/kcolumnheadersmodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

To: ahiemstra
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-12-02 Thread David Edmundson 
davidedmundson added inline comments.

INLINE COMMENTS

> kcolumnheadersmodel.cpp:83
> +
> +if (newSourceModel) {
> +connect(newSourceModel, 
> &QAbstractItemModel::columnsAboutToBeInserted, this, [this](const 
> QModelIndex&, int first, int last) {

connect(modelAboutToBeReset
modelReset

REPOSITORY
  R275 KItemModels

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

To: ahiemstra
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-12-02 Thread Volker Krause 
vkrause added a comment.


  In D25311#570700 , @ahiemstra 
wrote:
  
  > @vkrause Is the updated version more what you expected?
  
  
  Yep, thanks, this addresses my concerns. So +1 from me on this.

REPOSITORY
  R275 KItemModels

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

To: ahiemstra
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-12-02 Thread Arjen Hiemstra 
ahiemstra marked an inline comment as done.
ahiemstra added a comment.


  @vkrause Is the updated version more what you expected?

REPOSITORY
  R275 KItemModels

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

To: ahiemstra
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-11-27 Thread Arjen Hiemstra 
ahiemstra marked an inline comment as done.
ahiemstra added inline comments.

INLINE COMMENTS

> kossebau wrote in kcolumnheadersmodel.h:57
> For consistency I would prefer having a full Q_SIGNALS section:
> 
>   Q_SIGNALS:
>   void sourceModelChanged();
> 
> That matches the usual pattern and helps when reading raw headers to learn 
> about API of classes.

I actually prefer the combined style since it groups together all related bits 
regarding a single property. But I've changed it to a separate section now.

> kossebau wrote in kcolumnheadersmodel.h:60
> Why not QScopedPointer (for consistency in KF code)? Is that one going to be 
> deprecated in favour of std::unique_ptr?

Mostly force of habit. I also see little point in using QScopedPointer 
nowadays, as it does not offer anything over std::unique_ptr.

REPOSITORY
  R275 KItemModels

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

To: ahiemstra
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-11-27 Thread Arjen Hiemstra 
ahiemstra updated this revision to Diff 70437.
ahiemstra added a comment.


  - Use separate Q_SIGNALS for consistency

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=70405&id=70437

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersmodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersmodel.cpp
  src/core/kcolumnheadersmodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

To: ahiemstra
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-11-27 Thread Friedrich W. H. Kossebau 
kossebau added a comment.


  Two more nitpicks while this has my attention (sadly no time/idea to comment 
on the general usefulness) :)

INLINE COMMENTS

> kcolumnheadersmodel.h:57
> +void setSourceModel(QAbstractItemModel *newSourceModel);
> +Q_SIGNAL void sourceModelChanged();
> +

For consistency I would prefer having a full Q_SIGNALS section:

  Q_SIGNALS:
  void sourceModelChanged();

That matches the usual pattern and helps when reading raw headers to learn 
about API of classes.

> kcolumnheadersmodel.h:60
> +private:
> +const std::unique_ptr d;
> +};

Why not QScopedPointer (for consistency in KF code)? Is that one going to be 
deprecated in favour of std::unique_ptr?

REPOSITORY
  R275 KItemModels

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

To: ahiemstra
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-11-27 Thread Arjen Hiemstra 
ahiemstra updated this revision to Diff 70405.
ahiemstra added a comment.


  - Do not use nested Private class

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=70359&id=70405

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersmodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersmodel.cpp
  src/core/kcolumnheadersmodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

To: ahiemstra
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-11-26 Thread Friedrich W. H. Kossebau 
kossebau added inline comments.

INLINE COMMENTS

> kcolumnheadersmodel.cpp:22
> +
> +class KColumnHeadersModel::Private
> +{

Instead of using a nested class Private, consider using a non-nested 
KColumnHeadersModelPrivate class.
Otherwise you want to tag this class to not inherit the symbol visibility 
attributes, using Q_DECL_HIDDEN. Not going nested spares to do this.

REPOSITORY
  R275 KItemModels

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

To: ahiemstra
Cc: kossebau, vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-11-26 Thread Arjen Hiemstra 
ahiemstra updated this revision to Diff 70359.
ahiemstra added a comment.


  - Change to using QAbstractListModel as base

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=69807&id=70359

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersmodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersmodel.cpp
  src/core/kcolumnheadersmodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

To: ahiemstra
Cc: vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-11-15 Thread Volker Krause 
vkrause added a comment.


  In D25311#563011 , @ahiemstra 
wrote:
  
  > > I'm wondering if this is technically a proxy model, or rather a 
QAbstractListModel? Its content is not representing cells in the source model 
after all, which mapTo/FromSource assumes I think.
  >
  > Yeah I'm not entirely sure. If it was possible I would probably have based 
this on QTransposeProxyModel, but that is Qt 5.13+ stuff.
  
  
  I don't think that would help much either, as that assumes you are mapping 
cells to cells, not cells to header data.
  
  >> It works in the example as you implement the entire QAIM interface needed 
for it, so the mapping functions are probably not even called, until the source 
model changes, or you add a selection model on top.
  > 
  > Most of what I needed I needed to implement because QAbstractProxyModel 
does not reimplement them.
  
  Right. My point is that if you'd inherit from QAbstractListModel and drop 
parent, index, columnCount and mapTo/FromSource you should end up with the same 
result. The sourceModel property might need to be added explicitly in that case 
though.

REPOSITORY
  R275 KItemModels

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

To: ahiemstra
Cc: vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-11-15 Thread Arjen Hiemstra 
ahiemstra marked an inline comment as done.
ahiemstra added a comment.


  > We should probably have a unit test, even if it just covers the basic case.
  
  Done.
  
  > This currently assumes the source model's columns are static. At a minimum 
that needs a comment.
  
  Oops. That was not the intention. Fixed.
  
  > I'm wondering if this is technically a proxy model, or rather a 
QAbstractListModel? Its content is not representing cells in the source model 
after all, which mapTo/FromSource assumes I think.
  
  Yeah I'm not entirely sure. If it was possible I would probably have based 
this on QTransposeProxyModel, but that is Qt 5.13+ stuff.
  
  > It works in the example as you implement the entire QAIM interface needed 
for it, so the mapping functions are probably not even called, until the source 
model changes, or you add a selection model on top.
  
  Most of what I needed I needed to implement because QAbstractProxyModel does 
not reimplement them.

REPOSITORY
  R275 KItemModels

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

To: ahiemstra
Cc: vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-11-15 Thread Arjen Hiemstra 
ahiemstra updated this revision to Diff 69807.
ahiemstra added a comment.


  - Add a unit test for KColumnHeadersProxyModel

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=69806&id=69807

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersproxymodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersproxymodel.cpp
  src/core/kcolumnheadersproxymodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

To: ahiemstra
Cc: vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-11-15 Thread Arjen Hiemstra 
ahiemstra updated this revision to Diff 69806.
ahiemstra added a comment.


  - Forward column change singals as row change signals
  - Forward header data change signals as dataChanged
  - Add a unit test for KColumnHeadersProxyModel

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=69765&id=69806

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersproxymodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersproxymodel.cpp
  src/core/kcolumnheadersproxymodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

To: ahiemstra
Cc: vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-11-15 Thread Volker Krause 
vkrause added a comment.


  I'm wondering if this is technically a proxy model, or rather a 
QAbstractListModel? Its content is not representing cells in the source model 
after all, which mapTo/FromSource assumes I think. It works in the example as 
you implement the entire QAIM interface needed for it, so the mapping functions 
are probably not even called, until the source model changes, or you add a 
selection model on top.

REPOSITORY
  R275 KItemModels

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

To: ahiemstra
Cc: vkrause, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25311: Add KColumnHeadersProxyModel

2019-11-14 Thread David Edmundson 
davidedmundson added a comment.


  We should probably have a unit test, even if it just covers the basic case.
  
  This currently assumes the source model's columns are static.  At a minimum 
that needs a comment.

INLINE COMMENTS

> kcolumnheadersproxymodel.h:30
> + *
> + * This model will expose the source model's headers as a simple list. This 
> is
> + * mostly useful as a helper for QML applications that want to display a 
> model's

Worth clarifying that the list isn't across columns.

i.e
"Each header section (column) column in the source model is presented as a row 
in this proxy. Roles map directly"

REPOSITORY
  R275 KItemModels

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

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

D25311: Add KColumnHeadersProxyModel

2019-11-14 Thread Arjen Hiemstra 
ahiemstra updated this revision to Diff 69765.
ahiemstra added a comment.


  - Add @since for the class

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=69764&id=69765

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/kcolumnheadersproxymodel.cpp
  src/core/kcolumnheadersproxymodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

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

D25311: Add KColumnHeadersProxyModel

2019-11-14 Thread Arjen Hiemstra 
ahiemstra created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ahiemstra requested review of this revision.

REVISION SUMMARY
  It converts a model's column headers into a simple list model. It is
  mainly useful as a helper to create headers for QtQuick TableView but
  may also be useful for other things.

TEST PLAN
  The added test QML file shows a single 1 as described in the test.

REPOSITORY
  R275 KItemModels

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/kcolumnheadersproxymodel.cpp
  src/core/kcolumnheadersproxymodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

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