QAbstractItemModel::setItemData partial success

J.Hilk

Hi,

I would expect it to return false.

JonB

@VRonin said in QAbstractItemModel::setItemData partial success:
According to https://stackoverflow.com/questions/13265992/qabstractitemmodel-setitemdata-returns-false-when-inserting-qtuserrole

QAbstractItemModel::setItemData is simply a convenience function which calls setData for each item in the roles map.

bool QAbstractItemModel::setItemData(const QModelIndex &index, const QMap<int, QVariant> &roles)
{
    bool b = true;
    for (QMap<int, QVariant>::ConstIterator it = roles.begin(); it != roles.end(); ++it)
        b = b && setData(index, it.value(), it.key());
    return b;
}

So it's b = b && .... If any fail, it would return false.

VRonin

@JonB

@VRonin said in QAbstractItemModel::setItemData partial success:

P.S.
I know what the current behaviour is, I just want to know what a normal user would expect from that method given the documentation

JonB

@VRonin
Then since the docs don't state it's anyone's guess. Unless you have a pool of "normal users" whom you can poll... ;)

VRonin

@JonB said in QAbstractItemModel::setItemData partial success:

Unless you have a pool of "normal users"

Welcome to the pool my friend. What would you expect?

JonB

@VRonin
Oohhh, thank you for inviting me to participate.

I happen to be working on an SMTP emailing thing atm, which takes multiple recipients to send to. It errors if none could be sent to, otherwise it returns a list of those recipients to whom it could not send.

So I would expect QAbstractItemModel::setItemData to return a list of those roles which it could or could not send to!

Given the bool return result however, if I had to bet I'd concur with @J-Hilk and go for false. But I'd still claim I might as well toss a coin.... :)

kshegunov

false, as any reasonable guess. If you have a compound operation, I'd expect it to fail as a whole whenever any or all of the constituent operations fail.

JKSH

@kshegunov said in QAbstractItemModel::setItemData partial success:

false, as any reasonable guess. If you have a compound operation, I'd expect it to fail as a whole whenever any or all of the constituent operations fail.

Agreed.

Transactions come to mind: If part of the operation fails, I'd expect the whole operation to be rolled back for integrity.

JonB

@JKSH
For the record: setItemData(multiple-roles) does not behave like a transaction: ~~it sets those roles it can, omits those roles it cannot~~, and returns false. No "rollback".

EDIT Looking at the code I posted earlier, the correct description of behaviour is:
It continues setting roles while successful. It stops setting roles as soon as one fails. And finally returns false.

VRonin

@JKSH said in QAbstractItemModel::setItemData partial success:

I'd expect the whole operation to be rolled back for integrity

Yes, that's what sparked my doubt as this is not what's happening in the Qt code

DavidFaure

Returning false is obviously what one would expect (this didn't succeed). But the real question is, should the supported roles be stored, or should the whole thing fail (not making any changes).

If I'm calling setItemData with text, pixmap and tooltip, and the underlying model is a QStringListModel, what should happen?
In the review I argued for "complete failure, nothing happens".

Looking at the specific example above, I can see how one could say "well, if you were calling setData three times instead, then the text would be set, and the pixmap+tooltip would be rejected, and in the end your text would be set". So one could say that for consistency setItemData should set the text.

What's for sure is that the QAIM implementation is wrong, it shouldn't stop at first error (that's based on the order of integral values for the roles... pretty arbitrary). Instead it should set as many as it can, and return false if any failed. Then that would match the above example...

Just some thoughts. I never actually used setItemData myself :-)

JKSH

@DavidFaure said in QAbstractItemModel::setItemData partial success:

Returning false is obviously what one would expect (this didn't succeed). But the real question is, should the supported roles be stored, or should the whole thing fail (not making any changes).

Currently, QAIM saves everything up to the first failure.

If we follow the principle of Transactions, nothing should be saved if any of them are invalid.

If I'm calling setItemData with text, pixmap and tooltip, and the underlying model is a QStringListModel, what should happen?
In the review I argued for "complete failure, nothing happens".

I agree with this argument :)

Looking at the specific example above, I can see how one could say "well, if you were calling setData three times instead, then the text would be set, and the pixmap+tooltip would be rejected, and in the end your text would be set". So one could say that for consistency setItemData should set the text.

I consider the 3x setData() as 3 separate transactions (like executing 3 SQL queries in a row) and setItemData() as 1 transaction.

What's for sure is that the QAIM implementation is wrong, it shouldn't stop at first error (that's based on the order of integral values for the roles... pretty arbitrary). Instead it should set as many as it can, and return false if any failed. Then that would match the above example...

Agreed, stopping at the first error is the worst of both worlds.

My preference is for the strict transaction approach. If that's rejected, I would then go for storing every valid role.

(Having said that, we shouldn't change the existing behaviour before Qt 6 as it risks breaking existing code)

Just some thoughts. I never actually used setItemData myself :-)

That's the perfect scenario for bike-shedding! :-D

Disclaimer: I haven't used it either

VRonin

@JKSH said in QAbstractItemModel::setItemData partial success:

My preference is for the strict transaction approach.

That's easy to implement in specific models like QStringListModel but close to impossible in the generic implementation

@JKSH said in QAbstractItemModel::setItemData partial success:

we shouldn't change the existing behaviour before Qt 6 as it risks breaking existing code

We had cases where this happened before: @SGaist broke a lot my code but I still think changing it was the right choice

VRonin

Quick update, asked this question on the dev mailing list but no traction so far

SGaist

Don't forget it might still be holiday season.

kshegunov

Yep, what @SGaist wrote. My advice is to give it a couple of days more, and if there's nothing try pinging the list.

VRonin

@kshegunov said in QAbstractItemModel::setItemData partial success:

try pinging the list

I won't even try pinging it before September, don't worry. It was more to keep the 3 channels (forum/gerrit/mailing list) aware of each other