[Qt5-feedback] Adding better data access method to QAbstractItemModel?

[Stephen Kelly]

Andre Somers wrote:

Hi,

Thanks for bringing up these concerns.

> IMHO, it is always a good idea to think about how the model/view
> interface in Qt can be improved, but I am not sure that this is the
> solution. I have a few doubts, perhaps you can clarify:
> 
> 1) How is this supposed to work in the context of proxy models? As you
> know, proxy models are able to sort (so continuous blocks all of a
> sudden are not continuous at all anymore) and filter (block sizes
> change!). And we are not talking about proxies that actually modify or
> extend the data then. I don't see how this change is going to make
> implementing those easier or more efficient.

If a proxy is filtering only, it means there are multiple contiguous blocks. 
The proxy can get them in a loop. If it is sorting, it degrades to querying 
each row (better than querying each index in the row, which is todays case).

> 
> 2) How will this work with the all-important tree models?

The same way. If there is a model like this:

- 1
- 2
- 3
- - 4
- - 5
- - 6
- - - 7
- - - 8
- - - 9
- - 10
- - 11
- - 12

and QTreeView wants to show:

- - 6
- - - 7
- - - 8
- - - 9
- - 10
- - 11

it calls 

model->rangeData(indexFor(6), indexFor(11), roles)
model->rangeData(indexFor(7), indexFor(9), roles)

Two API calls instead of six.

> 
> 3) I have some doubts to the ease of use of the flat list of QVariants.
> I think it is *very* easy to make mistakes in using that list. You have
> to start calculating offsets for every piece of data you are after. At
> the very least, I think you should then introduce a class like a
> QItemPackage, that provides these offsets for you in a slightly more
> friendly way than a flat list, so you can at least access the EditRole
> of item (4,8) of parent X with ease. You may base that on a QVariantList
> if you want (why not a QVector<QVariant>?), but I don't think passing
> structured data in a flat list is particulary good API.

How would the QItemPackage definition look like?

I think that if you want the EditRole of item(4,8) then you ask for that 
from the model using the current data() API. You don't use the new API for 
that.

Similarly if you want several particular roles from several particular 
indexes, use the more targetted existing data() API.

The new API is only for when you know you want some specific roles from a 
range, such as if you are a view. Your use cases don't suit the new API I 
think.

The view can do things like this (imagining we're handling multiple roles 
with one API call):

// Remember, roles is a list in the accessor.
int offset = roles.indexOf(Qt::SizeHintRole);
QVariantList data = model->rangeData(topLeft, bottomRight, roles);

int position = offset;
while (position < data.size()) {
  QVariant sizeHintData = data.at(position);
  // Do something with the size hint.
  position += offset;
}

If you look into the QTreeView implementation you might understand more.

And yes I fully agree it should be QVector<QVariant>, but if the trolls 
agreed, all the QVariantList in the API would already be QVariantVector by 
now. Changing QVariantList to QVariantVector is an entirely different thread 
though. I think the trolls think the QList API is friendlier. If you propose 
changing the existing API in Qt though, I'd support it.

> 
> 4) The data changed signal can be emitted frequently. Isn't constructing
> a QSet for each call an expensive operation?

I have no idea. If the new API is added, these are the possibilities for the 
view:

1)
onDataChanged(topLeft, bottomRight, roles)
{
  // possibly filter roles through black or whitelist if I know I can 
  // ignore some of them
  QVariantList newData = model->rangeData(topLeft, bottomRight, roles);
}

2)
onDataChanged(topLeft, bottomRight, roles)
{
  // optimization: only call parent once.
  parent = topLeft.parent();
  for (int row ...)
    for (int column ...)
       index = model->index(row, column, parent)
       foreach (role in roles)
         QVariant roleData = index.data(role);
}

Do you really suspect that 2) could be faster than 1)? We call index() a 
bunch of times, and we call data() a bunch of times.

I strongly doubt the creation of a QList or QSet is going to be the rate 
determining step here.

> 
> 5) Shouldn't this package of data also contain the flags() data? You
> need that piece of data anyway for rendering, so you end up querying it
> for each model index again. Or should there just be a Qt::FlagsRole be
> introduced that (also) returns this information? Would relying on _that_
> still be source compatible?

There may be an argument for introducing a 

QList<Qt::ItemFlags> QAIM::itemFlags(QModelIndex topLeft, QModelIndex 
bottomRight)

I think. I see no reason to put that through the data() API now.

> 
> I guess I need to give this more thought. I am not sure if this change
> is worth it.

Are you referring to the additional virtual or changing the signal? Are you 
considering the usecase of gatting the EditRole from item(4, 8) or the use 
case of a view, or the use case of reacting to the dataChanged signal which 
already gives you a range?

Do you see the value of having the dataChanged signal telling you what roles 
have changed? (I don't think you referred to that in your mail). Do you see 
the value in being told that the ContactNameRole or ContactEmailRole has 
changed versus being told the ToolTip role has changed? Have you used QML 
with itemmodels and realised that QML likes changed signals to be more 
specific than less? Consider:

  Text {
    text : model.display
  }

What happens if the tooltip role is changed today? Does the binding get re-
evaluated? Does it need to be? What happens if the binding expression is 
more complex than a simple assignment?

I hope that addresses your concerns. This is definitely worth changing imo. 
And after all, if you don't like the new API, you can safely ignore it.

Thanks,

Steve.