Add KColumnHeadersProxyModel
ClosedPublic

Authored by ahiemstra on Nov 14 2019, 6:56 PM.

Details

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.

Diff Detail

Repository
R275 KItemModels
Branch
columnheadersmodel
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19208
Build 19226: arc lint + arc unit
ahiemstra created this revision.Nov 14 2019, 6:56 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 14 2019, 6:56 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ahiemstra requested review of this revision.Nov 14 2019, 6:56 PM
ahiemstra updated this revision to Diff 69765.Nov 14 2019, 6:57 PM
  • Add @since for the class

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.

src/core/kcolumnheadersproxymodel.h
30 ↗(On Diff #69765)

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"

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.

ahiemstra updated this revision to Diff 69806.Nov 15 2019, 2:43 PM
  • Forward column change singals as row change signals
  • Forward header data change signals as dataChanged
  • Add a unit test for KColumnHeadersProxyModel
ahiemstra updated this revision to Diff 69807.Nov 15 2019, 2:46 PM
  • Add a unit test for KColumnHeadersProxyModel
ahiemstra marked an inline comment as done.Nov 15 2019, 2:54 PM

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.

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.

ahiemstra updated this revision to Diff 70359.Nov 26 2019, 5:06 PM
  • Change to using QAbstractListModel as base
kossebau added inline comments.
src/core/kcolumnheadersmodel.cpp
23

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.

ahiemstra updated this revision to Diff 70405.Nov 27 2019, 11:04 AM
  • Do not use nested Private class

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

src/core/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.

60

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

ahiemstra updated this revision to Diff 70437.Nov 27 2019, 4:18 PM
  • Use separate Q_SIGNALS for consistency
ahiemstra marked an inline comment as done.Nov 27 2019, 4:23 PM
ahiemstra added inline comments.
src/core/kcolumnheadersmodel.h
57

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.

60

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

ahiemstra marked an inline comment as done.Dec 2 2019, 11:07 AM

@vkrause Is the updated version more what you expected?

@vkrause Is the updated version more what you expected?

Yep, thanks, this addresses my concerns. So +1 from me on this.

davidedmundson added inline comments.Dec 2 2019, 3:10 PM
src/core/kcolumnheadersmodel.cpp
84

connect(modelAboutToBeReset
modelReset

ahiemstra updated this revision to Diff 70948.Dec 5 2019, 11:21 AM
  • Add missing layoutChanged/reset signals
ahiemstra marked an inline comment as done.Dec 5 2019, 11:22 AM
davidedmundson accepted this revision.Dec 5 2019, 11:49 AM

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

This revision is now accepted and ready to land.Dec 5 2019, 11:49 AM

Sure, I can land it after saturday.

This revision was automatically updated to reflect the committed changes.