With the new uds implementation, when some data is meant to replace an old one, it has to use replace() instead of insert() to avoid an assert of data already exists.
Use 3 different entries and only insert().
Details
Details
before: crash in the uds assert
after: passes the test
Diff Detail
Diff Detail
- Repository
- R318 Dolphin
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
@jtamate , can you please use arc diff to upload patches - arc sucks, but having no patch context sucks even more ...
Comment Actions
Updated the patch with context.
It will not work with kio previous to 5.47, unless there is a way to check the kio version at compile time or runtime.
Comment Actions
There is, but maybe a better idea is to bump the minimum required version of KIO at 5.47 (I'm assuming the new UDS implementation is worth having it?)
Comment Actions
Did we break how KIO works and we're now going after the users of KIO and fixing them or was this a "bad usage" from starters that worked by chance?
Comment Actions
We break how KIO works and we're now going after the users of KIO and fixing them.
uds insert() is now only for insert, replace() is for insert or update. Since https://phabricator.kde.org/D12696
| src/tests/kfileitemmodeltest.cpp | ||
|---|---|---|
| 1562 | if you move the insert(UDS_NAME, ...) and insert(UDS_USER, ...) in front of the loop, you can call items.append(entry[i]...) inside the loop. | |
Comment Actions
@dfaure Are you aware we broke KIO backwards compatibility? Seems like we threw out of the window our compatibility promises for our of most important frameworks and really I don't understand why we did that in the sake of performance. If the performance boost was so great we could have introduced V2 classes and make people start using them, but breaking people's code seems really bad to me :/
Comment Actions
Albert, you are of course fully right.
My reasoning was "in practice, [outside of strangely written tests such as the one here], there is zero reason to replace entries in a UDSEntry. All kioslaves either create a new UDSEntry for every file, or do reuse it after calling clear(). And people use if() statements rather than overwriting what they just wrote for the same file, which would be rather inefficient.". And indeed I just checked a lot of kioslaves and they do that.
But ForwardingSlaveBase::prepareUDSEntry in KIO itself, uses insert() to replace UDS_URL, and in fact all of ForwardingSlaveBase should use replace(), as well as users of ForwardingSlaveBase like kio_desktop.
That is troublesome indeed.
The naming is the problem. Naming aside, the old method should still insert-or-replace and some new method should insert-only, but how to call that in a nice way other than insert, especially when 90% of the code out there should ONLY use that? Hmm, fastInsert?
Then if we have fastInsert() and replace(), we can have a deprecated insert() that calls replace().
Comment Actions
But ForwardingSlaveBase::prepareUDSEntry in KIO itself, uses insert() to replace UDS_URL, and in fact all of ForwardingSlaveBase should use replace(), as well as users of ForwardingSlaveBase like kio_desktop.
In ForwardingSlaveBase::prepareUDSEntry, it uses insert() only to insert an UDS URL, because it is done only if the UDS URL is empty, isn't it?
But for UDS_LOCAL_PATH, I guess you're right, it should use replace().
Comment Actions
No, it replaces the URL if it was found, i.e. not empty.
In any case, can you first restore compat?
Comment Actions
Not until dolphin depends on KIO 5.47, currently its minimum required kio version is 5.43.
But it could land as it is and be ready for just change insert by fastInsert when it depends on that kio version.
Comment Actions
Somehow my name and email address in .gitconfig got changed. How? No idea.
Should revert and push again with the right name?
Comment Actions
Ah ok. I thought this had something to do with the CI integration on phabricator.
Then yes, a revert+push is good enough imho.
Comment Actions
That would have happened locally, shouldn't be anything to do with our systems - especially given it's a Travis CI name/address being mentioned.