----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125976/#review88138 -----------------------------------------------------------
In general, I like the idea. We have to be a bit careful to keep the code maintainable though. Please also remove the extraneous debug output. src/kpackage/package.h (line 329) <https://git.reviewboard.kde.org/r/125976/#comment60456> Capital If src/kpackage/package.cpp (line 786) <https://git.reviewboard.kde.org/r/125976/#comment60457> QLatin1Char('/') ? src/kpackage/packagestructure.h (line 80) <https://git.reviewboard.kde.org/r/125976/#comment60458> and an older version (what?)...? src/kpackage/packagestructure.h (line 81) <https://git.reviewboard.kde.org/r/125976/#comment60459> If src/kpackage/private/packagejobthread.cpp (line 321) <https://git.reviewboard.kde.org/r/125976/#comment60460> spaces around <<; why a qWarning without any explanation? src/kpackage/private/packagejobthread.cpp (line 326) <https://git.reviewboard.kde.org/r/125976/#comment60461> Would probably be good to give the installed path here as well. Also, "Impossible *to* remove"... src/kpackage/private/packagejobthread_p.h (line 58) <https://git.reviewboard.kde.org/r/125976/#comment60463> As you note yourself, perhaps just make it an enum? It's not private API, OK, but the code here is getting really convoluted that I think another boolean trap won't help... src/kpackage/private/versionparser.cpp (line 25) <https://git.reviewboard.kde.org/r/125976/#comment60462> I think we can remove most of these includes? - Sebastian Kügler On Nov. 6, 2015, 5:57 p.m., Marco Martin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125976/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2015, 5:57 p.m.) > > > Review request for KDE Frameworks, Plasma and Kai Uwe Broulik. > > > Repository: kpackage > > > Description > ------- > > new job based update() function that compared to install() > if a package with the same pluginId is already installed, > removes the old one before installing the new one, if > and only if the version of the new one is more recent > > > Diffs > ----- > > autotests/plasmoidpackagetest.h f730dce > autotests/plasmoidpackagetest.cpp cccc567 > src/kpackage/CMakeLists.txt 3696f37 > src/kpackage/package.h 4ada8da > src/kpackage/package.cpp 539b21a > src/kpackage/packagestructure.h 9427b42 > src/kpackage/packagestructure.cpp 0070514 > src/kpackage/private/packagejob.cpp 0d2241b > src/kpackage/private/packagejob_p.h 267429f > src/kpackage/private/packagejobthread.cpp ca523b3 > src/kpackage/private/packagejobthread_p.h bf8a266 > src/kpackage/private/versionparser.cpp PRE-CREATION > src/kpackagetool/CMakeLists.txt 78e0fb0 > > Diff: https://git.reviewboard.kde.org/r/125976/diff/ > > > Testing > ------- > > covered by autotests > > > Thanks, > > Marco Martin > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel