[Panel-devel] [PATCH] Package refactoring

[Paolo Capriotti]

Aaron J. Seigo wrote:
> On Saturday 01 December 2007, Paolo Capriotti wrote:
>> Hi,
>> this is a refactoring of the Package classes, as previously discussed
>> with Aaron. A list of changes:
>> - Icon, preview and screenshots are gone from the metadata. The icon
>> field can still be written in the desktop file by
>> PackageMetadata::write. Packages don't have associated icon, previews
>> and screenshots anymore, 
> 
> this means that we now have to open each package to get at these things. this 
> is going to be too slow for use in package browsers (such as the applet 
> engine). we don't need both "preview" and "screenshot" (not even sure what 
> the difference was supposed to be in the first place, may be they both got 
> added unintentionally), but we do need both an icon and a screenshot 
> available in the .desktop file.

I don't know... opening a package (once we do the optimization of not 
loading the metadata on construction) is essentially a no-op. The actual 
problem is locating it, but I don't think this could be a bottleneck.
By the way, are you thinking of writing the full path to the screenshot 
(which is usually inside the package itself) in the desktop file?
That doesn't seem a good idea. Think of when you move the package around.
The icon issue is different, because one can simply write an icon name, 
as install the icon in the standard location.

> 
>> but they can handle them just like any other
>> file, by setting appropriate entries in their PackageStructure.
> 
> making them also available in the package structure is a good idea, so that 
> the .desktop file is used essentially as a hot-path cache, prevents needing 
> the metadata more than necessary as well as making an obvious way to present 
> this data to the metadata. all good things.

I agree, but I can't see how to do it cleanly.

> 
>> Comments?
> 
> is there a reason why the plasmoidpackgetest includes packages.cpp instead of 
> the header? it should probably just include the header and link to libplasma 
> so as to provide a more realistic test, no?

It should, but PlasmoidPackage is not exported. I'll leave it as it is 
for the moment, but the real solution is to create a PLASMA_TEST_EXPORT 
macro that expands to the export macro only when tests are enabled.

> the rest looks good =)

Ok, thanks, I'll commit, then.

> ideas for down the road:
> 
> we should eventually do some profiling on this class as well to find out where 
> the bumps, if any are, since this will likely end up being used often in 
> performance critical paths. for instance, i really don't know if we need to 
> construct the metadata object until metadata() is called. that's something i 
> can clean up post-commit though.

Agreed, that would speed up opening packages.

> i'm also a little concerned about how easy it will be to write creation tools 
> around this; as it stands they can use the PackageStructure to create the 
> on-disk layout and then call createPackage to have it bundled up. it might be 
> worthwhile, however, to allow absolute paths in PackageStructure for the 
> point of creating packages and have a createPackage that works off of a 
> PackageStructure object.

I'm not sure that having a class serve two purposes is a good idea. The 
plasmagik thing can have a class which behaves like that using 
composition with PackageStructure.
Generally speaking, I'm also not convinced about having functionality in 
  the Package* classes that is specifically thought for the package 
creation process. Even createPackage is a bit out of place there, and 
should be moved to plasmagik, I guess.

> for the purpose of use in creation tools, we may also want to add a 
> PackageStructure::save(KConfigBase*) and PackageStructure::open(KConfigBase*) 
> so that they can be used as part of project files.

Yes, that's something I definitely wanted to do.

> that can wait for plasmagik and/or other design tools to come about though, at 
> which point their needs can push the design a bit more here.

Of course, but let's try not to expose too much of the package creation 
stuff to Package users: all applications are potential users of Package, 
but only one (plasmagik) needs to use createPackage and friends.

Paolo Capriotti