----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.vidsolbach.de/r/248/#review240 -----------------------------------------------------------
the PackageMetadata class is really just a wrapper around a bunch of QStrings. i think it makes sense to change Plasma::Package to return a Plasma::PackageMetadata rather than a Plasma::PackageMetadata* and add a copy ctor to PackageMetadata. /trunk/KDE/kdebase/workspace/libs/plasma/package.cpp <http://reviewboard.vidsolbach.de/r/248/#comment194> why is this a static method in Pakage? does this belong in the default impl of PackageStructure::metadata? /trunk/KDE/kdebase/workspace/libs/plasma/package.cpp <http://reviewboard.vidsolbach.de/r/248/#comment192> return new'd objects with vague ownership is asking for leaks. /trunk/KDE/kdebase/workspace/libs/plasma/packagestructure.h <http://reviewboard.vidsolbach.de/r/248/#comment193> who owns the metadata at this point? perhaps call it "createMetadata" instead so it's clear that it's creating something new. the method itself could probably be const as well, so it's really obvious that this is making something new and returning it rather than storing it internally. however, even more sensible would be to have this create a PackageMetadata on demand, store in the Private class and return that object, deleting it on destruction. much safer. - Aaron On 2008-10-31 08:31:28, Petri Damstén wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.vidsolbach.de/r/248/ > ----------------------------------------------------------- > > (Updated 2008-10-31 08:31:28) > > > Review request for Plasma. > > > Summary > ------- > > This adds support for custom package metadata. > > > Diffs > ----- > > /trunk/KDE/kdebase/workspace/libs/plasma/package.h > /trunk/KDE/kdebase/workspace/libs/plasma/package.cpp > /trunk/KDE/kdebase/workspace/libs/plasma/packagestructure.h > /trunk/KDE/kdebase/workspace/libs/plasma/packagestructure.cpp > > Diff: http://reviewboard.vidsolbach.de/r/248/diff > > > Testing > ------- > > > Thanks, > > Petri > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
