Detach before setting the d pointer
ClosedPublic

Authored by apol on Aug 7 2017, 4:32 PM.

Details

Summary

Otherwise some objects have their information changed even though they
weren't modified

Diff Detail

Repository
R304 KNewStuff
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.Aug 7 2017, 4:32 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 7 2017, 4:32 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
broulik added a subscriber: broulik.Aug 7 2017, 8:36 PM
broulik added inline comments.
src/core/entryinternal.cpp
170

Detach here also

apol updated this revision to Diff 17863.Aug 8 2017, 2:01 AM

Add missing detach()

leinir accepted this revision.Aug 23 2017, 8:03 AM

Ah, yes, good catch :)

This revision is now accepted and ready to land.Aug 23 2017, 8:03 AM
This revision was automatically updated to reflect the committed changes.

This change causes a bug in loading previews, at least for KStars. Bug here: https://bugs.kde.org/show_bug.cgi?id=385516

@apol @whiting @leinir This change might have broken quite some things: EntryInternal has been an explicitly shared data object for some years (cmp. QExplicitlySharedDataPointer<Private> d;), and quite some logic seems to depend on that. E.g ItemsModel, which keeps a separate copy of the list of EntryInternal objects, does not update those objects on changes, but relies on this to happen automagically due to the explicitely shared data, c32c8d002e12 tries to work-around that, but actually makes things more convoluted.
Same with DownloadWidgetPrivate and Cache, which both keep a set of changed entries as a QSet<EntryInternal>, which they maintain by QSet::insert(), just that this will due to same uniqueId not update the contained item to the latest version. Still those (potentially outdated) objects are then passed via signals to e.g. library consumers, which get outdated or incomplete data (like installedFiles values).
Similar issue with AtticaProvider::entryFromAtticaContent() which also assumes in the logic explicitly shared data so the cache object gets all the updated data as well.

Given this is a public class, this is quite some behavioural change. One could assume this breaks 3rd-party code in a similar way. There is no specification of the behaviour in the API dox though, so unsure myself what is the best solution: https://api.kde.org/frameworks/knewstuff/html/classKNSCore_1_1EntryInternal.html

One fallout is at least this bug: https://bugs.kde.org/show_bug.cgi?id=386156

I had started a patch to fix this, but it has grown a lot to make up for the now no longer explicitely shared nature of this class, and there are still issues in some places. So I wonder if it is better to simply revert this commit instead, document the old behaviour properly in the API and fix the places which rely on non-explicitely shared data objects. That might be only some newer code, given the old behaviour has stayed around for some years.

What do you think?

leinir added a subscriber: sitter.Wed, Oct 25, 8:27 AM

Damn... Well spotted, @kossebau. Right, so immediate (at least temporary solution) to make things not broken would annoyingly enough be to revert the patch, yes... I am now thinking that another oddity noticed by @sitter last week was caused by this as well (going by the installedFiles data being out of sync, it would seem likely it would cause what they were seeing). So... while it feels a bit odd, i would have to vote to revert immediately, and create a new patch documenting why we can't detach in certain classes... Possibly adding in a TODO for Frameworks 6 (there's already a couple of those in kns).

@leinir I now completely gave up on the patch which tried to adapt to the non-shared EntryInternal. As the screenshot loading code and the payload download code rely on the explicitly shared data to attach the fetched screenshots resp. update the download state, that would need some more redesign of logic to update the central EntryInternal storage and from there any interested viewer on that. Which I do not have the energy to try now. So by what I have seen I would vote for a revert as well. + some kf6 notes as you mentioned.

@apol Was this patch created due to some bug seen, or perhaps only of the kind oh-that-seems-wrong-lets-be-nice-and-prevent-things? If the latter that might be handy :)

Too bad that EntryInternal is exposed at all in the KNS API, while there is a public read-only wrapper to it. Seems the person designing this code initially should have named it EntryInternalReallyIMeanIt, but then descendants still might have just helped themselves as now for whatever they needed to ;)

apol added a comment.Wed, Oct 25, 6:00 PM

@apol Was this patch created due to some bug seen, or perhaps only of the kind oh-that-seems-wrong-lets-be-nice-and-prevent-things? If the latter that might be handy :)

I had some issue where I started to see objects changing what they reported over time in Discover. I don't remember what the exact issue was at the moment (It's from early August...).

I don't think it makes a lot of sense to have objects changing values that can't report which values changed, hence adding the detach API.

This already went into one release, and it would be quite useful to get it sorted before the next one rolls around, and the consensus seems to be reverting, as leaving kns with this patch in has some fairly unfortunate side effects. Could we get it reverted before the next release is done?

The point about not reporting value changes is very much a good one, though - i think it would make plenty of sense to add that. It would be useful in any number of places (including e.g. in QtQuick, which obviously is my little baby, if we add it as properties... and it was sort of my mental todo anyway as something to do as i went ahead, unless anybody has large objections to me doing that).

This already went into one release, and it would be quite useful to get it sorted before the next one rolls around, and the consensus seems to be reverting, as leaving kns with this patch in has some fairly unfortunate side effects. Could we get it reverted before the next release is done?

+1

The issues in Discover should hopefully be fixable by adapting to the explicitly-shared nature of the Entry* objects (famous non-maintainer/contributor words :) ).

The point about not reporting value changes is very much a good one, though - i think it would make plenty of sense to add that. It would be useful in any number of places (including e.g. in QtQuick, which obviously is my little baby, if we add it as properties... and it was sort of my mental todo anyway as something to do as i went ahead, unless anybody has large objections to me doing that).

Most KNS classes have already separate signals to report about changes of entries they exposed (driven by KNSCore::Engine::signalEntryChanged), that possibly should be relied on or extended. There still might be issues across threads with that, but then EntryInternal/Entry objects might not be thread-safe anyway?

Looking at the entry-object centric API though I wonder if it might not be better to turn for KNS4 to a usage with a central data model stored by the entry-maintaining model and consumers using indexes to query/set data when needed (seems entries are identifyable by provider + id). That would avoid creating lots of Entry* objects with duplicated data on every change of the data model and all the extra work by consumers to synchronize their copies of the Entry* objects they would maintain.