kossebau added subscribers: whiting, kossebau.
kossebau added a comment.

  @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, 
https://phabricator.kde.org/R304:c32c8d002e1216373560c94738841a7a5e1b976b 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?

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D7194

To: apol, leinir
Cc: kossebau, whiting, mutlaqja, broulik, #frameworks

Reply via email to