[Qt-interest] QAbstractProxyModel methods index, parent, rowCount, columnCount not implemented?

[Andre Somers]

Op Wo, 18 mei, 2011 3:03 pm, schreef Stephen Kelly:
> Andre Somers wrote:
>
>>
>>>>> Great, I will port my CheckableProxyModel to use it as soon as Qt 4.8
>>>>> is out.
>>>
>>> I had a quick look at your CheckableProxyModel, and it does a bunch of
>>> things wrong, such as storing QModelIndexes instead of
>>> QPersistentModelIndexes,
>> No, it does not do that. A QHash<QPersistentModelIndex, NodeState> is
>> used
>> for storing the states. I don't know where you get that I store
>> QModelIndex directly? I don't claim there are no mistakes in the code,
>> but
>> this is not one of them.
>
> I was referring to QModelIndexList members in the class. I only had a
> quick
> look and didn't think about it very hard, but if you also store
> QPersistentModelIndexes you might be ok. I also didn't try to run it or
> test it. I don't mean to offend.

None taken.
Actually, I think you are right that there is a possible issue with the
way I deal with retreiving the selection now. I should re-think that. The
base storing is probably OK, but there is an issue in the way I
implemented the API for retrieving the lists of checked and unchecked
items. These functions are meant to be called in close succession, but I
think I should do this differently as there is no way to guarantee that
they will be used in this way. Thanks for pointing that out.

>>> and not listening to the source model for changes
>>> and keeping up to date.
>> The persistent model indexes are checked for validity before they are
>> used, but you are right that tests are needed there. It does work just
>> fine for lazily populated models (such as the QFileSystemModel) though.
>
> Yes, you'd also want to test it for things like layoutChange (during which
> anything can happen).
Yes, I think you are right. Though I think it will be *very* tricky to
deal with this particular case. I may have to resort to documentation:
"Don't use this class on item models that may change their layout", or
just do a reset of the internal data if that happens. Anyway, I *do* have
to look into this.

>>> You should probably write some
>>> tests with a changing source model and see what happens.
>> That I should do indeed. Do you happen to have unit tests that you use
>> for
>> your KCheckableProxyModel that I could use as a starting point?
>>
>
> Not really. There's some model testing stuff in kdelibs, but it's too
> strict in its current form to be useful I think.
OK. Thanks.

>>> If you store data in a proxy, beware of reset()s of the source and the
>>> proxy. QIdentityProxyModel does not give you any API to handle them
>>> properly
>>> because the trolls don't want the fix in QAIM. That means everyone has
>>> to
>>> figure out the fix and apply it to each concrete proxy model. The fix
>>> is
>>> also not documented, so good look finding it :). It's described in a
>>> unit
>>> test in one of my old merge requests though.
>> Nice. A treasure hunt based on a couple of vague hints. I know that you
>> are an expert in Qt model-view programming, and you have contributed
>> many
>> useful classes dealing with proxymodels and their impact on selection
>> behaviour, but this comment is not all that helpful. Could you be a bit
>> more specific please?
>>
>
> See 6f1384fcbeea993d5be47590c696de60215b7608 which has since been reverted
> in favour of applying a patch in QSFPM, which means all concrete proxies
> need something similar (and maybe even QSFPM subclasses still. I didn't
> investigate). The comments in the unit explain all I know about the issue.
> You have to do connect(sourceModel, SIGNAL(modelReset()),
> SLOT(resetInternalData())); at the correct moment. You also have to
> disconnect at the correct moment, which is not in the unit test, and which
> I
> also didn't investigate further. I also didn't attempt to document the
> workaround because I can't think of a way to describe it clearly and
> sensibly and I didn't investigate it fully (like the disconnect). I only
> tested that the existing code could break and worked on that.
>
> Anyway, sorry for being vague. It's probably just residual annoyance that
> I
> still consider a bug. I'll back off anyway.
Thank you for the details. I will look into this.

André