[Panel-devel] [PATCH] Package and PackageStructure cleanup

[Aaron J. Seigo]

On Saturday 24 November 2007, Paolo Capriotti wrote:
> Hi,
> while working on background selection with ruphy, I've found some
> oddities in the existing Package code. Here is a cleanup patch that:
> 1) replaces all const char* with QString in the API;

the reason they are const char*, just as in KConfig, is to communicate that 
these are non-translated, abstract keys to the actual data. what benefit is 
there to having them as QStrings? these are, after all, literals.

> 2) changes the Package constructor to a single path argument;

while cleaner (though it should probably be changed from "package" 
to "packagePath" in that case), this put the onus on the app using it to do 
the concatenation.

the reason for the two parameter constructor are the following use cases:

Use case 0: using it with knownPackages:

QString packageRoot= locateLocal("appdata", "packages");
QStringList packages = Package::knownPackages(packageRoot);

foreach (const QString &package, packages) {
    Package p(packageRoot, package);
}

with packageRoot kept separate, the code doesn't need to really know the 
relationship. e.g. what happens if one day we change installPackage to 
install not to packageRoot but to packageRoot.append("plasmagik/")?


Use case 1: using it with installPackage

if (installPackage(pathToPackageArchive, packageRoot)) {
    Package p(packageRoot, packageName);
}


so this preserve both encapsulation as well as makes code easier to write. i'm 
ok with adding a convenience constructor as in your patch, but i'd like to 
keep the other constructor as well.

btw, this made me notice that installPackage doesn't actually return the name 
of the resulting package. that's not much good. it should probably return a 
QString with the package name, with QString() for failure.

> 3) makes some methods in PackageStructure const;

this part looks good.

> 4) updates the only usage of Package I could find, and unit tests.

thanks

> While we are at it, I was wondering if a more fundamental refactoring of
> the Package classes could be useful. Since there is almost no code using
> them, I would like to know what the use cases are, so that we can think
> about improving the API and functionality in the next few days.

use cases include plasmoids and svg themes, examples of each of those can be 
seen in packages.cpp.

the purpose of the Package* classes are:

* to make it possible to create any sort of package using a generic user 
interface that can read the structure of the package and create the resulting 
package. this needs to happen without any code specific to a given 
application or package type in that UI and it also needs to guarantee that 
bad packages (e.g. missing files, etc) will not be generated. bonus points 
for prompting the user as to what sorts of files belong where (that's the 
point of the mimetypes, descriptions, etc)

* to make it possible to open packages of any sort with fairly generic code

* to create new packages of a given type

* to easily locate packages

* to verify package integrity

* to install packages from archives

* to document the structure of packages as a sort of "contract" between the 
source application and other components that may wish to use them

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/panel-devel/attachments/20071124/23af3e0b/attachment.pgp