On jun 23, 2015, 12:26 p.m., David Faure wrote:
> > Regarding the style. AStyle time i think ;)
> > There is lots of function( arg ) instead of function(arg). ?besides that,
> > some places could use more line breaks for readability (marked above).
> >
> > Some API questions.
> > setSourceColumns seems to be used for both rearranging and filtering with
> > no way to change individual columns (that i see). Eg, to toogle them or
> > show/hide columns on demand.
> > Would it make sense to add the following API functions as well?
> >
> > // toggle the visibility of a column
> > void toggleColumnVisibility(int column)
> > {
> > // pseudo code
> > setColumnVisible(column, !isColumnVisible(column));
> > }
> >
> > // set the column visible or hidden, depending on the bool.
> > void setColumnVisible(int column, bool visible)
> >
> > // returns if a column is visible
> > bool isColumnVisible(int column)
> >
> > Adding the above functionality would make it much more convenient to use in
> > cases where you want to hide/show a column on the fly.
>
> David Faure wrote:
> Thanks for the reminder about astyle-kdelibs, forgot to do that. Done now.
>
> Toggle seems overkill (even QHeaderView doesn't have that), but the other
> two make sense. I'll add that (with QHeaderView naming).
>
> David Faure wrote:
> Hmm, it gets a bit hairy.
>
> If I do setSourceColumns(QVector<int>() << 2 << 1) and then
> setColumnHidden(2, true), I assume I'm hiding source-column-2, so the
> underlying vector becomes [1].
> But if I now do setColumnHidden(2, false), where should column 2 reappear?
> For the position to be kept, we would need (like QHeaderView) to
> distinguish reordering (visual<->logical mapping) and hiding (happens on top).
> This makes the implementation more complex and error-prone IMHO. I think
> I'd rather let the program decide how it wants to handle that "vector of
> source columns". Maybe it's statically known (like in the first app I wrote
> this for), or it's replicating what a QHeaderView does at report-generation
> time (like in the app I'm writing this for right now, see FatCRM on github)
> (*), and only in a possible third case there would be a need for separating
> reordering and hiding...
>
> (*) this reminds me, I wanted to add docu about how to replicate what
> QHeaderView does, using this proxy, but maybe this is only useful for use
> with KDReports...
>
> Mark Gaiser wrote:
> It's not hairy, but i see why you think it is. You're assuming that
> setColumnHidden hides the _source_ column. That's not how i read your API.
>
> You have setSourceColumns(QVector<int>() << 2 << 1).
> Then you have setColumnHidden(2, true)* which in my view hides the second
> _proxy_ column, not the source column. If it hides the source column the name
> should reflect that (eg setSourceColumnHidden).
>
> Think of it this way, if you use the Qt class QSortFilterProxyModel and
> you call index(row, col) on that class, you get the QModelIndex from the
> _proxy_, not the _source_, right? Or i'm very wrong and need to refresh my Qt
> Model knowledge when a proxy is involved ;-)
>
> David Faure wrote:
> That doesn't remove the complexity, since the problem of "what should
> setColumnHidden(2, false) do afterwards?" is still there, if the underlying
> implementation is a single QVector<int>. And if it's not, then the
> implementation needs to skip hidden columns, which becomes more complex.
>
> You're right about index(row, col), but this isn't what this is about.
> Look at QHeaderView::hideSection, it works on logical indexes, not visual
> indexes.
> Imagine a table with columns like "first name, last name, age, company".
> Then you want to programmatically use that table for a case where age doesn't
> matter, you do hideSection(2). This should work, even if the user reordered
> the columns. This is why the API works on logical indexes, they are the only
> thing that makes sense to the caller of the API.
>
> Of course in the proxy here there's no such user/programmer distinction,
> but in order to have some consistency I still think that *if* there should be
> any separate API for hiding and reordering, it should probably work like
> QHeaderView does.
>
> But at this point I think extra API isn't necessary.
Thank you for the detailed explenation. I see where you would have that
complexity issue.
- Mark
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124156/#review81702
-----------------------------------------------------------
On jun 23, 2015, 9:33 p.m., David Faure wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124156/
> -----------------------------------------------------------
>
> (Updated jun 23, 2015, 9:33 p.m.)
>
>
> Review request for KDE Frameworks.
>
>
> Repository: kitemmodels
>
>
> Description
> -------
>
> It supports reordering and hiding columns from the source model.
>
>
> Diffs
> -----
>
> autotests/CMakeLists.txt 4e604eeb6bd64529ec1fba983e3f58e2f8d0bd2c
> autotests/krearrangecolumnsproxymodeltest.cpp PRE-CREATION
> autotests/test_model_helpers.h PRE-CREATION
> src/CMakeLists.txt dc7d5f04c650f8d21784f16f8d7676e5c74c93e1
> src/krearrangecolumnsproxymodel.h PRE-CREATION
> src/krearrangecolumnsproxymodel.cpp PRE-CREATION
>
> Diff: https://git.reviewboard.kde.org/r/124156/diff/
>
>
> Testing
> -------
>
> Wrote it for a customer who allowed me to keep copyright and publish it under
> the LGPL.
>
> Unit-tested, and used in two real projects.
>
>
> Thanks,
>
> David Faure
>
>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
