KNewStuff: Use d-ptr pattern and make parts internal
Open, NormalPublic

Description

  • Use _p.h name prefix for internal headers
  • Unexport internal classes
  • Use d-ptr for all exported classes
alex created this task.Jan 5 2021, 11:51 AM
alex triaged this task as Normal priority.

i have a use case in mind specifically for the cache (in short, this is the only way we can create links between content and the fact that it's a kns originated thing, without initialising an engine (that is, without phoning home first)). The models we might need to consider, but if they're not exported, we can't easily wrap them in the qtquick components (for example).

As to the title and such, yup, pimplifying all the things needs to happen.

alex added a comment.Jan 5 2021, 12:06 PM

i have a use case in mind specifically for the cache (in short, this is the only way we can create links between content and the fact that it's a kns originated thing, without initialising an engine (that is, without phoning home first))

But there is also the DownloadManager class, it feels like to more appropriate place to put such logic.

leinir added a comment.EditedJan 5 2021, 12:07 PM
In T14010#247580, @alex wrote:

i have a use case in mind specifically for the cache (in short, this is the only way we can create links between content and the fact that it's a kns originated thing, without initialising an engine (that is, without phoning home first))

But there is also the DownloadManager class, it feels like to more appropriate place to put such logic.

It might seem that way, but DownloadManager initialises an engine the moment it's constructed, which is precisely what i want to avoid :)

However, it we say that we want DownloadManager to do that, then it will need a different name (because then it's doing more than downloading stuff), and it will need to be refactored to not initialise the engine until we attempt to perform an action that actually /requires/ the engine to be initialised. It could be done, i'm just not sure what that would actually give us. Arguably we could do that work anyway, leave the functionality in Cache (where the information is), and then add it to DownloadManager (so people can just interact with things through that class if that's really all they want to do).

alex added a comment.Jan 5 2021, 2:06 PM

But we could then debate, if we need the DownloadManager class at all, because it is a rather thin wrapper about the engine. And the engine is not that ugly :P

My concern is that by adding the functionality as a public API to the cache we no longer have consistent code paths. For example we would go through the cache to get installed entries, but if we also want to check if they are updatable we would again need the engine which internally needs a cache. If we were to remove the DownloadManager we could say "use the Cache to check if the entries are installed, but use the engine to check if they are updatable".

Do you understand where I am going?

leinir added a comment.Jan 5 2021, 2:10 PM

Yes, i would be more comfortable with just getting rid of DownloadManager (and... i suspect we might very well be able to just mark it as deprecated, though... a quick glance through does point me to e.g. yakuake, which uses it for that simplistic convenience thing i was talking about above... not sure)

alex added a comment.EditedJan 5 2021, 2:21 PM

And also in kdeplasma-addons for the comic applet.

Edit: I had looked inside of the Yakuake usage and they have basically re-implemented the RemoveDeadEntries option :P

alex updated the task description. (Show Details)Apr 20 2021, 2:35 PM
alex moved this task from Backlog to Waiting on KF6 Branching on the KF6 board.
alex renamed this task from Use d-ptr pattern and make parts internal to KNewStuff: Use d-ptr pattern and make parts internal.